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.
(principal == null || principal.getName() == null)
mean? Maybe it stands for!userAuthenticated()
? – iTollu Apr 20 at 11:34