What should I do when waiting for a review?

Question

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

Answer 1

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.

Answer 2

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

Answer 3

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.

Answer 4

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.

Answer 5

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.

Answer 6

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)

Answer 7

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.

Answer 8

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

Answer 9

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.