3
\$\begingroup\$

The original code: Refactoring conditional statements among different classes

I am now doing it continually. I fixed a few part as mentioned.

public abstract class BaseService<T> {

    protected Logger logger = Logger.getLogger(this.getClass());
    protected static final String SAVE = "save";

    public abstract Map action(T model) throws Exception;

    }

    public class AcrAccCtrlGateService extends BaseService<AcrAccCtrlGateModel> {
    @Autowired
     private AcrAccCtrlGateDao acrAccCtrlGateDao;
    @Override
    public Map action(AcrAccCtrlGateModel acrAccCtrlGateModel) {
        // TODO Auto-generated method stub
        Map model = new HashMap<String, Object>();
        switch (acrAccCtrlGateModel.getTrStatus()) {
            case CREATE:  
                createTrgStatusAction(acrAccCtrlGateModel);
                model.put(SAVE,acrAccCtrlGateModel);
                break;
            case DELETE:  
                deleteTrgStatusAction(acrAccCtrlGateModel);
                break;
             default :  
                // you can put code here what to do if no one of these cases is correct.
                break;
        }
        return model;
    }

    private void deleteTrgStatusAction(AcrAccCtrlGateModel acrAccCtrlGateModel){
        for(Map<String, String> acrDelList : acrAccCtrlGateModel.getAcrDelList()){
            acrAccCtrlGateDao.delGate(acrDelList);
            acrAccCtrlGateDao.del(acrDelList);
        }
    }

    private void createTrgStatusAction(AcrAccCtrlGateModel acrAccCtrlGateModel) {
        updateModel(acrAccCtrlGateModel);
        saveTheForLoop(acrAccCtrlGateModel);         
    }

    private void updateModel(AcrAccCtrlGateModel acrAccCtrlGateModel){
        acrAccCtrlGateDao.saveGate(acrAccCtrlGateModel);
        acrAccCtrlGateModel.setGateId(acrAccCtrlGateModel.getGateId());
    }
    private void saveTheForLoop(AcrAccCtrlGateModel acrAccCtrlGateModel){
        String gateId = acrAccCtrlGateModel.getGateId().toString();
        List<Map<String,String>> acrAreaList = acrAccCtrlGateModel.getAcrAreaList();
        for (Map<String,String> acrArea : acrAreaList) {    
            acrArea.put("gateId",         gateId);
            acrArea.put("crerId",         acrAccCtrlGateModel.getCrerId());
            acrArea.put("updrId",         acrAccCtrlGateModel.getUpdrId());
            acrAccCtrlGateDao.save(acrArea);
        }            
    }
}

public class AcrAccCtrlStdmService extends BaseService<AcrAccCtrlStdmModel> {

    @Autowired
    private AcrAccCtrlStdmDao acrAccCtrlStdmDao;

    @SuppressWarnings({ "rawtypes", "unchecked" })
    public Map action(AcrAccCtrlStdmModel acrAccCtrlStdmModel) throws Exception{

        Map model = new HashMap();
        try{
            switch (acrAccCtrlStdmModel.getTrStatus()) {
                case CREATE:  
                    model.put(SAVE,acrAccCtrlStdmDao.add(acrAccCtrlStdmModel));
                    break;
                case UPDATE:  
                    model.put(SAVE,acrAccCtrlStdmDao.up(acrAccCtrlStdmModel));
                    break;
                case DELETE:  
                    for(Map<String, String> stdmId : acrAccCtrlStdmModel.getAcrDelList()){
                        acrAccCtrlStdmDao.del(stdmId);
                    }                
                    break;
                 default :  
                    // you can put code here what to do if no one of these cases is correct.
                    break;
            }

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

public class AcrAccessAreaService extends BaseService<AcrAccessAreaModel> {
    @Autowired
    private AcrAccessAreaDao acrAccessAreaDao;
    @Autowired
    private FileService fileService;
    @SuppressWarnings("rawtypes")
    public Map action(AcrAccessAreaModel acrAccessAreaModel) throws Exception{
        Map model = new HashMap();

        try {

            switch (acrAccessAreaModel.getTrStatus()) {
                case CREATE:  
                    createTrgStatusAction(acrAccessAreaModel);
                    model.put("accAreaInfo", acrAccessAreaModel);
                    break;
                case UPDATE:  
                    updateTrgStatusAction(acrAccessAreaModel);
                    break;
                case DELETE:  
                    deleteTrgStatusAction(acrAccessAreaModel);
                    break;
                 default :  

                    break;
            }

        } catch (Exception e) {
            throw e;
        }

        return model;
    }

    private void deleteTrgStatusAction(AcrAccessAreaModel acrAccessAreaModel) throws Exception {
        // TODO Auto-generated method stub
        for (Map<String,String> accInfo : acrAccessAreaModel.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);
            }

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

    private void updateTrgStatusAction(AcrAccessAreaModel acrAccessAreaModel) throws Exception {
        // TODO Auto-generated method stub
        if(acrAccessAreaModel.getImgPath()!=null && acrAccessAreaModel.getImgPath().getSize()>0)
        {
            if(acrAccessAreaModel.getAttrNo() != null)
            {
                FileModel fileModel = new FileModel();                    
                fileModel.setFileSeq(acrAccessAreaModel.getAccssAreaId().toString());
                fileModel.setAttrNo(acrAccessAreaModel.getAttrNo());
                fileService.remove(fileModel);
            }

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

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

        acrAccessAreaDao.modify(acrAccessAreaModel);
    }

    private void createTrgStatusAction(AcrAccessAreaModel acrAccessAreaModel) throws Exception {
        // TODO Auto-generated method stub
        acrAccessAreaDao.add(acrAccessAreaModel);
        acrAccessAreaModel.setTrStatus(TrStatus.UPDATE);

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

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

I feel better about this code and am wondering about some things:

  1. Is there any benefit with using generic BaseService<T>?

  2. Could I apply strategy, state pattern or polymorphism with this code? If it's possible, could you please give me advice?

Could you look at this as well?

Refactoring duplicated code

\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

Have you heard about the term programming for interface not implementation? If not, then look it up. That is the reason why there should be a abstract class/interface. It makes the things like what I suggested in the linked question possible.

This code can be improved further. Placing another generic type variable U you can place the dao's declaration in the abstract class. It will make it clear that for the particular service what is the model and the DAO at class declaration itself.

public abstract class BaseService<T, U> {  
    protected Logger logger = Logger.getLogger(this.getClass());
    protected static final String SAVE = "save";

    @Autowired
    protected U curDAO;

    public abstract Map action(T model) throws Exception;
}

That is all I can say about this. Hope this helps.

EDIT

Regarding use of generic in the method I am starting to think that in this case it is not a good idea. Use of interface could be better. I have seen generic methods work wonders but that is when the method body is using those generics. Here it is only placing a compile time limitation. If the requirement is to limit the method parameters to a particular type of classes then Interface is the thing to use.

So I will change the code to something like this

public abstract class BaseService {  
    protected Logger logger = Logger.getLogger(this.getClass());
    protected static final String SAVE = "save";

    @Autowired
    protected DAOInterface curDAO;

    public abstract Map action(ModelInterface model) throws Exception;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.