Duplication with nested null checks

Question

How can I improve this code with nested null check statements?

public class AcrCategoryService extends BaseActionService<AcrCategoryModel> {

    @SuppressWarnings({ "rawtypes", "unchecked" })
    public Map getCategoryList(String categoryCd, String categoryNm, String rgstTrgt) throws Exception{        
        Map model = new HashMap();
        fileService.setIsGetFile(true);

        try {            

            List<AcrCategoryModel> cateData = acrCategoryDao.getCategoryList(categoryCd, categoryNm, rgstTrgt);

            for(AcrCategoryModel acr : cateData)
            {
                if(acr.getAttrNo() == null)
                {
                    System.out.println("------------------------getAttrNo is null");
                } else
                {
                    System.out.println("======getAttrNo===================>>>>>>>>>>>>>>>>>"+acr.getAttrNo());
                    FileModel file = (FileModel)fileService.get(acr.getAttrNo()).get("fileList");
                    acr.setListImg("<img src='"+file.getFilebyte()+"' height='25' width=100>");
                }
            }
            model.put("categoryList", cateData);    

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

        return model;
    }

    @SuppressWarnings({ "rawtypes", "unchecked" })
    public Map get(String categoryId, Integer attrNo) throws Exception{
        Map model = new HashMap();
        fileService.setIsGetFile(true);
        try {
            AcrCategoryModel categoryInfo = (AcrCategoryModel)acrCategoryDao.get(categoryId);
            model.put("categoryInfo", categoryInfo);

            List<AcrCategoryModel> subCateList = acrCategoryDao.getSubCate(categoryId);
            model.put("subCateList", subCateList);

            List<AcrCategoryModel> accAreaList = acrCategoryDao.getAccArea(categoryId);
            model.put("accAreaList", accAreaList);

            if(categoryInfo.getAttrNo() == null)
            {
            } else
            {
                model.put("fileList", fileService.getList(categoryId, categoryInfo.getAttrNo()).get("fileList"));
            }

        } catch (Exception e) {
            throw e;
        }

        return model;
    }
    }

    public class AcrColorService extends BaseActionService<AcrColorModel> {

    @Autowired
    private AcrColorDao acrColorDao;
    @Autowired
    private FileService fileService;

    @SuppressWarnings({ "rawtypes", "unchecked" })
    public Map getColorList(String colorCd, String colorNm) throws Exception{        
        Map model = new HashMap();
        fileService.setIsGetFile(true);

        try {            

            List<AcrColorModel> colorData = acrColorDao.getColorList(colorCd, colorNm);

            for(AcrColorModel acr : colorData)
            {
                if(acr.getAttrNo() == null)
                {
                    System.out.println("------------------------getAttrNo is null");
                } else
                {
                    System.out.println("======getAttrNo===================>>>>>>>>>>>>>>>>>"+acr.getAttrNo());
                    FileModel file = (FileModel)fileService.get(acr.getAttrNo()).get("fileList");
                    acr.setListImg("<img src='"+file.getFilebyte()+"' height='15' width=200>");
                }
            }
            model.put("colorList", colorData);    

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

        return model;
    }

    public void deleteTrgStatusAction(AcrColorModel colorModel) throws Exception {
        // TODO Auto-generated method stub
        for (Map<String,String> colorInfo : colorModel.getColorList()) {
            if(colorInfo.get("attrNo") != null && !colorInfo.get("attrNo").equals(""))
            {
                FileModel fileModel = new FileModel();
                fileModel.setFileSeq(colorInfo.get("colorId").toString());
                fileModel.setAttrNo(Integer.parseInt(colorInfo.get("attrNo")));
                fileService.remove(fileModel);
            }

            acrColorDao.remove(colorInfo);
        }
    }


    public void updateTrgStatusAction(AcrColorModel colorModel, Map model) throws Exception {
        // TODO Auto-generated method stub
        if(colorModel.getImgPath()!=null && colorModel.getImgPath().getSize()>0)
        {
            if(colorModel.getAttrNo() != null)
            {
                FileModel fileModel = new FileModel();                    
                fileModel.setFileSeq(colorModel.getColorId().toString());
                fileModel.setAttrNo(colorModel.getAttrNo());
                fileService.remove(fileModel);
            }

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

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

        acrColorDao.modify(colorModel);
    }

    @SuppressWarnings({ "rawtypes", "unchecked" })
    public void createTrgStatusAction(AcrColorModel colorModel, Map model) throws Exception {
        // TODO Auto-generated method stub
        acrColorDao.add(colorModel);
        colorModel.setTrStatus(Constants.TrStatus.UPDATE);

        int attrNo = 0;
        if(colorModel.getImgPath()!=null && colorModel.getImgPath().getSize()>0)
            attrNo = fileService.fileWrite(Constants.ServiceId.ACR_COLOR, colorModel.getColorId().toString(), colorModel.getImgPath());

        if(attrNo != 0)
        {
            colorModel.setAttrNo(attrNo);
            acrColorDao.modify(colorModel);
        }

        model.put("colorInfo", colorModel);
    }
    @SuppressWarnings({ "rawtypes", "unchecked" })
    public Map get(String colorId) throws Exception{
        Map model = new HashMap();
        fileService.setIsGetFile(true);
        try {
            AcrColorModel acrColorInfo = (AcrColorModel)acrColorDao.get(colorId);
            model.put("colorInfo", acrColorInfo);

            if(acrColorInfo.getAttrNo() == null)
            {

            } else
            {
                model.put("fileList", fileService.getList(colorId, acrColorInfo.getAttrNo()).get("fileList"));
            }

        } catch (Exception e) {
            throw e;
        }

        return model;
    }
}

I hate nested null check among different classes in this code. Can I make a method and move it up to a super class?

Answer 1

Use rawtypes is even worse than null check!

Your

Map model = new HashMap();

should be

Map<String, List<AcrCategoryModel>>  model = new HashMap<String, List<AcrCategoryModel>>();

or if you use Java >= 7

Map<String, List<AcrCategoryModel>>  model = new HashMap<>();

Do the same for others. With this you remove the useless casting!

---

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

} catch (Exception e) {
    throw e;
}

2 problems here:

1. You catch the exception but then rethrow the same without a message/or similar how can this be helpful? You should throw another exception which is more specific in your case or just remove the catch.

2. Don't catch Exception! If it's IOException catch it. Then if you want, you can throw another if you think exists another exception which helps the programmer to understand better the error.

---

if(categoryInfo.getAttrNo() == null)
{
} 
else
{
   model.put("fileList", fileService.getList(categoryId, categoryInfo.getAttrNo()).get("fileList"));
}

If you need to do something if it's null, do something or just ignore it. Your code can be just:

if(categoryInfo.getAttrNo() != null)
{
   model.put("fileList", fileService.getList(categoryId, categoryInfo.getAttrNo()).get("fileList"));
}

And fix your indentation!

return model;
}
}

