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 BrownApr 30 '13 at 9:35

3

Be patient, you have no idea how beneficial it is to have a second pair of eyes (esp. a senior) review your code.
–
JeffOApr 30 '13 at 12:53

9 Answers
9

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.

@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 BrownApr 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.
–
BFaceCoderApr 30 '13 at 6:17

26

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

Sounds like you suggesting a technical solution for an organizational problem. To my experience, that seldom works.
–
Doc BrownApr 30 '13 at 6:10

5

@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.
–
Carson63000Apr 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)
–
gnatApr 30 '13 at 6:18

1

@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 BrownApr 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)
–
gnatApr 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.

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.

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.

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)

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:

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.
–
jfgagneMay 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.
–
code4lifeMay 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.
–
VitalikMay 10 '13 at 14:07

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.

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.