0
\$\begingroup\$

Given the below code is there a way to eliminate the else clause from the below method? In my workplace else clause is considered evil because it somehow decreases the readability of the code.

public boolean validateAndSetDateRelatedSessionAttributes(String startDateFromParam, String endDateFromParam, HttpSession session) throws ParseException {
    boolean isDateRangeValid = true;
    final boolean requestContainsCustomDate = requestContainsCustomDate(startDateFromParam, endDateFromParam);

    if(requestContainsCustomDate || sessionContainsCustomDate(session)) {
        if(requestContainsCustomDate) {
            if(!isDateRangeValid(startDateFromParam, endDateFromParam)) {
                isDateRangeValid = false;
                startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
                endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
            }
        } else {
            startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
            endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
        }
        setSessionAttributes(startDateFromParam, endDateFromParam, session);
    }
    return isDateRangeValid;
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ While you have 3 answers, I would like to suggest that you read How do I ask a good question? so that you can get some points on this site. The title of the question should indicate what the code does rather than your concerns about the code. \$\endgroup\$
    – pacmaninbw
    Commented Feb 10, 2021 at 16:48
  • 1
    \$\begingroup\$ I get the feeling that your workplace is trying to fight cyclomatic complexity by banning else statements but not realizing that this leads to incredible increase in cognitive complexity. You could read into these and try to educate your coworkers. There are static code analysis tools that can be used for measuring these within the build process and you should use them instead of making blanket bans on normal programming structures. en.wikipedia.org/wiki/Cyclomatic_complexity en.wikipedia.org/wiki/Cognitive_complexity \$\endgroup\$ Commented Feb 11, 2021 at 7:28
  • 1
    \$\begingroup\$ And in case some of your coworkers read this: banning else statements is not normal. If that came up in a job interview I would prefer to choose another place to work. \$\endgroup\$ Commented Feb 11, 2021 at 7:34

3 Answers 3

4
\$\begingroup\$

"Else is evil"? That's a very peculiar point of view!

But if you really want to avoid it, you can look at early return, provided that isn't also regarded as evil - some people are very firm on the "single entry, single exit" principle.

Gilbert Le Blanc's code takes a similar approach to me, but in avoiding early return, he risks duplicated code execution, which may not be a problem here, but worries me a little.

The code below is not checked in any detail, but shows the idea, though it does duplicate code.

public boolean validateAndSetDateRelatedSessionAttributes(String startDateFromParam, String endDateFromParam, HttpSession session) throws ParseException {

  if (requestContainsCustomDate(startDateFromParam, endDateFromParam)) {
    boolean isDateRangeValid = isDateRangeValid(startDateFromParam, endDateFromParam);
    if (!isDateRangeValid) {
      startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
      endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
    }
    setSessionAttributes(startDateFromParam, endDateFromParam, session);
    return isDateRangeValid;
  }

  if (sessionContainsCustomDate(session)) {
    startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
    endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
    setSessionAttributes(startDateFromParam, endDateFromParam, session);
  }

  return true;
}
\$\endgroup\$
2
  • \$\begingroup\$ I'm not sure I like the method in general. Its functionality seems more than a little messy, but without any background I'm not sure what can be done with it. \$\endgroup\$ Commented Feb 10, 2021 at 16:09
  • \$\begingroup\$ Agreed with your comment, the above is bad code because it uses output parameters; it is a chunk of code extracted from a method that is around 1000 lines long and I can't do much about it as it would require me to refactor the entire method. \$\endgroup\$ Commented Feb 10, 2021 at 17:00
2
\$\begingroup\$

If I read correctly, the flow is:

if(A || B)
{
 if(A)
 {
   if(C)
   {
     ...
   }
 }
 else
 {
   ...
 }
}

Which is equivalent to:

if(A)
{
 b = true
 if(C)
 {
   ...
   b = false
 }
 ...
 return b;
}
if(B)
{
 ...
}
return true

So, in essence getting rid of the else can be done like this:

public boolean validateAndSetDateRelatedSessionAttributes(String startDateFromParam, String endDateFromParam, HttpSession session) throws ParseException {
    final boolean requestContainsCustomDate = requestContainsCustomDate(startDateFromParam, endDateFromParam);

    if(requestContainsCustomDate) {
        boolean isDateRangeValidResult = true;

        if(!isDateRangeValid(startDateFromParam, endDateFromParam)) {
            isDateRangeValidResult = false;
            startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
            endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
        }

        setSessionAttributes(startDateFromParam, endDateFromParam, session);

        return isDateRangeValidResult;
    }

    if(sessionContainsCustomDate(session))
    {
        startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
        endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
        
        setSessionAttributes(startDateFromParam, endDateFromParam, session);
    }

    return true;
}

If I didn't had to follow some company view about the readability of a statement, I would write instead:

public boolean validateAndSetDateRelatedSessionAttributes(String startDateFromParam, String endDateFromParam, HttpSession session) throws ParseException {
    boolean isDateRangeValidResult = true;
    final boolean requestContainsCustomDate = requestContainsCustomDate(startDateFromParam, endDateFromParam);

    if(requestContainsCustomDate) {
        if(!isDateRangeValid(startDateFromParam, endDateFromParam)) {
            isDateRangeValidResult = false;
            startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
            endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
        }
        setSessionAttributes(startDateFromParam, endDateFromParam, session);    
    }
    else if(sessionContainsCustomDate(session))
    {
        startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
        endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
        setSessionAttributes(startDateFromParam, endDateFromParam, session);    
    }

    return isDateRangeValidResult;
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Your second example calls setSessionAttributes under conditions where the original would not have done. This may or may not be acceptable. \$\endgroup\$ Commented Feb 10, 2021 at 16:12
  • \$\begingroup\$ You are right. I'm fixing it. Thanks. \$\endgroup\$
    – Hurricane
    Commented Feb 10, 2021 at 16:15
2
\$\begingroup\$

TLDR, the code could be refactored as below. There are several things that can be improved, so please have a look at my comments at the bottom.

    public boolean validateAndSetDateRelatedSessionAttributes(String startDate, String endDate, HttpSession session) throws ParseException {
        final boolean isCustomDateIncludedInRequest = isCustomDateIncludedInRequest(startDate, endDate);

        boolean isCustomDateIncludedInRequestOrSession = isCustomDateIncludedInRequest || isCustomDateIncludedInSession(session);
        if (!isCustomDateIncludedInRequestOrSession) {
            return true;
        }

        boolean isDateRangeValid = isDateRangeValid(startDate, endDate);
        boolean shouldDefaultDatesBeAddedToTheSession = !isCustomDateIncludedInRequest || !isDateRangeValid;
        if (shouldDefaultDatesBeAddedToTheSession) {
            startDate = getDefaultStartDate(session);
            endDate = getDefaultEndDate(session);
        }

        setSessionAttributes(startDate, endDate, session);
        return isCustomDateIncludedInRequest ? isDateRangeValid : true;
    }

    private String getDefaultEndDate(HttpSession session) {
        return session.getAttribute(BACalendarConstant.END_DATE).toString();
    }

    private String getDefaultStartDate(HttpSession session) {
        return session.getAttribute(BACalendarConstant.START_DATE).toString();
    }

Step by step refactoring and the reasoning behind each change

This is the original code:

public boolean validateAndSetDateRelatedSessionAttributes(String startDateFromParam, String endDateFromParam, HttpSession session) throws ParseException {
    boolean isDateRangeValid = true;
    final boolean requestContainsCustomDate = requestContainsCustomDate(startDateFromParam, endDateFromParam);

    if(requestContainsCustomDate || sessionContainsCustomDate(session)) {
        if(requestContainsCustomDate) {
            if(!isDateRangeValid(startDateFromParam, endDateFromParam)) {
                isDateRangeValid = false;
                startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
                endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
            }
        } else {
            startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
            endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
        }
        setSessionAttributes(startDateFromParam, endDateFromParam, session);
    }
    return isDateRangeValid;
}

The code is indeed quite difficult to read, with code duplication and nested ifs. It is not about that single else that you are referring to, but there is a range of things that can be done to make the code cleaner and concise.

When the whole body of the function is executed when a condition is true, which seems to be the case here, you can return early. Please have a look at Replacing Nested Conditional with Guard Clauses refactoring technique for more information.

But before, let us first simplify the condition of the first if, using the Extract Conditional refactoring technique as below:

public boolean validateAndSetDateRelatedSessionAttributes(String startDateFromParam, String endDateFromParam, HttpSession session) throws ParseException {
    boolean isDateRangeValid = true;
    final boolean requestContainsCustomDate = requestContainsCustomDate(startDateFromParam, endDateFromParam);

    boolean isCustomDateIncludedInRequestOrSession = requestContainsCustomDate || sessionContainsCustomDate(session);
    if(isCustomDateIncludedInRequestOrSession) {
        if(requestContainsCustomDate) {
            if(!isDateRangeValid(startDateFromParam, endDateFromParam)) {
                isDateRangeValid = false;
                startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
                endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
            }
        } else {
            startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
            endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
        }
        setSessionAttributes(startDateFromParam, endDateFromParam, session);
    }
    return isDateRangeValid;
}

Following that, we put the guard clause in place. We automatically remove one level of the if:

public boolean validateAndSetDateRelatedSessionAttributes(String startDateFromParam, String endDateFromParam, HttpSession session) throws ParseException {
    boolean isDateRangeValid = true;
    final boolean requestContainsCustomDate = requestContainsCustomDate(startDateFromParam, endDateFromParam);

    boolean isCustomDateIncludedInRequestOrSession = requestContainsCustomDate || sessionContainsCustomDate(session);
    if (!isCustomDateIncludedInRequestOrSession) {
        return true;
    }
    if (requestContainsCustomDate) {
        if (!isDateRangeValid(startDateFromParam, endDateFromParam)) {
            isDateRangeValid = false;
            startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
            endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
        }
    } else {
        startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
        endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
    }
    setSessionAttributes(startDateFromParam, endDateFromParam, session);
    return isDateRangeValid;
}

I can then see that you have a boolean variable called isDateRangeValid which is set to true in the beginning of the method. Then, in the inner if you do the following:

if (!isDateRangeValid(startDateFromParam, endDateFromParam)) {
    isDateRangeValid = false;
    ///...
}

That can be replaced as below. I have also moved the isDateRangeValid closer to where it is used.

boolean isDateRangeValid = true;
if (requestContainsCustomDate) {
    isDateRangeValid = isDateRangeValid(startDateFromParam, endDateFromParam);
    if (!isDateRangeValid) {
        startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
        endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
    }
} 
//...

If we replace the value of isDateRangeValid with the actual call, it becomes as below:

//...
boolean isDateRangeValid = isDateRangeValid(startDateFromParam, endDateFromParam);
if (requestContainsCustomDate) {
    if (!isDateRangeValid) {
        startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
        endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
    }
} else {
    startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
    endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
}

setSessionAttributes(startDateFromParam, endDateFromParam, session);
return requestContainsCustomDate ? isDateRangeValid : true; // The change has been reflected here as well.

Here, it is easier to see the code duplication! The following part is repeated twice!

startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();

Thus, we can remove the duplication (and the else) by enhancing the conditional as below:

public boolean validateAndSetDateRelatedSessionAttributes(String startDateFromParam, String endDateFromParam, HttpSession session) throws ParseException {
    final boolean requestContainsCustomDate = requestContainsCustomDate(startDateFromParam, endDateFromParam);

    boolean isCustomDateIncludedInRequestOrSession = requestContainsCustomDate || sessionContainsCustomDate(session);
    if (!isCustomDateIncludedInRequestOrSession) {
        return true;
    }

    boolean isDateRangeValid = isDateRangeValid(startDateFromParam, endDateFromParam);
    if (!requestContainsCustomDate || (requestContainsCustomDate && !isDateRangeValid)) {
        startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
        endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
    }

    setSessionAttributes(startDateFromParam, endDateFromParam, session);
    return requestContainsCustomDate ? isDateRangeValid : true;
}

Bonus section:

Based on Propositional Logic, the expression in the if can be simplified even more.

This statement (i):

!requestContainsCustomDate || (requestContainsCustomDate && !isDateRangeValid) i.e. !p || (p && !q)

And this statement (ii):

!requestContainsCustomDate || !isDateRangeValid i.e. !p || !q

are exactly the same!

You can verify this argument by testing the two expressions in the Standford University truth table tool.

Cool bonus statement: The results are exactly the same with this expression (iii) as well !(requestContainsCustomDate && isDateRangeValid) (De Morgan's Laws).

I am going to use expression (ii):

!requestContainsCustomDate || !isDateRangeValid

Which is extremely clear since it translates as below! IF the user did not request a custom date OR of the dates requested are not valid, use the default dates.

So, back to code. We make the replacement as below:

//...

boolean isDateRangeValid = isDateRangeValid(startDateFromParam, endDateFromParam);
if (!requestContainsCustomDate || !isDateRangeValid) {
    startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
    endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
}
//...

And we can make our intentions more clear by extracting the condition into a variable called shouldDefaultDatesBeAddedToSession:

//...
boolean shouldDefaultDatesBeAddedToSession = !requestContainsCustomDate || !isDateRangeValid;
if (shouldDefaultDatesBeAddedToSession) {
    startDateFromParam = session.getAttribute(BACalendarConstant.START_DATE).toString();
    endDateFromParam = session.getAttribute(BACalendarConstant.END_DATE).toString();
}
//...

We are missing the surrounding methods (e.g. setSessionAttributes etc). I am sure that we could also make it even cleaner, since there are still code smells in the code.

Further improvements / concerns:

  1. Single Responsibility Principle: This is my most important concern. This method does many things! A method should be responsible for one thing only (Single Responsibility Principle) and should do that very well. Please break it down to do less stuff! This brings us to the:

  2. Command-Query Separation. Strive to separate the methods that DO something and have side effects from the methods that RETURN something. Also, please have a look at the Pure Functions.

  3. Stick to naming conventions for better clarity. Avoid naming your variables like requestContainsCustomDate. Please stick to the related Java naming conventions. An alternative naming could be isCustomDatePresent. Boolean methods should start with is or if it makes more sense with has, can, or should.

  4. OOP! I see a lot of methods in the code, that take objects as parameters, to perform actions on those objects. For example, if possible, add the setSessionAttributes to the session object (or a wrapper/decorator of it), rather than passing the session in a method AND changing that session. A method from your method that does this is setSessionAttributes(startDateFromParam, endDateFromParam, session);

  5. Avoid noise when naming stuff. For example, in thestartDateFromParam and endDateFromParam function arguments, the FromParam is noise. You can change the variables to startDate and endDate.

  6. Make your intentions clear / Name your intentions. One could argue otherwise, but you could extract strange or large statements to their own variables, so that you can make your intention clear. An example is the below:

String defaultStartDate = session.getAttribute(BACalendarConstant.START_DATE).toString();

  1. BUG? What is the intention of the return value? Do you want to return true if the dates requested have been successfully set? The function does not do that, so I have a suspicion that the return values could be wrong.

The method becomes:

public boolean validateAndSetDateRelatedSessionAttributes(String startDate, String endDate, HttpSession session) throws ParseException {
    final boolean isCustomDateIncludedInRequest = isCustomDateIncludedInRequest(startDate, endDate);

    boolean isCustomDateIncludedInRequestOrSession = isCustomDateIncludedInRequest || isCustomDateIncludedInSession(session);
    if (!isCustomDateIncludedInRequestOrSession) {
        return true;
    }

    boolean isDateRangeValid = isDateRangeValid(startDate, endDate);
    boolean shouldDefaultDatesBeAddedToTheSession = !isCustomDateIncludedInRequest || !isDateRangeValid;
    if (shouldDefaultDatesBeAddedToTheSession) {
        startDate = getDefaultStartDate(session);
        endDate = getDefaultEndDate(session);
    }

    setSessionAttributes(startDate, endDate, session);
    return isCustomDateIncludedInRequest ? isDateRangeValid : true;
}

private String getDefaultEndDate(HttpSession session) {
    return session.getAttribute(BACalendarConstant.END_DATE).toString();
}

private String getDefaultStartDate(HttpSession session) {
    return session.getAttribute(BACalendarConstant.START_DATE).toString();
}
```
\$\endgroup\$

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