6

Almost a year ago I switched to my current job. I have 2 prior years of programming experience but this is my first time working on an entirely new product. I am part of a small team of (5 or less) developers. I have 1 coworker programmer with whom I sit in the office. My coworker who is a Sr. Developer is also project lead, and is therefore responsible for the product specifications, schedule and the expected release date.

Whenever I request a code review, the Sr. Developer claims that it's pointless to do so, unless the regarding feature has been finished. The Sr. Developer is convinced that because of our tight schedule the code reviews would consume too much time, and are too much out of context, because the feature isn't finished yet.

However, I keep making (logic) mistakes of which I am not aware, until the code is reviewed. This is because I'm still unfamiliar with some of the concepts used. These concepts are not pointed out until the code review is done. This means I usually spend alot of time refactoring the code, sometimes tossing days of work.

I've requested to schedule a moment once a week for code review, in hopes of preventing tossing days of work. The Sr. Developer isn't in favor of that idea and claims that he wants me to function less dependently of their input.

Right now I'm looking for insights from Intermediate or Senior programmers, hence I've some questions below. Feel free to not answer the questions and give other input instead, all input is appreciated.

  • Am I focusing too much on the standard coding conventions?
  • Are code reviews on a frequent base required?
  • (If applicable) how frequent should code reviews be done?
  • Why should(n't) a code review be required on a frequent basis?
  • When in the process is the best time to do a code review?
  • How would you deal with this situation if you were me?

Thanks in advance for your input.

EDIT: Thanks everyone for your input. It helped me look at the situation in multiple ways. I've had some talks and as it turns out, there are no specifications to work towards. This is why code reviews are useless because we're not sure we'll be using the code after all... I'll leave it at that.

  • 1
    A lot of logical/syntax or other high-level mistakes can be found in pull request 'code reviews' which are less formal and can be done when other devs have time. Are you not even doing that? I would be surprised if you're all expected to blindly merge code in – n_plum Nov 14 '19 at 17:52
  • 4
    Instead of reviewing the code, could you arrange to spend a few minutes up front discussing your design or plan with your coworker before you get to far in? Rather than having him go line by line thru the code, it might be much faster for both of you – DaveG Nov 14 '19 at 19:50
8

Have you considered a job change? Seriously feedback like code-reviews is vital to YOUR career growth. Some companies take code reviews seriously, some don't. Even if you could manipulate this situation to force the senior-dev to review your code, he's not going to review it to the degree that you want since he feels this is a waste of time. Rather than trying to influence up the chain-of-command, find a position where the feedback-loop is working as it should.

5

Am I focusing too much on the standard coding conventions?

One could argue that if, after a year, you are still making logical mistakes that you cannot find yourself before code review, that you are not focusing enough on standard coding conventions. Do you do any unit testing? What kind of testing do you do? It is general good practice to verify your code before submitting it to review.

Are code reviews on a frequent base required? (If applicable) how frequent should code reviews be done?

I'm not sure they're "required" per se. Code reviews should be done as necessary, not just for the sake of doing them. If you are making incremental changes on a features/development branch, then you should perform most of the code review yourself and only involve others when you have reviewed it to the best of your ability.

Why should(n't) a code review be required on a frequent basis?

Code review is very time consuming. I have previously spent a solid week reviewing code for someone who would not self-code-review at all. We were at over 30 patches before the code was finally acceptable. At the same time, I had other things I was expected to be doing.

When in the process is the best time to do a code review?

When you have verified your code thoroughly, and done a self review. Make sure people aren't wasting time commenting on styling errors.

How would you deal with this situation if you were me?

Do more self-review. You should go through your code and verify the logic before you ask anyone else to review. Test test test everything. Write unit tests, check your logic using Python (great for validating corner cases!). Hold design reviews before jumping into implementation if you frequently find yourself not understanding the logic behind an implementation.

3

In nearly all cases when designing a business process, or optimizing one, it's best if the means are justified by the end goal.

You need to know what the end goals are for your team: likely, it's something obvious like delivering code of a certain quality on a certain timeline. But it's important to not assume, since in many cases there's subtle nuance in terms of what counts as "good enough" or "ready." Once you have that clearly understood, you can test any process or suggested change against that goal. Will "more code reviews" actually help you get to goal X? Having X well defined - including nuance and subtlety - makes that a very easy question.

If it turns out that you think adding more code reviews can actually help, then having gone through this thought process should give you the objective justification you need to help your coworker understand your case. Instead of just sounding like a needy newbie, you can show your coworker that helping me understand my mistakes at an early stage in development will help us, together, reach our goals. Or whatever specific sentence is appropriate. Your coworker may see a few hours a week spent in code reviews as wasted time, but if you can objectively show that doing such reviews actually saves time - and therefore, makes delivery deadlines easier to achieve, then you should have an easier time getting the change you want.

It may also be the case that you're able to identify a problem (you are making logical mistakes) but your idea of a solution (more code reviews) ignores other potential options for solving the problem. So, while you may be able to relate your problem to your team's goals, you may also want to approach this by presenting the problem instead of your solution. This gives others, who may have more experience in software development processes, the ability to guide you towards a better solution.

It's important that you approach this objectively. It's easy, and common, for newer programmers to want to over-optimize things, and over-compensate for perceived issues or problems. People that fall into this trap can find themselves striving for a sense of perfection that ends up having no meaning in the real-world context in which they're actually working.

In a vacuum, "more code reviews" might sound like it's always a good idea - but, practically speaking, it is quite possible to spend so much on review that your overall goals are missed. In other words, your coworker may have a legitimate point.

You listed a handful of questions - ultimately, we can't answer those in a way that's ideally applicable for your situation. However, you can do some reflection on your situation, and come up with the answers yourself.

2

Code review is similar to hygiene... Do you take 10 showers a day or once a week? The answer depends on your environment... anything over the required is a waste of time. The trick is to find out what is required in your environment...in some development shops it’s before you push anything to source control, in some shops, it’s pick out a few commits after things get into source control.

Try pair programming if you’re really wanting 100% code review 100% of the time. I think that’s a little extreme but some shops/developers swear by it.

  • There is a thing called best practices. This goes for QA just as much as for implementations. Even though your need to shower may depend on the environment you're in, the established best practice is to shower once a day, or every two days, not once a month or every hour. – Mär Nov 15 '19 at 15:06
  • Wouldn’t it be nice if all development shops adhere to development best practices? :Sigh: – Goose Nov 15 '19 at 15:20
  • Whether or not you adhere to best practices is your choice. Others can do what they want, but they also deal with the typical nonsense as well. Almost all who ever worked in development stumbled accross some poorly maintained codebase at some point in their career. These typically demonstrate what technical debt means in terms of costs. – Mär Nov 15 '19 at 16:00
1

I kind of agree with the point of

pointless to do so, unless the regarding feature has been finished.

Logical mistakes not suppose to be a reason for code review, there are specs and QA to catch that.

But, IMHO, your tasks may be broken down in to larger than logical (unmanageable) pieces of work (tasks).

Are you guys using TDD?

  • First of all, code reviews are part of the QA process. Second of all, features are checked using software testers, not code reviews. Why would you need a feature to be completed, when your review aims to eliminate stuff like, function xTwice( int x ){ return x + x; }, to present a naive example? Code reviews are neither done on singel commits, nor on whole features, but typically on merge requests into the master. Seems like the Sr. simply has no concept of the term technical debt. – Mär Nov 15 '19 at 15:04
  • @Mär Code reviews are not connected to QA, most of QA people are not required to know environment product is developed in. If Function twice is working, the code can make to QA but not in to the repository. P.S. reviewing "Work In Progress" code IS meaningless and waste of time. Developer should concentrate on his task and not how it looks, When it works - then it can be reviewed and refactored. Perhaps you need to look up terms like "Gold plating" and KISS – Strader Nov 15 '19 at 15:18
  • en.wikipedia.org/wiki/Code_review Read the very first sentence. QA is not "people", QA is a process. It may have people or even departments attached to it, but it is typically not at the end of a process, but applied during. – Mär Nov 15 '19 at 16:03
  • What you are thinking of is a QA department that ensures quality after a process pipeline XY has been worked through. This is not anywhere close to how QA and specifically code reviews are done in practice. A common technique is review everything that is supposed to be commited to the master. See the four-eyes principle: unido.org/overview/member-states/change-management/faq/… – Mär Nov 15 '19 at 16:08
1

I would focus on making your pull requests easier to review. I review several per day. In the best case, I can complete a review in under 5 minutes, while I'm waiting on a build or something so it doesn't interrupt my work. In the worst case, it's a half hour per day for several days until it can get merged, and I have to specifically allocate time to address it. Thankfully, the first category is much more common. What makes the first category so much easier to review?

  • It makes a minimal change. Our latest big feature required upwards of 30 pull requests to complete. Other teams sometimes do the same amount of work in one or two pull requests, but then take weeks to get it up to standard to merge it.
  • It is thoroughly tested. The vast majority of my comments on easy code reviews are asking to add a test for a particular use case.
  • There aren't any design surprises. We've already had design discussions over chat or at the whiteboard. I don't have to tell them they have been going in the wrong direction for the last week.
  • There aren't any apologies. If you know I'm going to complain about something, don't apologize for that code, just fix it. If you don't know how to fix it, hit me up on chat.
  • It clearly denotes what will be coming later. If your pull request is just laying runway and not actually finishing the feature yet (as should frequently be the case), make that clear in your pull request description. This is different from an apology. Apologies are for something bad quality you don't intend to fix. This is for something good quality but incomplete, with a defined plan for completion.
  • It provides context. I'm a good reviewer because I'm familiar with the domain and the language. I'm usually not as familiar with your exact feature request as you are. Provide the context I need to get up to speed.
  • The reviews are spread around. It shouldn't be just one person doing all the reviews. Peers new and old can all provide valuable feedback.
  • It has been refactored. I can tell when people have made refactoring passes and when they haven't.
  • It shows a willingness to accept feedback. If you argue about every suggestion I make, forcing me to write lengthy justifications, it's going to take me a lot longer to get the time and motivation to finish the review.
  • It was not done blindly. If you don't understand how the code you're changing works, don't change it yet. Gain an understanding first. Lengthy code reviews have a lot of comments along the lines of, "This code doesn't work the way you think it works. Here's the background you need to do it right."
  • 1
    "We've already had design discussions" - that is probably the most important phrase in your answer. The asker seems to be mistakenly asking for a code review for too early, when they are at the point where what they really need is mentorship in the form of having a specific discussion about something they are uncertain how to do. As you point out even peer situations benefit from such discussions; but in the asker's junior-senior situation it is even more important. – Chris Stratton Nov 16 '19 at 5:37
1

The Sr. Developer is convinced that because of our tight schedule

Right there is what you need to focus on, not code reviews or best-practice. If Sr. Dev is not reviewing your code, it is doubtful that any other developer is reviewing his.He is getting whipped from above. And no, self code-review does not work. No-one wants to see the ugly in their own code, especially when rushed.

once a week for code review, in hopes of preventing tossing days of work. The Sr. Developer isn't in favor

Where is the continuous-delivery? Why are tasks not being broken down into at most one day deliverables?

What can you do?

  • Document request, CYA.
  • TDD - test driven development. This will alleviate much of your apprehension, allow you to understand the problem finer, and hopefully break the feature apart.
  • Break the feature down into smaller features of one day deliverables.
  • Very difficult - try to show Sr. Dev how code review can loosen the tight schedule by reducing rewrites. See if their are tasks/features Sr. Dev can be offloaded, to you.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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