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 simplify this?

if ((principal == null || principal.getName() == null)) {
            boolean isValidInputData = true;
            if ((!reCaptchaBean.isValidCaptcha(recapchaResponse))) {
                model.addAttribute("captchaError", messageSource.getMessage("СaptchaError", null, LOCALE_RU));
                isValidInputData = false;
            }
            if (!validateAddParamsPageForDemoMode(campaignBeanDto, bindingResult)) {
                isValidInputData = false;
            }
            if (!isValidInputData) {
                model.addAttribute("useCampaignBeanDto", true);
                return createCompanyAddParams(principal, model, session);
            }
}
share|improve this question
2  
It'll help (maybe slightly) if you can provide the method name/signature too, and a brief description of what this is supposed to do. –  h.j.k. Apr 20 at 10:52
    
What does condition (principal == null || principal.getName() == null) mean? Maybe it stands for !userAuthenticated()? –  iTollu Apr 20 at 11:34
    
@iTollu good point –  gstackoverflow Apr 20 at 11:57
    
@h.j.k. my method enough huge but provided snippet looks ugly –  gstackoverflow Apr 20 at 11:58

2 Answers 2

First, we don't need the outer check for simplifying, it can't be improved much.

boolean isValidInputData = true;
if ((!reCaptchaBean.isValidCaptcha(recapchaResponse))) {
    model.addAttribute("captchaError", messageSource.getMessage("СaptchaError", null, LOCALE_RU));
    isValidInputData = false;
}
if (!validateAddParamsPageForDemoMode(campaignBeanDto, bindingResult)) {
    isValidInputData = false;
}
if (!isValidInputData) {
    model.addAttribute("useCampaignBeanDto", true);
    return createCompanyAddParams(principal, model, session);
}

Now, abstract the statements away. I'm assuming there's no side effects in your validations here.

boolean controlA = true;
if (!conditionA) {
    functionA();
    controlA = false;
}
if (!conditionB) {
    //no special code executed.
    controlA = false;
}
if (!controlA) {
    functionB();
}

If not conditionA, then functionA. If not conditionA or not conditionB, then controlA is false. If controlA is false, then functionB.

We can remove some of the nots here, make it more clear.

boolean controlA = false;
if (!conditionA) {
    functionA();
    controlA = true;
}
if (!conditionB) {
    //no special code executed.
    controlA = true;
}
if (controlA) {
    functionB();
}

This makes the logic as such: If not conditionA, then functionA. If not conditionA or not conditionB, then controlA is true. If controlA is true, then functionB.

Now, if attribute order is important, then we must implement "if not conditionA then functionA" first.

if (!conditionA) {
    functionA();
}

Then after that comes the next statement, If not conditionA or not conditionB, then controlA is true.

if(!conditionA || !conditionB) {
    controlA = true;
}

And the last statement, If controlA is true, then functionB.

if(controlA) {
    functionB();
}

The combined version looks like this:

controlA = false;
if (!conditionA) {
    functionA();
}
if(!conditionA || !conditionB) {
    controlA = true;
}
if(controlA) {
    functionB();
}

It makes no sense, however, to have a structure like this:

if(!conditionA || !conditionB) {
    controlA = true;
}
if(controlA) {
    functionB();
}

We're far better off by changing "If not conditionA or not conditionB, then controlA is true. If controlA is true, then functionB." to "If not conditionA or not conditionB, then functionB."

if(!conditionA || !conditionB) {
    functionB();
}

Makes for a total structure of this:

if (!conditionA) {
    functionA();
}
if (!conditionA || !conditionB) {
    functionB();
}

Let's translate that back into code:

if (!reCaptchaBean.isValidCaptcha(recapchaResponse)) {
    model.addAttribute("captchaError", messageSource.getMessage("СaptchaError", null, LOCALE_RU));
}
if (!reCaptchaBean.isValidCaptcha(recapchaResponse) || !validateAddParamsPageForDemoMode(campaignBeanDto, bindingResult)) {
    model.addAttribute("useCampaignBeanDto", true);
    return createCompanyAddParams(principal, model, session);
}

But this contains double work, as we're validating the captcha twice.

So I'd suggest storing this in a boolean:

boolean captchaValid = reCaptchaBean.isValidCaptcha(recapchaResponse);
if (!captchaValid) {
    model.addAttribute("captchaError", messageSource.getMessage("СaptchaError", null, LOCALE_RU));
}
if (!captchaValid || !validateAddParamsPageForDemoMode(campaignBeanDto, bindingResult)) {
    model.addAttribute("useCampaignBeanDto", true);
    return createCompanyAddParams(principal, model, session);
}

Makes the end result look like this:

if (principal == null || principal.getName() == null) {
    boolean captchaValid = reCaptchaBean.isValidCaptcha(recapchaResponse);
    if (!captchaValid) {
        model.addAttribute("captchaError", messageSource.getMessage("СaptchaError", null, LOCALE_RU));
    }
    if (!captchaValid || !validateAddParamsPageForDemoMode(campaignBeanDto, bindingResult)) {
        model.addAttribute("useCampaignBeanDto", true);
        return createCompanyAddParams(principal, model, session);
    }
}

