7

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.

Improve this question
7
  • 1
    First simple hint: Avoid "this is wrong", prefer "this could be implemented neater/faster/... this way: ...".
    – Joachim Sauer
    Feb 7, 2013 at 9:39
  • Are you have QA? If it is, QA should return your task to you if finds errors.
    – Denis
    Feb 7, 2013 at 9:48
  • 2
    @Denis: QA and code quality reviews focus on very different kinds of quality (although one influences the other)
    – Joachim Sauer
    Feb 7, 2013 at 9:49
  • @JoachimSauer I understand that, but, man, you are junior and if you go to regular or senior developer and for example say to him: "Your code is shitty, and I know how do it better" ) I think you will have many enemies but code don't makes better. If you feel and know what you coworkers are weak just go away. I saw teams where seniors or leads were complete idiots it happens. Search for team where you can improve your skills.
    – Denis
    Feb 7, 2013 at 9:55
  • @Denis: sometimes it's a question of phrasing: "I've seen your code and personally I'd have implemented it <this way>. Could you explain to me why your approach is better?". You might be surprised and get an actual answer ;-) Also: see the question I linked to. Its answers contain everything I post here and are much better worded than my comments here.
    – Joachim Sauer
    Feb 7, 2013 at 9:57

4 Answers 4

Reset to default
4

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.

Improve this answer
2

On some projects, a process exists when you discover a potential problem in the code:

  1. First tell the author or your project leader,

  2. Report it in the issue tracker,

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

Improve this answer
1

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.

Improve this answer
0

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.

Improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.