Refactoring conditional statements among different classes

Question

I wonder there is any method of refactoring nested condition. It's just the same content with different contents and different classes which looks bad. I really want to reform this code which looks bad.

My sample code consists of the same conditions with different classes and contents.

public class AService extends BaseService {
    public Map action(AModel aModel throws Exception{
        Map model = new HashMap();

        try {
            if(aModel.getTrStatus().equals(TrStatus.CREATE))
            {
                aDao.add(acrAccessAreaModel);
                aModel.setTrStatus(Constants.TrStatus.UPDATE.toString());

                int attrNo = 0;
                if(aModel.getImgPath()!=null && aModel.getImgPath().getSize()>0)
                    attrNo = fileService.fileWrite(Constants.ServiceId.ACR_AREA, aModel.getAccssAreaId().toString(), aModel.getImgPath());

                if(attrNo != 0)
                {
                    aModel.setAttrNo(attrNo);
                    aDao.modify(acrAccessAreaModel);
                }

                model.put("accAreaInfo", acrAccessAreaModel);
            }
            else if(aModel.getTrStatus().equals(TrStatus.UPDATE))
            {    

                if(aModel.getImgPath()!=null && aModel.getImgPath().getSize()>0)
                {
                    if(aModel.getAttrNo() != null)
                    {
                        FileModel fileModel = new FileModel();                    
                        fileModel.setFileSeq(aModel.getAccssAreaId().toString());
                        fileModel.setAttrNo(aModel.getAttrNo());
                        fileService.remove(fileModel);
                    }

                    int attrNo = 0;
                    attrNo = fileService.fileWrite(Constants.ServiceId.ACR_COLOR, aModel.getAccssAreaId().toString(), aModel.getImgPath());

                    if(attrNo != 0)
                        aModel.setAttrNo(attrNo);
                }

                aDao.modify(acrAccessAreaModel);

            }
            else if(aModel.getTrStatus().equals(TrStatus.DELETE))
            {
                for (Map<String,String> accInfo : aModel.getAccssAreaList()) {
                    if(accInfo.get("attrNo") != null && !accInfo.get("attrNo").equals(""))
                    {
                        FileModel fileModel = new FileModel();
                        fileModel.setFileSeq(accInfo.get("accssAreaId").toString());
                        fileModel.setAttrNo(Integer.parseInt(accInfo.get("attrNo")));
                        fileService.remove(fileModel);
                    }

                    aDao.remove(accInfo);
                    aDao.removeAccArea(accInfo);
                    aDao.removeGate(accInfo);
                }
            }

        } catch (Exception e) {
            throw e;
        }

        return model;
    }

     }



public class BService extends BaseService {

