Code Smells: How Many Do You See in This Method?

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

Follow-up posts:

About the Author Jon Reid

When I was a kid, programming was fun. But working in Silicon Valley, I saw poor code lead to fear, with real human costs. Searching for ways to make life better, I learned about Design Patterns, Refactoring, and Test Driven Development (TDD). Programming became fun again! I've now been doing TDD in Apple environments for 17 years. I'm committed to software crafting as a discipline, with the hope of raising us all to greater effectiveness and joy.

follow me on:
  • Sam Warfield says:

    I see 3 right off…

    • Sam Warfield says:

      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…

  • Josh Adams says:

    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.

  • Josh Brown says:

    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.

  • Vui Nguyen says:

    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.

  • Paolo says:

    1. The if-else could grow infinitely
    2. The variables are named horribly with a digit suffix. What is what?

  • Jon Reid says:

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

  • TurboGrandpa says:

    Please, change color of font in code examples. It is difficult to read.

  • Alexander says:

    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.

  • >