patternjavaMinor
Extracting the location for a page with fewer conditionals
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
I came across this particular method:
The method
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).
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
Using
Although this doesn't actually improve number of
Combining conditionals
You are returning
After this there's no point in checking if
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:
Pointless Conditionals
If
Ternary Operators
I already used these when assigning a value to
Becomes:
Could even become:
C# so the it's probably slightly different than you're used to:Using
else ifAlthough 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 reasonsCombining 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.