Every year in january we process a big task with our system. While the performance during the task was above average the maintaince follow up is currently having a lot of trouble with jobs running too long and out of their respective time schedule. Therefore I am tasked with the optimization of those maintaince jobs and I am doing thusly the most thorough code review the system has ever seen. As I am good with analysing code I am also doing a lot of bug fixes.
This means I see lots and lots of bad or completely wrong code and implementations. My question is: How can I deal with that in front of my coworkers?
I hate it if I cause a bug (which I guess happens to everyone sometimes) and someone else fixes it without telling me about it. Because then I cannot learn from my mistake. On the other hand I do not want to be the guy on the team who is constantly nagging about how everyone else has to do his or her work. Also I am a bit wary of my own hubris. I do not want to find myself in the position that I start looking down on coworkers just because my biased mind thinks my code is superior. Currently I am not checking who has caused a bug I just fix it. I am not sure whether this is the best way to go.
I am a Junior level developer and English is not my mother tongue. Any advice (and grammar edits) are highly appreciated. The team follows the Rational Unified Process framework.
7
This will depend on your company culture and processes. But personally I wouldn’t just fix it for a number of reasons.
- It may actually be correxct and my understanding might be wrong
- It may be the lesser of two evils, it might be wrong but may prevent some other part failing altogether
- It may not be worth the businesses time and money to fix it, there may be more important things for me to work on.
The preferable thing to do in this situation is raise a bug report. Note for some work this is necessary anyway e.g. for safety related work all faults probably need to be formally raised so that they can be safety assessed.
On some projects, a process exists when you discover a potential problem in the code:
-
First tell the author or your project leader,
-
Report it in the issue tracker,
-
Wait for assignment of issue resolution to you or to another team member.
The general rule is to NOT fix a bug if not explicitely asked for. Some reasons for that are:
-
It costs time and you are not the best person in the project to decide how to balance between that time cost and the criticity of the bug.
-
If you don’t know the code base enough, it may introduce bugs in parts that you didn’t imagine
-
It is not a bug, it’s a feature.
You may be in the perfect situation with this do-it-once-and-thoroughly code reviewing. As you are fixing bugs left and right, what you should do is keep note of the bug patterns/types. Do not tell individual developers (unless being asked) and also do not care about who wrote the code – just fix it and keep track of it.
Once you’re done with your work walk up to your manager and tell him about the situation:
We have a lot of developers doing bad things in the code and they are
probably not aware of how to do it better. During my review work in the recent weeks, I have kept track of the
typical mistakes that have been made. Would it be possible to arrange
a meeting with the developers in which we could discuss the most
blatant problems in order to educate the whole team and dramatically
reduce our error potential in the future?
That kind of win-situation will almost never be rejected by a manager and as you will present the error patterns you avoid offending or singling out any specific developer. You also reduce the amount of time you spent talking to others about their bugs and you ensure that everyone in the team is aware of such pitfalls afterwards.
Code review is never a one-person task. Code review happens with both the reviewer and the author present and looking at the code togehter (ideally), or communicating via tools (e.g., Review Board). If you’re not involving the author, you’re not reviewing code, you’re fixing bugs. If you’re fixing bugs, don’t worry about the review aspects.