How Many Code Smells Can You Find in This Short Method? 

 February 27, 2018

by Jon Reid


It’s time for a quick exercise in code smells!

How many code smells do you see below?

override func viewDidLoad() {
    if viewModel.isAlternateStyle {
        // Alternate style needs different placeholder text.
        textField1.placeholder = strings.field1Hint
        textField2.placeholder = strings.field2Hint
    } else {
        textField1.placeholder = strings.field1Placeholder
        textField2.placeholder = strings.field2Placeholder

(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

Click to Tweet

All Articles in this series

Improving a Short, Smelly Method

Jon Reid

About the author

Programming was fun when I was a kid. But working in Silicon Valley, I saw poor code lead to fear, with real human costs. Looking for ways to make my life better, I learned about Extreme Programming, including unit testing, test-driven development (TDD), and refactoring. Programming became fun again! I've now been doing TDD in Apple environments for 20 years. I'm committed to software crafting as a discipline, hoping we can all reach greater effectiveness and joy. Now a coach with Industrial Logic!

    • 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.

  • 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! :)

  • 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?

  • 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.

    • 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

  • {"email":"Email address invalid","url":"Website address invalid","required":"Required field missing"}