Code Enhancement Needed for a better design pattern

[Jeansonne Pierre]

Hi Guys,

Kindly need your expertise here to enhanced my code logic in these 2 endpoint class as below,
because my amateur Java level now is stopping me to improve this code.

Question:
1) Seems like i'm using a lot of dummy logic/method. Can this code be made more elegant and with improved clarity?
2) Please correct any mistake that causing performance issue in this code because i feel like im not implementing any good standard design pattern in my code.


Thanks in advanced, your help is much appreciated.

Abstract Class

package com.sg.loanstate.web.contact;

import com.sg.db.contact.dao.NoteDao;
import com.sg.db.contact.model.note.Note;
import com.sg.db.contact.model.note.NoteContentService;
import com.sg.integration.loanstate.obj.common.Status;
import com.sg.integration.loanstate.obj.contact.VoidContactResponse;
import com.sg.rc.db.dao.ContactDoNotCallDao;
import com.sg.util.UtilConstant;
import flexjson.JSONSerializer;
import org.springframework.beans.factory.annotation.Autowired;

public abstract class ContactAbstractController {

    public final static String CONTACT_UPDATE_NOTE_MESSAGE_TEMPLATE = "Customer contact Updated by %s <br> Updated Data:<br> %s";
    public final static String CONTACT_UPDATE_NOTE_SUBJECT_TEMPLATE = "Customer contact Updated";
    public final static String CONTACT_DC_TO_DNC_TEMPLATE = "Contact changed from type Disconnected to DoNotCall";
    public final static String CONTACT_DNC_TO_DC_TEMPLATE = "Contact changed from type DoNotCall to Disconnected";
    public final static String CONTACT_NEW_DNC_TEMPLATE = "Contact updated to DoNotCall";
    public final static String CONTACT_NEW_DC_TEMPLATE = "Contact updated to Disconnected";
    public final static String CONTACT_NA_TEMPLATE = "Contact changed back to active mode";

    String response, ssn, phoneToBeVerified, newPhone, newType, newReason, newCreatedBy;
    @Autowired protected ContactDoNotCallDao contactDoNotCallDao;
    @Autowired protected NoteDao noteDao;

    protected void registerPhone(){
        contactDoNotCallDao.register(newPhone,newType,newCreatedBy,newReason);
    }

    protected String serializedResponse(Status status) {
        VoidContactResponse voidContactResponse = new VoidContactResponse();
        voidContactResponse.setStatusCode(status.getCode());
        voidContactResponse.setMessage(status.getMessage());
        return new JSONSerializer().deepSerialize(voidContactResponse);
    }

    protected void buildNote(){
        String changeBuffer;
        if((newType.equalsIgnoreCase(UtilConstant.CONTACTDNC_TYPE_DNC)) ? true : false){
            changeBuffer = (contactDoNotCallDao.isExists(newPhone))
                    ?newPhone+" : "+CONTACT_DC_TO_DNC_TEMPLATE:newPhone+" : "+CONTACT_NEW_DNC_TEMPLATE;
        }
        else if((newType.equalsIgnoreCase(UtilConstant.CONTACTDNC_TYPE_DC)) ? true : false){
            changeBuffer = (contactDoNotCallDao.isExists(newPhone))
                    ?newPhone+" : "+CONTACT_DNC_TO_DC_TEMPLATE:newPhone+" : "+CONTACT_NEW_DC_TEMPLATE;
        }
        else{
            changeBuffer = newPhone+" : "+CONTACT_NA_TEMPLATE;
        }

        String noteMessage = String.format(CONTACT_UPDATE_NOTE_MESSAGE_TEMPLATE, newCreatedBy, changeBuffer);

        Note.NoteBuilder builder = new Note.NoteBuilder();
        Note note = builder.ssn(ssn)
                .subject(CONTACT_UPDATE_NOTE_SUBJECT_TEMPLATE)
                .message(noteMessage)
                .csrEmail(newCreatedBy)
                .isSystemNote(true)
                .serviceType(NoteContentService.SERVICE_TYPE_COLLECTIONS_CODE)
                .build();

        noteDao.create(note);
    }
}

Endpoint A

package com.sg.loanstate.web.contact;

import com.sg.integration.loanstate.obj.common.Status;
import com.sg.integration.loanstate.obj.contact.VoidContactRequest;
import com.sg.util.UtilConstant;
import flexjson.JSONDeserializer;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseBody;

@Controller
public class ContactDncUpdateController extends ContactAbstractController {

