Sign up ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free.

Before asking my question, I must explain the situation.

I'm working for a company as a junior software engineer. One of the seniors always stops me when I have finished my development and want to commit.

He always wants me to wait for him to review it. This is ok, because usually he finds some bugs, and does some optimizations.

However I must commit my code before the deadline. When I finished, I call him and say it is finished. He usually comes late. So my code is late too.

My question is, what should I do? Should I wait him for a review?

EDIT: Addition to the question. I'm curious about one more issue.

I want to be free when coding. How could I gain the trust for freedom of development?

Some explanations: I've talked with him about this. But it didn't help. We use an issue tracker already, but there isn't any task for reviews. There are just development and test tasks.

share|improve this question
Talk with him about it. – Florian Margaine Apr 30 '13 at 6:03
email him to say it's done and you are waiting his review. Then you can always refer back to that email if someone asks why you missed a deadline. – dreza Apr 30 '13 at 6:08
An issue tracker is just a tool to remind you of important steps the team does not want to forget. If your team sees reviews as one of those steps, reviews probably should be added as separate tasks. If your team can handle which of the parts of the code has to be reviewed without entering that explicitly into the issue tracker, then there is no need to add such tasks. That is something you should discuss with your team. – Doc Brown Apr 30 '13 at 9:35
Be patient, you have no idea how beneficial it is to have a second pair of eyes (esp. a senior) review your code. – JeffO Apr 30 '13 at 12:53

9 Answers 9

up vote 67 down vote accepted

So my code is late too.

No, it is not your code, it is the code of you and the senior. You are working as a team, you have a shared responsibility, and when you two miss a deadline, it is the fault of both of you. So make sure the one who makes the deadlines notices that. If that person sees that as a problem, too, he will surely talk to both of you together - that may help more than a single talk with you coworker.

And to your EDIT:

I want to be free when coding. How could I gain the trust for development freedom?

Reviewing code is one of the most important quality savers. It is virtually impossible to write excellent code without a second pair of eyes, even when you have >20 years of programming experience. So in a good team, everyones code should be constantly reviewed - your senior's code as well as your code. This has nothing to do with distrust against you in person (or, at least, it should not). As long as you believe that "free coding" without a second pair of eyes is better, you are still a junior programmer.

share|improve this answer
@blank: you missed my point - I am talking about responsibilities and your viewpoint about them. You believe you alone are responsible for holding the deadline - that's wrong, and you should make sure that everybody else in your team knows that, too. – Doc Brown Apr 30 '13 at 6:13
You are right about it. But there is no responsibility for the senior. There is no task for him to review the code. But he does it always. – yfklon Apr 30 '13 at 6:17
@blank: that's exactly my point - if the senior tells you to wait, he takes responsibility. Make that transparent to the one who defines the deadlines. – Doc Brown Apr 30 '13 at 6:19

In a good team, you should have a queue of development tasks assigned to you in an issue tracker.