It's not much clearer though.

Also, in case you DO have side-effects from validateAddParamsPageForDemoMode(campaignBeanDto, bindingResult), then this can be worked back in too:

if (principal == null || principal.getName() == null) {
    boolean captchaValid = reCaptchaBean.isValidCaptcha(recapchaResponse);
    if (!captchaValid) {
        model.addAttribute("captchaError", messageSource.getMessage("СaptchaError", null, LOCALE_RU));
    }
    if (!validateAddParamsPageForDemoMode(campaignBeanDto, bindingResult) || !captchaValid) {
        model.addAttribute("useCampaignBeanDto", true);
        return createCompanyAddParams(principal, model, session);
    }
}

Like that, both validation functions will be called before createCompanyAddParams.


By recognizing the statement "If not conditionA or not conditionB, then controlA is false. If controlA is false, then functionB." to be "Only if not conditionA or not conditionB, controlA is false. If controlA is false, then functionB.", it becomes easier to see that there's no need for the control.

Only if !cA || !cB, controlA. If controlA, functionB. So if !cA || !cB, functionB.

This simplifying can only be done once you've removed the specifics of the code, as what specific code is executed is too much detail for pruning branches.

share|improve this answer
    
method validateAddParamsPageForDemoMode should executes independently captchaValid –  gstackoverflow Apr 20 at 12:18
    
@gstackoverflow fixed, right? –  Pimgd Apr 20 at 12:20
    
@Pimgd dont you think your suggestion just mechanicaly reduced the code? On the other side, it complicated the readability –  Zavael Apr 20 at 12:40
    
@Zavael "It's not much clearer though." I don't think it's clearer. It's simpler, but it doesn't add much clarity. I can't tell whether it complicates the code either, since I spent quite a bit of time looking at it. –  Pimgd Apr 20 at 12:44

Additional information would be helful like method signature, method purpose described or code commented, so I will add at least some general advice.

Everytime if I see lots of ifs or if-else, bad design comes to my mind. There are of course exceptions, but mostly if I am going to create 3 and more conditions, I start to think about refactoring my code.

If you are interested only in simplyfing (reducing) code, Pimgd answer is good enough. If not, you have to provide more info about the code.

This code - in the state as it is - is hard to simplify, because it has some functionalities in there which are hidden/grouped in the functions validateAddParamsPageForDemoMode and createCompanyAddParams.

From the name of them, it seems that they are not related (one is validating, the other is creating the response maybe) that violates the SOLID principles which i use as a good guide. Mostly the first one - Single responsibility principle which could take you to the right way to refactor your code.

...the single responsibility principle states that every class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class

In other words, it should have only one reason to change. I understand it as "one reason to change = one responsibility" (i think "Uncle" Bob Martin defined this). Whats is the goal of the code snippet or the whole method? Authentication (principal) check? Capcha validation? Data validation? Creating company parameters? If you can decide this, it can show you, what should be externalized in other class.

I am not sure if the validateAddParamsPageForDemoMode is only validating (by name it should only validate inputs) and in that case it is not needed to execute it in case the capcha fails. It adds nothin to the method return. If thats true, i would fast fail right from the first if condition.

I wouldnt hold the validateAddParamsPageForDemoMode in the same class as createCompanyAddParams. The first one is validating, the second one is creating result it seems. Two reasons to change, two responsibilities. But this is not only about the code snippet, but the code around it - in the class. I think one of them should go to another class.

Next, the usage of null is pretty unfortunate, if you could get rid of it or switch it with null object pattern, it would help you a lot. If it cannot be changed, I would externalize the null principal checking to standalone instance of "Principal" class with method isValid() that returns boolean, because everytime the bussines rule for satisfying the "principal needs" would change, you would have only one place to change the code.

SIDENOTE: If the SOLID principles are too abstract to understand, the Law of Demeter helped me at the start to write the code better. But in your case we dont know, where does reCaptchaBean, model and messageSource come from.

share|improve this answer
    
Why would use of Law of Demeter improve the code? How do you suggest changing the code to adhere to the single responsibility principle? –  Pimgd Apr 20 at 12:26
    
its hard to speak about this code snippet as we have little background, but as the author stated, it is just a snippet from huge method and I suggested him to simplify it, where the LOD can help –  Zavael Apr 20 at 12:30
1  
I'm not sure how generic advice helps if you can't say how it applies to the code. Make some assumptions, treat them as axioms. ... Put differently, I know why you'd use SRP and LOD, and I don't see how you'd implement them here. I'd need more context before starting to make such a suggestion, because for all I know, this function could be "handling the web request", and that's a single responsibility. –  Pimgd Apr 20 at 12:37
    
@Pimgd i tried to clear my answer a little with some suggestions and defining the single responsibility... yeah its hard to know the granularity of the SRP, there is no "one right answer" –  Zavael Apr 20 at 14:24

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.