In engineering, it's normal that teams sometimes struggle to get stories/tasks/bugs done. We, too, were facing this issue, so we sat down with the tech leads and managers, and discussed the most common reasons for issues we've had in our development work.
The top 6 reasons we identified are (in no particular order):
- Refactoring code as part of another task.
- The story's scope is unclear.
- The story's scope is too wide.
- Implementation hasn't (really) been planned.
- Doing other "small" changes as part of another task.
- Arguing over opinions in PRs.
We obviously wanted to tackle these issues and decided to lay down a few ground rules for development work. Here are the rules we agreed on:
1. Never refactor code as a part of another task
"Why not? I can do it in this small PR while I'm doing this other feature!"
– Random Developer
Refactoring changes the internals of existing code. Suppose we find a new bug after deployment. It may be hard to identify if the faulty behavior is caused by the new feature or bug fix, or by the code that was refactored simultaneously.
So, when should you refactor code? Here's what we think:
Good reasons to refactor code
- The requested change can't be done on the existing code.
- The existing code is very difficult and/or risky to extend.
- Not refactoring it will definitely cause us problems.
- Refactoring will provide us with business value.
Bad reasons to refactor code:
- I don't like the code.
- The code is legacy.
- It's implementing a paradigm X, and I'd want it to use Y.
- I'd like to refactor.
- It could be written better.
- I don't understand the code.
How to progress to refactoring
We decided that refactoring must be decided together with the tech lead for a good reason and it must be a separate task. No more refactoring just because someone felt like it! Our job isn't to write perfect code but to bring value to our clients.
Requirements of refactoring
When refactoring, if the code isn't tested, you must write the tests for the to-be-refactored code before the refactoring. After refactoring, you must ensure the code gives the exact same results. The bugs and all. Really.
Some other code might rely on very specific behavior of the code you're refactoring, so you must refactor it to do also the not-so-obvious stuff. Changing the behavior of the code is another task — read on.
2. Know what you're doing, part 1
You must have a clear vision of the outcome of what you're going to be working on. If you don't really know the outcome of the story, ask for more information.
3. Know what you're doing, part 2
The story/task must be split into workable chunks (subtasks). One chunk should be one PR, and one PR should be a stand-alone thing. It can be a new helper, and the second PR will use that helper. Use feature branches as the target for your PRs related to a single issue if you need multiple PRs.
Remember, you can add as many subtasks as you want to the issue you're working!
4. Know what you're doing, part 3
Have an implementation plan before you implement, and don't only have inside your own head. Talk to other people about your plan and ask for feedback. We've all been guilty of just starting to implement something because it's so simple. Have you ever ended up telling yourself "I should have realized this in advance" because of poor planning? Yeah, me too.
For new features (and bugs turning into adding something new), you must plan the architecture before you start implementing it. With sound architecture, the implementation can easily be replaced.
5. Stay within the scope
Avoid out-of-scope changes in your PRs. In the worst case, the out-of-scope changes require quite a bit of attention, delaying the completion of the actual task. If you're thinking, "is this inside the scope?" it likely isn't.
Again, as a reviewer, you're expected to reject any PRs that are doing too much, even if it seems ok.
6. Know when to stop
If the PR requires more than a couple of approval rounds, one or more of the previous "Know what you're doing" steps were likely skipped.
Stop! Go back to the drawing board, and don't try to force the current code to be approved.
As a reviewer: if the PR has so many problems that you don't know where to start reviewing, you can simply reject the PR and require additional planning.
Extra: A quick guide to reviewing a PR
Reviewing code is one of the final quality assurance tools before the code is deployed. It's the last chance for us to pick out any issues, making it a very important part of our development process.
Your task as a reviewer is to offer a second pair of eyes for the big picture, not to take responsibility over the details of the code being delivered. Even if you disagree with some of the original developer's design preferences, you should still approve the PR — as long as you agree the reviewed code is working, tested, and not going to cause problems in the foreseeable future.
For example, if you disagree on naming, ensure you aren't complaining about opinions. That being said, if the naming is clearly wrong, comment on it. Also, keep in mind that the PRs must follow the coding style guide.
Self-review
Just like you review other developers' code before approving it, you should review your own code. Self-review decreases the number of review cycles and shows consideration for other people's time and effort. It's a great opportunity to catch accidental changes, forgotten comments, typos, and more.
If you take a short break before reviewing for your code, you should get better results. If you doubt any part of your own code, others will too!
Summary
You'll be able to avoid quite a few pitfalls and delays by following these ground rules of development work.
- Know what you're expected to achieve.
- Plan how you're going to get there.
- Stick with the plan.
- Refactoring is a stand-alone task.
- Review your own code.
- Be strict but fair with your PR reviews.
Learn more about Supermetrics as a workplace at supermetrics.com/careers.
Top comments (0)