    public Map action(BModel bModel) throws Exception{  

        Map model = new HashMap();
            try{
                if(bModel.getTrStatus().equals(TrStatus.CREATE)){

                    model.put("save",bDao.add(acrAccCtrlStdmModel));

                } else if(bModel.getTrStatus().equals(TrStatus.UPDATE)){

                    model.put("save",bDao.up(acrAccCtrlStdmModel));

                } else if (bModel.getTrStatus().equals(TrStatus.DELETE)){

                    for(Map<String, String> stdmId : bModel.getAcrDelList()){
                        bDao.del(stdmId);
                    }                            
                }
            }catch (Exception e) {
                throw new Exception(e);
            }
            return model;
        }
    }


public class CService extends BaseService {
    public Map aciton(CModel cModel) throws Exception{
        Map model = new HashMap();
        List<Map<String,String>> acrAreaList = null;
        String gateId = "";
        try{
            if(cModel.getTrStatus().equals(TrStatus.CREATE)){

                cDao.saveGate(cModel);
                cModel.setGateId(cModel.getGateId());
                gateId = acrAccCtrlcModel.getGateId().toString();
                acrAreaList = cModel.getAcrAreaList();

                for (Map<String,String> acrArea : acrAreaList) {    
                    acrArea.put("gateId",         gateId);
                    acrArea.put("crerId",         cModel.getCrerId());
                    acrArea.put("updrId",         cModel.getUpdrId());
                    cDao.save(acrArea);
                }            
                model.put("save",acrAccCtrlGateModel);
            }else if (cModel.getTrStatus().equals(TrStatus.DELETE)){

                for(Map<String, String> acrDelList : acrAccCtrlGateModel.getAcrDelList()){
                    cDao.delGate(acrDelList);
                    cDao.del(acrDelList);
                }
            }
        }catch (Exception e) {
            throw new Exception(e);
        }
        return model;
    }
}

Answer 1

Time to review, while I think your second line just have a typ fault I will not consider that.

Implement abstract method with use of generics.

All your Service extends BaseService.
I see the method public Map action(AModel aModel) throws Exception{ for AService.
I see the method public Map action(BModel bModel) throws Exception{ for BService.
I see the method public Map aciton(CModel cModel) throws Exception{ for CService.(I asume typo?)

You can put this abstract method to the BaseService if you use generics.

So it could look like this for BaseService :

public abstract class BaseService<T> {

    public abstract Map action(T model);

}

And for the AService this should be the result :

public class AService extends BaseService<AModel> {

    @Override
    public Map action(AModel model) {

    }
}

if - else if - else if ...

When you see this, it's time to consider switch statements.
I hope the TrStatus is an enum, so you can refactor this to a switch.
If it is a String, you need java 7 or higher to do switch on Strings.

Code can be as followed :

switch (cModel.getTrStatus()) {
    case TrStatus.CREATE:  
        createTrgStatusAction(cmodel);
        break;
    case TrStatus.DELETE:  
        deleteTrgStatusAction();
        break;
     default :  
        // you can put code here what to do if no one of these cases is correct.
        break;
}

Naming of methods, variables and classes

Well, it's sample code and I hope it's because of that, but some of your naming is realy awfull.
AService, BService and CService shall never speak to me that I know what kind of service it is.
BaseService on the other hand is well chosen.
The same as for names of the DAO's => cDao ...

Use constants for (recurring) "magic" String's.

model.put("save",bDao.add(acrAccCtrlStdmModel));

The "save" String comes twice in your class. You could create a constant for that.

private static final String SAVE = "save";

So your code should become :

model.put(SAVE,bDao.add(acrAccCtrlStdmModel));

Implement more methods to reduce method complexity

Like I already showed you with the switch case, make methods what do less things.
This reduce the complexity of your method.
Readability of the code comes higher and troubleshooting goes easier.

private void createTrgStatusAction(CModel cmodel) {
    cDao.saveGate(cModel);
    cModel.setGateId(cModel.getGateId());
    gateId = acrAccCtrlcModel.getGateId().toString();
    acrAreaList = cModel.getAcrAreaList();

    for (Map<String,String> acrArea : acrAreaList) {    
        acrArea.put("gateId", gateId);
        acrArea.put("crerId", cModel.getCrerId());
        acrArea.put("updrId", cModel.getUpdrId());
        cDao.save(acrArea);
    }            
    model.put("save",acrAccCtrlGateModel);
}

As you can see this is already a lit clearder of what it does.
You put yourself a white space just before the for loop.
You seperate 2 different actions there so refactor this method further so it will look somthing like this in the end :

private void createTrgStatusAction(CModel cmodel) {
    updateModel(cmodel);
    saveTheForLoop(cmodel); // Please name this better.         
    model.put("save",acrAccCtrlGateModel);
}

Catch en rethrow error

} catch (Exception e) {
    throw new Exception(e);
}

This is just one bad piece.
You catch an exception, doesn't do a thing with it and creates a new exception to be trown.
First of all, try to catch the detailed exception if possible.
Secondly, If you need to throw the exception back you could use :

throw e;

Third, in this case the whole try catch you can remove cause you trows through the method the exception to higher place.

Use braces for if

if(attrNo != 0)
    aModel.setAttrNo(attrNo);

The intendation is correct but always use braces for if's.
A lot of mistakes with if's comes from this.
Example :

int number;
if (someCondition)
    if (otherCondition)
        number = 5;
else number = 3;

The else counts on what if?
Mine intendation clearly says the first if, will the compiler see that also like this?
If I change mine intendation, the compiler shall still give the same result, but I clearly mean something else at that point.
Putting braces is a small effort to do, you lose a lot more time searching a fault like that.