HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Extracting the location for a page with fewer conditionals

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
theconditionalswithfewerforextractingpagelocation

Problem

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).

Solution

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);

Code Snippets

if (aPage.getSiteDetail() != null && 
    (aPage.getLocationDescription() == null || aPage.getLocationDescription().getLocationLevel() == null))
{
    return aPage.getSiteDetail();
}
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;
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";
}
if (locationDescription == null) 
{
    return siteDetails;
}
if (locationLevel != null)
{ 
    return siteDetails.trim() + "-" + locationLevel;
}
return locationDescription == null ? siteDetails : siteDetails.trim() + "-" + location.level;

Context

StackExchange Code Review Q#71639, answer score: 2

Revisions (0)

No revisions yet.