5
\$\begingroup\$

I've been given the job of trying to reduce the line count for a file that helps generate a PDF for a web app. The class itself is pdfHelper, and contains a number of methods used to generate a PDF such as getting the location, custom methods for formatting lowagie.text table cells etc.

I came across this particular method:

    public String getLocationToString(Page aPage) {
    if (aPage.getSiteDetail() != null || aPage.getLocationDescription() != null) {
        if (aPage.getSiteDetail() != null && aPage.getLocationDescription() == null) {
            return aPage.getSiteDetail();
        } else if (aPage.getSiteDetail() != null && aPage.getLocationDescription().getLocationLevel() == null) {
            return aPage.getSiteDetail();
        } else if (aPage.getSiteDetail() != null && aPage.getLocationDescription().getLocationLevel() != null) {
            return aPage.getSiteDetail().trim().toString() + "-" + aPage.getLocationDescription().getLocationLevel();
        } else {
            return "N/A";
        }
    } else {
        return "N/A";
    }
}

The method getSiteDetail() returns a String, getLocationDescription retrieves a LocationDescription object from the Page object. getLocationLevel() is a method for the LocationDEscription object and also returns a String.

Is there a way to shorten this method? I can't seem to figure out a way to reduce the amount of conditionals in this method (admittedly I'm not much of a programmer yet).

\$\endgroup\$
  • 3
    \$\begingroup\$ ... "reduce number of lines of code" is a terrible description of a task. It would be better to reduce the complexity of the code, but this is more difficult to measure (to be sure, number of lines does factor into this...). I'm worried about a couple of other things here - 1) .trim().toString() - at minimum toString() is unnecessary, as the previous operation returns a string. Should you also trim() for the other instances (or is it unneeded)? 2) it would probably be better to return null or an empty string, than "N/A". \$\endgroup\$ – Clockwork-Muse Dec 4 '14 at 11:11
  • \$\begingroup\$ It's been specc'd that N/A should be returned to indicate that location is not applicable for the particular type of pdf requested. The pdfs generated are reports and there is a large number of types so this is needed for the user...or so I've been told \$\endgroup\$ – jbailie1991 Dec 4 '14 at 11:49
  • \$\begingroup\$ Also, for other instances, trim() is unnecessary, it is needed for the one instance its called for presentation reasons \$\endgroup\$ – jbailie1991 Dec 4 '14 at 11:58
  • \$\begingroup\$ @Clockwork-Muse I agree that reducing the number of lines of code is not the key, reducing complexity is the key, good point. I assume that as long as it is in a single function, complexity is a function of the lines of code (not only, but pretty much). Splitting (aka "Extract 'till you drop") already reduces complexity. Complexity can actually be measured, i.e. McCabe's Cyclomatic Complexity. \$\endgroup\$ – Christian Hujer Dec 4 '14 at 18:14
2
\$\begingroup\$

In advance sorry for the formatting but I code in C# so the it's probably slightly different than you're used to:

Using else if

Although this doesn't actually improve number of if statements, why are you using else if? If the previous if statement is satisfied you're returning a value anyway, so the code won't execute the next if statement anyway. This also renders your else keyword pointless too because of the same reasons

Combining conditionals

You are returning aPage.getSiteDetail() in two places, you can combine these if statements together:

if (aPage.getSiteDetail() != null && 
    (aPage.getLocationDescription() == null || aPage.getLocationDescription().getLocationLevel() == null))
{
    return aPage.getSiteDetail();
}

After this there's no point in checking if aPage.getLocationDescription() == null || aPage.getLocationDescription().getLocationLevel() == null because if aPage.getLocationDescription() then aPage.getLocationDescription().getLocationLevel() has to be null:

Repeated method calls

You can really improve the readability of your code and minimise the number of external method calls your making by assigning the return value of your method calls to a variable:

string siteDetails = aPage.getSiteDetail();
// Here you need to replace var with whatever the return value of aPage.getLocationDescription() is
var locationDescription = aPage.getLocationDescription(); 
string locationLevel = locationDescription != null ? locationDescription.getLocationLevel() : null;

Pointless Conditionals

If aPage.getSiteDetail() != null || aPage.getLocationDescription() != null but none of the rest of your conditionals are satisfied you return "N/A" but you also do this if both are null anyway. So your first conditional is pointless, because you continuously evaluate those values later on making a final code looking something like this:

public String getLocationToString()
{
    string siteDetails = aPage.getSiteDetail();        
// Here you need to replace var with whatever the return value of aPage.getLocationDescription() is
    var locationDescription = aPage.getLocationDescription();
    string locationLevel = locationDescription != null ? locationDescription.getLocationLevel() : null;

    if (siteDetails != null))
    {
        if (locationDescription == null) 
        {
            return siteDetails;
        }
        if (locationLevel != null)
        { 
            return siteDetails.trim() + "-" + locationLevel;
        }
    }

    return "N/A";
}

Ternary Operators

I already used these when assigning a value to locationLevel but you could also do it for the following two if statements which means you'd only have one if in your whole function:

if (locationDescription == null) 
{
    return siteDetails;
}
if (locationLevel != null)
{ 
    return siteDetails.trim() + "-" + locationLevel;
}

Becomes:

return locationDescription == null ? siteDetails : siteDetails.trim() + "-" + location.level;

Could even become:

return siteDetails.trim() + (locationDescription == null ? "" : location.level);
\$\endgroup\$
  • \$\begingroup\$ @Heslacher good spot, in C# we have var I don't know what your equivalent is in Java (if any) \$\endgroup\$ – ediblecode Dec 4 '14 at 11:09
  • \$\begingroup\$ As far as I know there is nothing similiar in java. \$\endgroup\$ – Heslacher Dec 4 '14 at 11:10
  • \$\begingroup\$ @Heslacher Ah then whatever the return value of that statement is, he needs to set it to that \$\endgroup\$ – ediblecode Dec 4 '14 at 11:13
  • \$\begingroup\$ Yeap. I have put some notice in my answer about the missing context. \$\endgroup\$ – Heslacher Dec 4 '14 at 11:14
  • \$\begingroup\$ so the end result would be if(siteDetails != null){ return locationDescription == null ? siteDetails : siteDetails.trim() + "-" + locationLevel;} return "N/A"; ? If I've understood your answer correctly then fantastic work, thank you \$\endgroup\$ – jbailie1991 Dec 4 '14 at 12:02
2
\$\begingroup\$

For each if condition you are checking aPage.getSiteDetail() != null so let us use this as a guard condition.

if (aPage.getSiteDetail() == null) {
    return "N/A";
}

this will reduce your method to

public String getLocationToString(Page aPage) {
    if (aPage.getSiteDetail() == null) {
        return "N/A";
    }


    if (aPage.getLocationDescription() == null) {
        return aPage.getSiteDetail();
    } else if (aPage.getLocationDescription().getLocationLevel() == null) {
        return aPage.getSiteDetail();
    } else if (aPage.getLocationDescription().getLocationLevel() != null) {
        return aPage.getSiteDetail().trim().toString() + "-" + aPage.getLocationDescription().getLocationLevel();
    }
}  

Now as we don't need the else if because if the condition is true the method will return. After extracting the result of getSiteDetail() and rearanging the conditions we get

public String getLocationToString(Page aPage) {
    String siteDetail = aPage.getSiteDetail();
    if (siteDetail == null) {
        return "N/A";
    }

    LocationDescription locationDescription = aPage.getLocationDescription();
    if (locationDescription != null) {

        String locationLevel = locationDescription.getLocationLevel();
        if (locationLevel != null) {
            return siteDetail.trim() + "-" +  locationLevel;
        }
    }

    return siteDetail;
}  

We also have removed the not neccessary call to toString().

\$\endgroup\$
  • \$\begingroup\$ apologies, this is code created a while ago by someone else that I've been tasked with refactoring. Added more detail in the question \$\endgroup\$ – jbailie1991 Dec 4 '14 at 11:40
1
\$\begingroup\$

A good method to solve such issues is to come up with a decision matrix and then re-implement that decision matrix. Another good method to solve such issues is to use a good IDE like IntelliJ IDEA, it will offer many of the required simplifications as intentions.

Here's the decision matrix based on the if-else-chain. The three conditions checked are, along with abbreviations to make it easier to read with the limited space here:

  • aPage.getSiteDetail() != null -> hasSiteDetail
  • aPage.getLocationDescription() != null -> hasLocation
  • aPage.getLocationDescription().getLocationLevel() != null -> hasLevel