    @RequestMapping(value = "/contact/updateVoidedContact", method = RequestMethod.POST)
    public @ResponseBody String updateVoidedContact(@RequestParam(value = "param", required = true) String request) {

        final JSONDeserializer<VoidContactRequest> deserializer = new JSONDeserializer<>();
        VoidContactRequest voidContactRequest = deserializer.deserialize(request);
        ssn = voidContactRequest.getSsn();
        newPhone = voidContactRequest.getPhone();
        newType = voidContactRequest.getType();
        newReason = voidContactRequest.getReason();
        newCreatedBy = voidContactRequest.getCreatedBy();

        if (contactDoNotCallDao.isExists(newPhone)) {

            switch (newType.toLowerCase()) {
                case UtilConstant.CONTACTDNC_TYPE_DNC:
                    if (contactDoNotCallDao.isDc(newPhone)) {
                        buildNote();
                        registerPhone();
                        response = serializedResponse(Status.SUCCESS);
                    }
                    break;

                case UtilConstant.CONTACTDNC_TYPE_DC:
                    if (contactDoNotCallDao.isDnc(newPhone)) {
                        buildNote();
                        registerPhone();
                        response = serializedResponse(Status.SUCCESS);}
                    break;

                default:
                    contactDoNotCallDao.unregister(newPhone);
                    response = serializedResponse(Status.SUCCESS);
                    break;
            }

        } else {

            if (newType.equalsIgnoreCase(UtilConstant.CONTACTDNC_TYPE_DNC) ||
                    newType.equalsIgnoreCase((UtilConstant.CONTACTDNC_TYPE_DC))) {
                buildNote();
                registerPhone();
                response = serializedResponse(Status.SUCCESS);
            } else {
                response = serializedResponse(Status.ERROR_CRITERIA_NOT_MET);
            }
        }

        return response;
    }

}

Endpoint B

package com.sg.loanstate.web.contact;

import com.sg.integration.loanstate.obj.common.Status;
import com.sg.integration.loanstate.obj.contact.VoidContactRequest;
import flexjson.JSONDeserializer;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseBody;

@Controller
public class ContactDncCheckController extends ContactAbstractController{

    @RequestMapping(value = "/contact/checkVoidedContact", method = RequestMethod.POST)
    public @ResponseBody String checkVoidedContact (@RequestParam(value = "param", required = true) String request){

        final JSONDeserializer<VoidContactRequest> deserializer = new JSONDeserializer<>();
        VoidContactRequest voidContactRequest = deserializer.deserialize(request);


        phoneToBeVerified  = voidContactRequest.getPhone();

        if(contactDoNotCallDao.isExists(phoneToBeVerified)){

            if(contactDoNotCallDao.isDnc(phoneToBeVerified)){
                response = serializedResponse(Status.CONTACT_DNC_FOUND);
            }
            else if(contactDoNotCallDao.isDc(phoneToBeVerified)){
                response = serializedResponse(Status.CONTACT_DC_FOUND);
            }
            else {
                response = serializedResponse(Status.ERROR_CONTACT_NOT_FOUND);
            }
        }
        else{
            response = serializedResponse(Status.ERROR_CONTACT_NOT_FOUND);
        }
        return response;
    }

}

[Winston Gutkowski]

Jeansonne Pierre wrote:
1) Seems like i'm using a lot of dummy logic/method. Can this code be made more elegant and with improved clarity?
2) Please correct any mistake that causing performance issue in this code because i feel like im not implementing any good standard design pattern in my code.

A few things I see:

