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.
