Refactoring #2

In this post let us look at a piece of code that was written by senior devs trying to consolidate the code. Since I cannot post the code as it is, lets look at portions of the code.

The Codebase

Say the codebase is common and shared between variants of the app that can be compiled as white labeled products, so there is an abundant sprinkling of # conditional compiler surrounding the code. Say we are making an app that will have three variants, Students, Teachers and Parents, so the #conditionals look something like

#if Students
  var name = "Students"
#elseif Teachers
  var name = "Teachers"
#else // Parents
  var name = "Parents"
#endif

Now this is fine but if this is not readable and clean, instead personally I would like to have an enum that manages these as

enum TargetAudience {
    case students
    case teachers
    case parents
}

Not as simple

The problem looks quite simple as defined above, however the problem in the codebase is not as simple as simply defining a variable, it is about displaying the terms and conditions data from a file where the filename is a combination of a couple of things, like the language, the country and the target audience.

1. The outer wrapper

so the original code looked something like

#if !WATCH_TARGET
 // Lots of code here
#else
 return "This is when run for the Watch"
#endif

In this block of code, the biggest issue is that the developer used a ! condition and the else was simply a single simple line to return, so in terms of readability and good practice, I would have written it as

#if WATCH_TARGET
    return "This is ..."
#else
  // Lots of code here
#endif

This way I can focus on the bulk code without having to bother about stray runaway code

2. The Inner targets – Part A

The lots of code here block then looked something like

if notShownTandC() == true {
    let tcFilename: String?
    if let currentTarget = target as? hasTermsAndConditions {
        tcFilename = currentTarget.tcKey
    } else {
        tcFilename = nil
    }
    if let tandcFilename = tcFilename {
        // More conditional code here
    }
    return "standard terms and conditions"
} else {
    return "Terms and conditions are already shown here..."
}

The problem with this block itself is is a bit too complicated with too many scopes, this bit itself can be simplified and be re-written as

if notShownTandC() == false {
    return "Terms and conditions are already shown here..."
}

if let currentTarget = target as? hasTermsAndConditions {
    let tcFilename = currentTarget.tcKey
    // More conditional code here
}
return "standard terms and conditions"

In the same way as earlier, I would rather move the easy portion out of the way so first if the t&c are shown, let’s just return from the code

Next, if we can get terms and conditions from the target and the target has a tcKey, then we perform the more conditional code. If there is a possibility of this tcKey being nil, then I would include that in the conditional line as well,

if let currentTarget = target as? hasTermsAndConditions,
    let tcFilename = currentTarget.tcKey {
    // More conditional code here
}

3. The Conditional Code

The fun part of this was in this conditional code block that included more if’s and #if’s

#if Parent
 var tcResourceFilename = "TC_Parent"
 if getUserRegion == "Europe" {
   if getUserLanguage() == "English" {
       tcResourceFilename = "TC_Parent_Europe"
   } else {
       tcResourceFilename = "TC_Parent_Europe_French"
   }
 } else if getUserRegion() == "Asia" {
    if getUserLangauge() == "English" {
        tcResourceFilename = "TC_Parent_Asia"
    } else {
        tcResourceFilename = "TC_Parent_Asia_Mandarin"
    }
 }
#else
 var tcResourceFilename = "TC_Student"
 if getUserRegion == "Europe" {
   if getUserLanguage() == "English" {
       tcResourceFilename = "TC_Student_Europe"
   } else {
       tcResourceFilename = "TC_Student_Europe_French"
   }
 } else if getUserRegion() == "Asia" {
    if getUserLangauge() == "English" {
        tcResourceFilename = "TC_Student_Asia"
    } else {
        tcResourceFilename = "TC_Student_Asia_Mandarin"
    }
 }
#endif

For this example, I have tried to make it simpler and keep it to a minimum, however it can be seen how redundant and unnecessary code was written and this is simply taking into account that we are not managing all the languages in the regions and int he example we are only considering two regions. On top of that we are not writing the conditional for Teachers.

The moment we add more complexities, the code can seem to explode exponentially and become unmanageable. This is how fragile the code written for some large codebases is.

What would be the solution for this?

The simplest solution that makes sense is using a consistent pattern to display the terms and conditions. The pattern would be as simple as naming the filenames consistently as

TC_<Audience>_<Region>_<Langauge> and then this can be easily constructed as

let audience = getAppAudience()   // returns Teacher, Student or Parent
let region = getUserRegion()      // returns Asia, Europe, Americas, etc
let langauge = getUserLangauge()  // returns English, French, Spanish, Mandarin, etc

let tcFileName = "TC_\(audience)_\(region)_\(language)"

and that is the easiest way to get the filename constructed instead of all that crappy redundant code.

More additions

In addition to the code above, we can also use enums to make the code better and readable with the audience being enums as in the code above, and the regions and languages also being enums themselves.

If for some reason there arises a situation to do some complex code juggling to get the filename then the best way is to use swift switch cases as

var filename: String 
switch (region, language) {
    case (.europe, .french):
    case (.europe, .spanish):
    case (.europe, .italian):
    case (.europe, .german):
        return "Europe_EU"
    case (.europe, .english)
        return "Europe_English"
    case (.asia, .english):
        return "Asia_English"
    case (.asia, _):
        return "Asia_Other"
    default:
        return "Worldwide"
}

Though this would also benefit from the pattern matching for files, there can be other ways to manage this given the complexities. However it is readable in comparison to the nested if conditions. At least in my book.

This is by no means a jab at any developers, it is about the distinct code smells that come from Java or C# as it is the way to code with those languages but makes for a lot of unreadable and unmaintainable code. This is not the best refactor but it is a great improvement on the original block of code that was written.

Views All Time
Views All Time
196
Views Today
Views Today
1
Posted in Article, Basics, Code, Tip.