That way, while you are waiting for a reviewer, you could (should) work on next task waiting in that queue. Once you get used to work in that fashion, this will open an opportunity to have your changes reviewed in "batches", thus decreasing delays.

  • If you don't have such a "queue", discuss this with your manager, or better yet, with reviewer. If your team doesn't have reasonably convenient issue tracker for stuff like that, consider studying job boards or company internal job opportunities to find a better team (you might also discuss this with manager / reviewer but don't expect this to help - lack of a good issue tracker is often a symptom of something being severely broken in a team).

I want to be free when coding. How could I gain the trust for development freedom?

To find out, you need first to understand the purpose of code reviews. You mentioned trust - that's a good "approximation", but not entirely accurate one.

  • For example, in one of my recent projects development has been done by a mini-team of me and my colleague. We fully mutually trusted and respected each other - but despite this we reviewed 100% of code. We were doing it because this allowed us to find and quickly fix some bugs and, which is also very important, because reviews didn't take much time and didn't block our work.

You see, it would be more accurate to think of code reviews in terms of efforts invested in order to prevent certain risks. In a good team, you can expect sort of shared understanding of how to "properly balance" this. Note there is no one-size-fits-all proper balance, it heavily depends on a project - risk and impact of bugs in a mission critical software naturally differs from that in a non-critical application.

Using your example, you can expect "blocking reviews" as long as efforts invested by your reviewer are justified by finding bugs and improvements that are better be fixed prior to committing your code.

They likely expect that with practice and with guidance received at reviews you will get better at coding, so that they will find less and less issues worth fixing prior to commit. As soon as they discover that your code got "safe enough" to allow less cumbersome "risk prevention measures", you could expect the process to change, for example to reviews after commit.

Depending on a project, at some point your code may be even considered safe enough to skip reviews at all, leaving discovery of the bugs to testers (but that not necessarily will happen, see my example above).

share|improve this answer
Sounds like you suggesting a technical solution for an organizational problem. To my experience, that seldom works. – Doc Brown Apr 30 '13 at 6:10
@DocBrown I don't think this answers really focuses on a technical solution. The core of the solution is "you should have a queue of tasks assigned to you". That is an organizational solution to an organizational problem. Whether that queue is maintained in an issue tracker, emails, a spreadsheet, a whiteboard, or a stack of post it notes is just a detail. – Carson63000 Apr 30 '13 at 6:13
@Carson63000 exactly so. I would also add that having tasks in an issue tracker so that one doesn't need to run to manager / senior to ask for new task is also an organizational detail (and not quite minor in my experience) – gnat Apr 30 '13 at 6:18
@gnat: well, you could have written "for example in an issue tracker" to make that more clear. But I am not sure if the question you are answering (the one from the title) is the core point of the OPs question written in the text below (which is a different one). – Doc Brown Apr 30 '13 at 6:21
@DocBrown I did not do that intentionally, because I believe it's too important organizational detail to state as "for example" (the very thought of junior teammates coming to me to ask for next task when they are done with current sends shivers down my spine) – gnat Apr 30 '13 at 6:25

There are multiple possible answers here, depending on exactly what your problem is.

  • If your major concern is "I am missing deadlines", no. The two of you are jointly missing deadlines. Can you (with confidence) say "I will be done in an hour, can we do the code review then"? That might be enough. Can you complete the code the day before the deadline? That should be a plentiful buffer. Are you completing your code, with plenty of buffer between "please review" and the deadline? If the latter, it's not even a joint fault, I'd say.

  • Code should always be reviewed. I can't check anything in without (at least) a second set of eyes and another human going "that's OK". That goes for projects where I am the lead programmer as well as for projects where I don't normally contribute (but have managed to find a bug impacting me that I want fixed). However, the strictness of a review is very much based on trust. If I trust that the person wanting to submit code knows the code-base well, I will not be as strict as if I don't know how well the person knows the code-base.

share|improve this answer

My question is, what should I do? Should I wait him for a review?

No, you shouldn't just sit idle. There is always something to do. As Gnat suggested, there should be a queue of tasks. Or, in an agile way of development, list of tasks assigned to you for the current iteration. If you sit idle, there is something wrong in the organization of your company or your team.

Another thing is : is your senior supervisor really checking every piece of code you do? If yes, you could also do pair programming.

I want to be free when coding. How could I gain the trust for development freedom?

There are some regulations which require that senior checks the work of junior (I think medical iso 62304 requires this). If it is so, you can not do anything.

What you can change is to ask senior not to check literally everything. You can set code review process, and check important things.

share|improve this answer

Use git locally, commit your changes to a branch and start on task 2 while you wait. Then when he gets done, you can merge his changes into your new work and you are already ahead of the curve on the next task.

Do this long enough and pretty soon, he can review 2 or more things at one sitting. Pick things where the lines are unlikely to overlap to minimize conflicts.

share|improve this answer

One solution to this could be to get the senior developer involved much earlier by Pair Programming on your work.

Wikipedia page on Pair Programming

The most obvious advantage for you would be that the review happens much earlier in the process so you no longer have to wait for the senior developer.

As well as this you would be able to see the thought processes and techniques of the senior developer as he writes the code, and learn from this.

You might have the problem of the senior developer may not want to pair with you. That can be difficult, but my own experience is that both senior and junior developers gain a lot of experience from pair programming.

There is also often the concern that you will half productivity by having 2 developers work on the same piece of work. It's difficult to measure what the affect on productivity is with Pair Programming, the most common response I've heard is that productivity of teams that do pair and those that don't is about the same. (If anyone has knows any good research on this I would love to hear about it)

share|improve this answer

I think performing manual code reviews is... well... kinda 80's. Well, maybe 90's.

In this modern era of continuous integration and online code review systems, you really don't want to hold back any code commits just because you're afraid that "it might break source control".

Come on, people. That's what changeset (or change lists) are for. You make your programmers feed the hungry maws of your source control system. Then your continuous integration server kicks in with a litany of targeted builds (well, hopefully just the daily build, but some of us get carried away). If anything breaks, you put the code monkey trophy (usually a plastic toy that someone found from a Lucky Charms cereal box) on the perpetrator's desk, and roll back the breaking change list. Well, some continuous integration systems automatically blasts out email/IM/desktop notifications to everyone in the team/department/organization that the build is broken, along with a nifty hyperlink to show everyone who exactly broke the build in which file or test. It's now the hapless programmer's job to fix whatever broke the compile/unit test/integration test/etc.

As this process runs on, the code review system kicks in (again, triggered by the check in). A list of qualified team members are notified of the change list being committed to source control, a review is started in the review system, and everyone starts to make annotations to the changes in the change list. Hopefully everyone will say "LGTM". If the programmer is smart, he will remember to pray/bribe/hide. If there are serious issues, the reviewers can create a defect (which can be hooked into the bug tracking system), or even require the changelist to be backed out. Yes, backed out changes hurt not only the ego, but the mind, that is true. It is a good seasoning on the junior developers, to reintegrate rejected change lists.

If your dev environment is lacking a CI or a code review system, you should seriously investigate these. A couple of links might help you:

Atlassian Crucible
JetBrains TeamCity
Cruise Control

If you're going to get a CI server, you should also seriously think about unit test frameworks. If you're a C# dev, look into something like NUnit to get started.

share|improve this answer
I do not know who downvoted this answer, but I do not agree with him. I totally agree with code4life that code review should be done from source control, not from local copy. A change that takes some day to complete should be committed every day, maybe in a branch, but still be committed daily. From here, code review could be done on partial change, and CI, daily build and integration tests can be applied on that branch when it becomes stable enough. – jfgagne May 4 '13 at 9:29
Yup. And code reviews are done against a changelist these days. Rejected changelists get backed out (that's the nuclear option), or defects get raised. We want to throw the commits against the CI as soon as possible, per McConnell's Code Complete. – code4life May 4 '13 at 15:33
I guess whoever downvoted the answer didn't read past the first line. I think the first line is a little misleading. – Vitalik May 10 '13 at 14:07
LOL, well, the 2010's... it's the era of ADHD-ism...! – code4life May 10 '13 at 14:09

Not a full answer on its own, just an addition to the excellent answers above...

Do you review your own code before checking it in? I know it's not the most fun, but I try to make myself do it most of the time. I have been programming professionally for 20 years (34 years total), but I usually find at least one bug and/or one thing I forgot, or could at least make better. I agree with the sentiment that your code should always be reviewed and that a second set of eyes is better than one pair. But even the same pair going over the code twice is better than once.

Do you write unit tests for your code? In addition to unit tests, I also have a little shell script that searches for the most common mistakes I personally make. Some of it is English grammar and spelling, some is coding issues that the compiler doesn't catch. I run it before checking in large changes as a courtesy to everyone downstream.

I normally let people write their code and occasionally complain about it later, but I don't review every single check-in. I once worked with a very junior programmer whose code I had to review and usually undo because they made so many mistakes. That didn't end well. You are in a profession where it's often more important to get it done right than done on time. If you learn from your mistakes, you will go far.

If you can minimize the number of changes your reviewer needs to make to your code, you maximize the chance that they will trust you to write code that doesn't always need to be reviewed so carefully. If you want freedom from reviews, take the maximum responsibility for the quality of your own output.

Some or all of these suggestions could be done while waiting for someone else to review your code.

share|improve this answer

You tell him in advance when your code will be ready, not at the moment when it's done. You should be able to determine that approx. one week ahead. That gives him time to prepare and plan the review so that it fits into both your plannings.

share|improve this answer

protected by gnat Jul 1 at 23:20

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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