Now that we have identified all conditions, we will create the matrix input. Basically, there are 2^3 = 8 possible combinations. Not all make sense, i.e. hasLevel can be assumed false if hasLocation is false, I'm just telling the basic approach.

hasSiteDetail hasLocation hasLevel result
true          true        true     siteDetail.trim() + "-" + locationLevel;
true          true        false    siteDetail
true          false       true     IMPOSSIBLE
true          false       false    siteDetail
false         true        true     "N/A"
false         true        false    "N/A"
false         false       true     "N/A"
false         false       false    "N/A"

Wherever the decision matrix creates the same result for the same column in both cases, true and false, it means that the condition is irrelevant for the result.

We see that when !hasSiteDetail it returns "N/A" regardless of the other checks. We see that the result involving the locationLevel is only returned if the locationDescription is available and the locationDescription has a locationLevel. And otherwise, siteDetail is returned.

Based on that information we can come up with the following equivalent method, which is significantly simpler:

public String getLocationToString(final Page aPage) {
    final String siteDetail = aPage.getSiteDetail();
    if (siteDetail == null)
        return "N/A";
    final LocationDescription locationDescription = aPage.getLocationDescription();
    if (locationDescription != null) {
        final String locationLevel = locationDescription.getLocationLevel();
        if (locationLevel != null)
            return siteDetail.trim() + "-" + locationLevel;
    }
    return siteDetail;
}

Actually I got that result in a few steps with IntelliJ IDEA without involving the decision matrix. I'm just showing the decision matrix because that's the generic appraoch.

Disclaimer: This new method is only equivalent with the original if all methods that have names starting with get have no side-effects.

\$\endgroup\$
0
\$\begingroup\$

Try the method like this:

public String getLocationToString(Page aPage) {
    var siteDetail = aPage.getSiteDetail();
    var locationDescription = aPage.getLocationDescription();

    if (siteDetail != null && locationDescription == null) {
        return siteDetail;
    } else if(siteDetail != null && locationDescription != null){
        var locationLevel = locationDescription.getLocationLevel();

        if(locationLevel == null){
            return siteDetail;
        } else{
            return siteDetail.trim().toString() + "-" + locationDescription.getLocationLevel();
        }
    }

    return "N/A";    
}

You are calling the methods too many times in your function, do the calls as few times as possible.

\$\endgroup\$
  • 1
    \$\begingroup\$ Although this answer provides an alternative way of implementing what the OP wants, commenting on what things you do differently, why you are doing it that way and pointing out the flaws in the OP's original code would make this a better answer. Simply providing a different way to do things without explaining them does not always provide a good learning experience. \$\endgroup\$ – Simon Forsberg Dec 4 '14 at 12:59
  • \$\begingroup\$ @SimonAndréForsberg Thanks for your advice in how to provide a better answer, I will take it onboard for fuuture answers. The answer provided by danrhul is a much better answer than I could provide. \$\endgroup\$ – d347hm4n Dec 4 '14 at 13:03
0
\$\begingroup\$

Please find inline comments to guide you through my approach. Any input is invited.

    // 1. Operate on local variables rather than accessing it everytime through a getter.
    final String siteDetail = null;
    final String locationLevel = null;
    final String output = "N/A";// Default output.

    // 2. Populate the variable as per the required condition.     
    if(aPage.getSiteDetail()!=null){
        siteDetail = aPage.getSiteDetail().trim().toString();
    }

    if(aPage.getLocationDescription() != null && aPage.getLocationDescription().getLocationLevel() != null){
        locationLevel = aPage.getLocationDescription().getLocationLevel();
    }

   // 3. Keep it simple. Makes debugging easy.
    if(locationLevel !=null && siteDetail != null){
       output = siteDetail + "-" + locationLevel;
    }else if(siteDetail != null){
       output = siteDetail;
    }
    // Are you sure you dont require non-null locationLevel if siteDetail is null.
    return output;
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Although this answer provides an alternative way of implementing what the OP wants, commenting on what things you do differently, why you are doing it that way and pointing out the flaws in the OP's original code would make this a better answer. Simply providing a different way to do things without explaining them does not always provide a good learning experience. \$\endgroup\$ – Simon Forsberg Dec 4 '14 at 12:58
  • \$\begingroup\$ Appreciate the input :) \$\endgroup\$ – thepace Dec 5 '14 at 5:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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