Note the two }} this let me think: "he closed two times the method?" then i noticed it's the method and class close. With indentation the code will be clear for you and others.

public Map get(String categoryId, Integer attrNo)

Avoid use of boxed natives types this could create problems if you don't be careful.

---

if(colorInfo.get("attrNo") != null && !colorInfo.get("attrNo").equals(""))

What about cache .get("attrNo") value?

String attrNo = colorInfo.get("attrNo");
if(attrNo  != null && !attrNo.equals(""))

I would like to talk about how to avoid null checks at the end of the answer, but i give an hint here:

You can avoid the null check if inverting your .equals condition.

"".equals(attrNo)

It's legal, so if attrNo is null it will return false.

Ah, if you need to check if the string is empty use .isEmpty() or .length()!

(p.s since you cache attrNo you will use it inside fileModel.setAttrNo(Integer.parseInt(colorInfo.get("attrNo"))); too)

---

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

What about

int attrNo = fileService.fileWrite(Constants.ServiceId.ACR_COLOR, colorModel.getColorId().toString(), colorModel.getImgPath());

?

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

Add the { } you code will be more clear and in future you can edit your code without think two times about it.

if(attrNo != 0) {
   colorModel.setAttrNo(attrNo);
}

---

The same of above here

    if(colorModel.getImgPath()!=null && colorModel.getImgPath().getSize()>0)
       attrNo = fileService.fileWrite(Constants.ServiceId.ACR_COLOR, colorModel.getColorId().toString(), colorModel.getImgPath());

---

It should be all, now how to avoid null checks?

It depends. What your code returns? What you except to return in case of error? Can you create a Object which rappresents a "no-valid-value"?

Strings

For strings, as i said above, when you compare them if you can use literal.equals(variable) or try to put at the left side the least-probable-null (ok, i coined this term).

Example:

Suppose you have a List of String which will not contains null strings and you want to compare a string and check if it's inside this collection. You will not write yourSearchString.equals(stringInsideCollection) because yourSearchString is more probabile to be null, instead you are sure stringInsideCollection will never be null. So why make a NullPointerException if we can avoid it?

Another thing you can do with strings is never return null, instead return an empty string so you can do return ""; (i know it look ugly) and in your caller code you can just do if (returnedString.isEmpty()) so you merge together two conditions: You check if the server returned a valid response and if it's empty you don't want to do something.

Collections

Not so much to say about it, JDK helped us with Collections.emptyList and others for all kind of collections types. Return it instead of null so you don't need to check null.

Arrays

Like Collections you can return an empty array.

Custom classes

It's a bit hard here, can you create a "default" status to your object?

An example of a "Default" status of a Point class would be a Point which always points to [0, 0]...

If you can, return it instead of null!

public class Point {
 public final int x;
 public final int y;

 public static final Point MAIN_POINT = new Point(0, 0);

 public Point(int x, int y) { this.x = x; this.y = y; }
}

so when you do an operation you could return MAIN_POINT instead of null.

Yeah, it looks great. But it could be a problem. What makes [0, 0] (example) a good thing to return instead of null or of point [5, 3]?

Sometimes instead of return null you could throw an exception! ... or leave null. Sometimes null is a great alternative to an exception (which costs).

To reduce your null checks we need more info about your getAttrNo() method. If it's something which shouldn't be null, when you set it check if it's null, if yes throw a IllegalArgumentException.

(i like null)

Answer 2

• Remove @SuppressWarnings and remove the warnings by fixing your code.
• Don't use System.out.println for logging
• Don't catch unspecified Exception