Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question

2 Answers 2

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)

share|improve this answer
1  
Corrected minor spelling mistake, otherwise awesome answer. +1. –  h.j.k. Jun 10 at 0:00
    
I disagree with returning an empty list instead of null. This may lead to problems in your business logic if there is a case where an empty list is a valid output (or will be so in future). Therefore, if it is expected from a method to return some content but the method can't because of invalid arguments/state or anything else it should throw an exception instead and not return an "uncommon" value. –  valenterry Jun 10 at 16:30
    
@valenterry Yes as i said don't exists a general way to avoid nulls sometimes you just need to accept them. Anyway there are cases where an empty list is ok, better than nuĺl. If a method is excepted to return some things means that you can avoid a null check in eromg cases, use .size directly and an else with "something went wrong" –  Marco Acierno Jun 11 at 0:59
    
@MarcoAcierno You don't need to accept them. You can either use Option Types (or similiar types, depending on the language) or - if null is not a valid return value as for example in databases - then you just throw exceptions. null should only be used for legacy compatibility or if the language is so poor that it does not provide any other mechanism. –  valenterry Jun 11 at 10:31
    
@valenterry Java have Optional since v. 8 (or before with Lib) but personally i never see null as a problem it's normal that a pointer could be null and you can handle this case without problems. (but it's a my opinion, of course) maybe for others null could be see as a problem, especially when null could happen after a product is released and not handled correctly could do disasters, but it's something of "natural". –  Marco Acierno Jun 11 at 11:57
  • Remove @SuppressWarnings and remove the warnings by fixing your code.
  • Don't use System.out.println for logging
  • Don't catch unspecified Exception
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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