en
de

Can you find the 5 smells in “I reviewed your code last night and deleted all of it since it’s crap”

24 September 2015
| |

Ok…
Let’s make this a short one.

I talked to lots of people about this statement and after lots of strange looks I got lots of comments about the behaviour. Especially about the “since it’s crap”. This is pretty obvious a very strong opinion about something.

But hey! Maybe the code works, passed acceptance quality gates, the client user interface looks pretty good that uses this code piece and it is deployed in production. And maybe the company is making loads of money $$$ with it.

Making lots of money.. Is my code still crap??

Ok… Let me slow down.
How did this came about?
The statement happened during the Daily Scrum after one of the devs finished talking about what he did. Another developer jumped in and finished this sentence with:

“I reviewed your code last night and deleted all of it since it’s crap”

How many smells can you find in that statement and behavior?
I found 5.

1. “I reviewed”
What? 1 single person is the code gatekeeper and making sure that code is “right”??

2. “Your Code”
Is this your code, my code and every developer has their code area?
Can we talk about “Collective Code ownership” and the advantages about it?
See Martin Fowler on the topic “Collective Code Ownership”

3. “Last night”
Someone is working late nights?
Why is that? Is he not co-located or working different work hours?
How is that affecting team work?
Is this an incarnation of the “Hotel Room Rule”? The “Hotel Room Rule” as described by me on my private blog.

4. “Deleted”
A code review where someone just deletes code?
What about giving feedback in a constructive way?
Can you provide feedback and avoid this in the future?
If you are a team, focus on how we can improve together. How can we guide ourselves in the direction of Clean Code. Why CleanCode in the 1st place?
More links to giving feedback in my talk about “Agile and Software Craftmanship”

5. “It’s crap”
What is crap about it?
The style?
The complexity?
The overall class design?
The infrastructure usage? What is it?
Is this your opinion? Can we do a team code review about this?

Some tips to avoid this situation:

  • Schedule regular team code reviews (we call this DevExchanges and do that every 30 minutes after lunch).
    These sessions help to share knowledge and “how do we do things here” (every team is different). Did you ever have a discussion about “tabs versus whitespaces” or “member variables with a leading underscore _”?
  • Use Merge Pull Requests and the comment feature to discuss code issues
    Github, Gitlab or others provide great tooling around reviewing and discussing code
  • Provide lots of context in commit statements
    Context is important to know what problem are we trying to solve with the specific code. Is this a refactoring? Fix a bug? Why? Why? Why?
  • Give feedback by asking questions. Instead of “this is crap” say “Can we improve this?”
    It’s very hard to ask a question and be negative in it… Not impossible though
  • Use NVC to communicate, “I see”, I observe”, “I realize” are great statements to start with.
    “I see these lines of code are crap.” is not a good one😉 Non Violent Communication on Wikipedia
  • Try mobprogramming or pair programming
    http://mobprogramming.org/

BTW: we made quite good experiences with remote people as well when we did Dev Exchanges distributed as described by Emilija on Distributed Development.

Important to remember. Grow as a team and

“The doing is more important than the result.”

Question to you:

What do you do if you browse the codebase and you see something smelly?

Put it on a list? Mark the code? Add a TODO? Send an email? Approach someone? Wait for the retrospective? Call the lead dev?

Comments (5)

Denniz

25 September 2015 at 20:44

>> Making lots of money.. Is my code still crap?

I think this statement adds an interesting perspective.
If we make a lot of money with that code and we never need to change it, why bother?
Code coverage is another important aspect of code quality to talk about

Thanks anyway

    Bachi

    1 October 2015 at 15:39

    >> Making lots of money.. Is my code still crap?

    > I think this statement adds an interesting perspective.
    > If we make a lot of money with that code and we never need to change it, why bother?

    Because doing nothing will give opportunities to eager and new players. Think Nokia.

      Peter Gfader

      1 October 2015 at 16:18

      I agree that in the long term “changeability” (or better “adaptability”) plays a big role for all features.

        Dom

        2 October 2015 at 18:18

        “If It Ain’t Broke, Don’t Fix It”

        If code works, has been stable for a longer period of time, and there is no reason to change it in the near future (i.e., there is no upcoming change affecting the code), why risking to break something?

        See also corresponding reengineering pattern in http://scg.unibe.ch/download/oorp/OORP.pdf

        Of course, some people might argue that reengineering legacy code and working on a newly developed system are not the same tasks. Then again, when does code become legacy? When it’s deployed in production? When the developer left?

          Peter Gfader

          5 October 2015 at 09:19

          Thanks Dom for the link to “Object-Oriented Reengineering Patterns”.
          Didn’t know that book yet!

×

Sign up for our Updates

Sign up now for our biweekly updates.

This field is required
This field is required
This field is required

I'm interested in:

Select at least one category
You were signed up successfully.