There's almost never a need to use a trinary operator to return a boolean; and certainly not in an if statement.
if((newType.equalsIgnoreCase(UtilConstant.CONTACTDNC_TYPE_DNC)) ? true : false)
is the same thing as
if((newType.equalsIgnoreCase(UtilConstant.CONTACTDNC_TYPE_DNC))

Your names are very long-winded.

You use two different forms to check for constants: if...else and a switch statement in the same place (lines 27-61 of Endpoint A).
Personally, I prefer the latter; but whatever you choose, be consistent.

You make your code quite tortuous just to have a single return statement (at least I assume that's why you do it).

What about:

    @RequestMapping(value = "/contact/checkVoidedContact", method = RequestMethod.POST)
    public @ResponseBody String checkVoidedContact (@RequestParam(value = "param", required = true) String request) {
        final JSONDeserializer<VoidContactRequest> deserializer = new JSONDeserializer<>();
        VoidContactRequest voidContactRequest = deserializer.deserialize(request);

        phoneToBeVerified  = voidContactRequest.getPhone();
 
        if(contactDoNotCallDao.isExists(phoneToBeVerified)) {
           if(contactDoNotCallDao.isDnc(phoneToBeVerified))
                return serializedResponse(Status.CONTACT_DNC_FOUND);

            if(contactDoNotCallDao.isDc(phoneToBeVerified))
                return serializedResponse(Status.CONTACT_DC_FOUND);
        }

        return serializedResponse(Status.ERROR_CONTACT_NOT_FOUND);
    }

Then it's possible that you don't have to declare 'response' in your superclass (which breaks encapsulation anyway).

I hate to say, but it looks like the sort of code that code generators tend to spew out - verbose and monolithic - but at least they have the excuse that theirs is generally not for human consumption. I presume yours is.

Sorry if it sounds harsh, but you did ask. :-)

HIH

Winston

[Jeansonne Pierre]

Hi Winston Gutkowski,

Thanks a lot for your valuable time and help. I'm sincerely appreciate your valuable guidance and advice.
I have correct my code according to your suggestion and i've learned something from you again. The code looks much better now.
Thanks a lot Winston Gutkowski. Thanks

Feel free to give feedback on my code if you think there's still changes/modification needed. I'll more than happy to learn from my mistake.
Again thanks a lot, Winston Gutkowski.

[Winston Gutkowski]

Jeansonne Pierre wrote:Feel free to give feedback on my code if you think there's still changes/modification needed. I'll more than happy to learn from my mistake.

OK, well there are a few more things I see:

1. Don't scrunch up all your code. Adding whitespace makes it much more readable.

2. On a similar tack: Don't try to do too much in a single statement. Breaking things up makes them more readable, and also helps if you need to debug.

3. You seem to repeat an awful lot of JSON boilerplate. I suspect that this is because your JSON code is too closely coupled with your information code but, not being a JSON expert, I don't know whether it's necessary or not.
It does bother me a bit that you create a new deserializer every time you call one of your methods though. At the very least, I would have thought you could add something like:

protected final VoidContactRequest getVCRequest(String request) {
    JSONDeserializer<VoidContactRequest> deserializer =
       new JSONDeserializer<>();
    return deserializer.deserialize(request);
}

to your superclass, and then use:
VoidContactRequest voidContactRequest = getVCRequest(request);
in your subclass methods.
That way, if there IS a better way of handling the deserializer, you only have to change the code in one place.

4. Whenever you have a big list of static constants, consider using an enum. For example:

public enum ContactTemplate {
    UPDATE_NOTE("Customer contact Updated by %s <br> Updated Data:<br> %s"),
    UPDATE_NOTE_SUBJECT("Customer contact Updated"),
    DC_TO_DNC("%s : Contact changed from type Disconnected to DoNotCall"),
    DNC_TO_DC("%s : Contact changed from type DoNotCall to Disconnected"),
    NEW_DNC("%s : Contact updated to DoNotCall"),
    NEW_DC("%s : Contact updated to Disconnected),
    ACTIVE("%s : Contact changed back to active mode");

    private final String format;
    private final boolean formattable;

    private Contact(String format) {
        this.format = format;
        this.formattable = format.indexOf('%') >= 0;
    }

    public String getMessage(Object... args) {
        return formattable ? String.format(format, args) : format;
    }
}

then your buildNote() code becomes something like:

...
boolean change =  = contactDoNotCallDao.isExists(newPhone);
ContactTemplate template;

switch (newType.toLowerCase()) {
   case UtilConstant.CONTACTDNC_TYPE_DNC:
      template = change ? ContactTemplate.DC_TO_DNC
         : ContactTemplate.NEW_DNC;
      break;
   case UtilConstant.CONTACTDNC_TYPE_DC:
      template = change ? ContactTemplate.DNC_TO_DC
         : ContactTemplate.NEW_DC;
      break;
   default:
      template = ContactTemplate.ACTIVE;
}

changeBuffer = template.getMessage(newPhone);
 
String noteMessage = ContactTemplate.UPDATE_NOTE.getMessage(
   newCreatedBy, changeBuffer);
...

Enums are incredibly useful things, and it's well worth reading up about them.

HIH

Winston

[Jeansonne Pierre]

Hi Winston Gutkowski,

Wao, this is impressive for me. your advice really makes my code more elegant and with improved clarity and more robust.
Really appreciate your help. I will never have this design if you didn't told me. Thanks a lot. This is exactly what i'm looking for. Thanks again.

I have change my ContactAbstractController class as below:

package com.sg.loanstate.web.contact;

import com.sg.db.lion.dao.NoteDao;
import com.sg.db.lion.model.note.Note;
import com.sg.db.lion.model.note.NoteContentService;
import com.sg.integration.loanstate.obj.common.Status;
import com.sg.integration.loanstate.obj.contact.VoidContactResponse;
import com.sg.rc.db.dao.ContactDoNotCallDao;
import com.sg.util.UtilConstant;
import flexjson.JSONSerializer;
import flexjson.JSONDeserializer;
import com.sg.integration.loanstate.obj.contact.VoidContactRequest;

import org.springframework.beans.factory.annotation.Autowired;

public abstract class ContactAbstractController {

    String ssn, phoneToBeVerified, newPhone, newType, newReason, newCreatedBy;
    @Autowired
    protected ContactDoNotCallDao contactDoNotCallDao;
    @Autowired
    protected NoteDao noteDao;

    protected final VoidContactRequest getVoidContactRequest(String request) {
        JSONDeserializer<VoidContactRequest> deserializer =
                new JSONDeserializer<>();
        return deserializer.deserialize(request);
    }

    protected void registerPhone() {
        contactDoNotCallDao.register(newPhone, newType, newCreatedBy, newReason);
    }

    protected String serializedResponse(Status status) {
        VoidContactResponse voidContactResponse = new VoidContactResponse();
        voidContactResponse.setStatusCode(status.getCode());
        voidContactResponse.setMessage(status.getMessage());
        return new JSONSerializer().deepSerialize(voidContactResponse);
    }

    protected void buildNote() {
        boolean change = contactDoNotCallDao.isExists(newPhone);
        ContactTemplate template;
        switch (newType.toLowerCase()) {
            case UtilConstant.CONTACTDNC_TYPE_DNC:
                template = change ? ContactTemplate.DC_TO_DNC
                        : ContactTemplate.NEW_DNC;
                break;
            case UtilConstant.CONTACTDNC_TYPE_DC:
                template = change ? ContactTemplate.DNC_TO_DC
                        : ContactTemplate.NEW_DC;
                break;
            default:
                template = ContactTemplate.ACTIVE;
        }

        String changeBuffer = template.getMessage(newPhone);

        String noteMessage = ContactTemplate.UPDATE_NOTE.getMessage(newCreatedBy, changeBuffer);

        Note.NoteBuilder builder = new Note.NoteBuilder();
        Note note = builder.ssn(ssn)
                .subject(ContactTemplate.UPDATE_NOTE_SUBJECT.getMessage())
                .message(noteMessage)
                .csrEmail(newCreatedBy)
                .isSystemNote(true)
                .serviceType(NoteContentService.SERVICE_TYPE_COLLECTIONS_CODE)
                .build();

        noteDao.create(note);
    }
}

[Winston Gutkowski]

Jeansonne Pierre wrote:Really appreciate your help. I will never have this design if you didn't told me. Thanks a lot. This is exactly what i'm looking for...

You're most welcome.

One of the traps that most beginner (or "out of practise") programmers fall into is that they equate programming with coding. I think you were starting to realise this when you began this thread, because you had a pile of code that lacked cohesion. Coding is a mechanical process that simply involves listing the steps required to solve a problem, whereas design involves stepping back and looking at a problem as a whole, and trying to work out the tools or approaches that will help to solve it.

The trouble is: coding is more satisfying. It's relatively simple, and we feel like we're "getting something done". For small problems, it's also quite possible to translate straight to code with decent results, but it doesn't scale well as a methodology. And IMO, the two processes are almost mutually exclusive: You can't do one while you're doing the other.

So my advice:

Resist the urge to code.

Before you start to code, think about what you want to do, and the classes (or enums) you need to do it. You might find some help in the WhatNotHow (←click) page.

When you get stuck: StopCoding (←click).

If you think you're "missing" something, STOP, and work out if you might need a new class. Beginners, in general, don't write nearly enough classes.

I like to think of a computer program as a house or a landscape garden. Both need workers and carpenters and plumbers to fit things together, but they also need an architect - ie, someone with the "vision" of what the thing will look like (or do) when it's complete. And an architect doesn't deal with piping or plumbing; not because he (or she) can't do it, but because it's a distraction. Similarly, a carpenter or joiner can't solve design problems.

Hope it helps; and good luck.

Winston