Five code smells in the making

If you have read Refactoring, and if you have not you should definitely stop reading this blog and grab a copy immediately, you should be familiar with the term code smell.

A code smell is something in the code, apparently not very important, but that is a symptom of an underlying significant problem.

Some code smells have been catalogued, but there are some others, way more subtle, that are more difficult to spot. Following is a list of some of those proto-smells, that I have catalogued in my own practice. In my experience, every time that I notice any of these, I end up needing to rethink/refactor significant parts of my code.

Please take all of them with a grain of salt, more as generic pseudo-guidelines than as strict rules. What might indicate a bad design at one given moment and in one given situation, might just be strictly necessary, and even good, in another.

Too many imports

This one is from the (good) old days, when we didn’t have Swift, and we had to import a class dependencies manually. Like animals.

If a class implementation starts with a long list of imports, there is something wrong somewhere.

Either that class has too many responsibilities and therefore is doing too much, breaking the Single Responsibility Principle, or it truly needs a lot of context, which might indicate that it is not the right abstraction or it is not collaborating with the right abstractions. In any case, this is usually a prelude to a big refactor.

This one is particularly evident in the case of an AppDelegate; if it to import needs too many classes, it is probably a good idea to introduce some abstraction.

Private enumerations or instance variables of type enumeration

If the previous smell was only applicable to Objective-C code, this one is mostly applicable to Swift.

Private enumerations usually hide either state or lack of abstraction.

Complicated logic.

Nested if-clauses in particular make code difficult to understand, and therefore difficult to reason about.

Complex if-clauses are a symptom of lack of abstraction

Here, When I say lack of abstraction, I mean “lack of the most basic abstraction. For example, this code:

Swift

1

2

3

4

5

ifcounter==0&&retries<2&&today.day==1&&totalAmmount<100{

totalAmmout=totalAmmount+20

}else{

totalAmmount=totalAmmount*0.8

}

is impossible to understand to:

Anyone other than the original author.

The original author, one month after writing the code.

Every time I see code like that, I start to refactor complexity out by decomposing the different parts of the conditional into separate methods. Sometimes that will be enough, some other times, that would show me that there is an abstraction I can extract even further.

Swift

1

2

3

4

5

ifcardCanBeCharged(){

chargeCard(20)

}else{

applyDiscount(20)

}

Properties referencing gesture recognisers.

I haven’t found any situation yet where holding a reference to a gesture recogniser in a view controller is not a symptom of a major issue.

Holding references to gesture recognisers indicates improper management of behaviour in the UI

A gesture recogniser should be provided a selector, and then should be attached to a view. Period. If I need to hold a reference to it, that might indicate either temporal coupling (I am building the UI in sequence, and I need that sequence to be executed in a particular order) or it might indicate that I am dealing with state in the UI. None of those scenarios are particularly good.

Properties in view controllers referencing transient views.

This happens almost exclusively in view controllers.

Properties pointing to transient views in a view controller indicate that the view controller has different states.

If I need to reference a view that might or might not be on screen, that most likely means that my view controller is dealign with state. And we have already discussed why that might not be a good idea.

These are five proto-smells that help me become aware of when I start drifting towards one of those tangled messes I tend to put myself into. I always keep a vigilant eye on them to avoid not being able to see the forest for the trees.