It’s time for a quick exercise in code smells!
How many code smells do you see below?
override func viewDidLoad() {
super.viewDidLoad()
if viewModel.isAlternateStyle {
// Alternate style needs different placeholder text.
textField1.placeholder = strings.field1Hint
textField2.placeholder = strings.field2Hint
setUpAlternateView()
} else {
textField1.placeholder = strings.field1Placeholder
textField2.placeholder = strings.field2Placeholder
setUpNormalView()
}
}(This example is much shorter than the original. And I anonymized the field names, so don’t be thrown off by the unhelpful names textField1 and field1Hint.)
Follow your nose, share your observations
I’ll say more in follow-up posts. First, please share your observations in the comments below. Don’t read the other comments until you’ve come up with your own list, though!
How many code smells do you see in this short method? #SwiftLang
All Articles in this series
I see 3 right off…
1: The placeholder strings should come from the view model.
2: There should be one setupView(). Inside there you should set the placeholder text.
3: The view controller shouldn’t care about viewModel.isAlternateStyle. The view model should already know we are in the alternate style and give the alternate placeholder strings…
I would use an enum to model the style state.
I would put the most-common case first.
I would not split setup logic between viewDidLoad() and setUp*View.
I would use more-descriptive variable names.
On an unrelated note, I respect your consistent, long-lived brand as the iOS-testing dude.
The View Controller should just ask the View Model for the placeholder text, rather than having a conditional statement. That conditional belongs in the View Model.
Is it just me, or is it difficult to read the code snippet with the light color text and the shadows? Can you make that darker and clearer to read please? That would help a lot, thank you! :)
Vui, could you share a screenshot with me? I’m wondering if there’s a CSS issue, because the code looks super-clear to me.
Thank you, Jon, for your quick reply and for making the styling changes on the website. It is much easier to read the code now. :)
First, though this code has smells, none of them are appalling or urgent. If this were the only problem method in the class, it could be tolerated.
The first smell is the comment; if part of the code needs explanation, it can be explained by making it a method.
The second smell is conditional behavior based on the boolean flag isAlternateStyle. This sort of condition (and related case statements) should almost always be replaced with polymorphism or with a strategy object. Moreover, a significant else{} clause is, if not a code smell, at least showing signs of being overripe.
The third smell is that viewModel.isAlternateStyle is a naked boolean; if we ever need a third style, things will get even uglier. It can be the policy object, and that object can know how to set up the view. (It’s tempting to settle for an enum, but in real life code there are bound to be more chores that we can shuttle off to the viewStyle strategy object.)
Fourth, why does the viewModel know about the view style? This likely belongs to the view or its controller, not the model. Moving it will avoid having the viewModel know about the viewStyle and its related policy objects, which is bound to be indecent exposure.
A fifth smell is that we’re setting the placeholder here, when clearly that belongs in the setUpNormalView() and setUpStrategyView() methods — each of which will move to the strategy object.
The strings object’s placeholders for alternative views are misnamed; the alternate placeholder for field 1 should be field1AlternatePlaceholder, not field1Hint. I don’t think the view should define these strings; that belongs in the viewController (or, conceivably, the model), or refactored to the new viewStyle strategy object. In the latter case, both cases share the same code, making the strategy object even simpler.
#4. composition root should assign the placeholder text isn’t it?
ie. composition root will assign the value based on the view type, it is better to define the placeholder text in VC rather than in VM?
1. The if-else could grow infinitely
2. The variables are named horribly with a digit suffix. What is what?
Sorry about the identifier names, folks. I anonymized them, but neglected to mention that before. So, good job everyone who said the names need improvement. ?
Please, change color of font in code examples. It is difficult to read.
The background color isn’t coming through for everyone. I’m looking into it. In the meantime, you can read the AMP version: https://qualitycoding.org/code-smells-exercise/amp/
Thank you
Okay, I changed the code theme. It should now have a tan background — but even if the background fails, at least the code will be dark-against-light. I hope this helps.
I viewed this page on MacOS Chrome and Safari and both display your code as a single long line of text, which is hugely annoying.
Thanks for letting me know. I’ve been chasing this problem, and finally tracked down the plugin that was breaking newlines. Until they provide a fix, I’ll keep it disabled — which should keep the code legible.
No tests.
It seems for me, that there are 5 code smells in this code:
1) Using if statement. It’s better to move this logic into view model.
2) ViewModel (it looks like MVVM pattern) should not know about view style.
3) There are bad names for text fields: textField1 and textField2.
4) There are bad names for placeholders: field1Placeholder and field2Placeholder.
5) It seems strange, that placeholders have different names: field1Hint and field1Placeholder.
The setting of the textfield can be moved to setupAlternateView and SetupNormalView
While that is a possibility, I think that might result in methods that do too many things. Still, that is the kind of thing that would be worth a quick experiment, without going through refactoring steps. In other words, hack it together and see. Or even, sketch out the responsibilities on a whiteboard. Are they similar enough to belong together? Or would bringing them together entangle unrelated things?
1 – viewModel should provide all value for placeholders
2 – if else disappears all this logic goes inside viewModel
3 – The same for setupViews methods disappears
4 – Function`s name wrong written
5 – ‘strings’ object name does not represent his commitment
Nice, Carlos. Check out the rest of this series to see if there are any smells you missed.