10

I'm a recently graduated software engineer at a small startup with 7 staff, of whom 3 work on software - the CTO, myself, and another software engineer. I've been there nearly a year, and the other software engineer has been there a couple of months. I'm nobody's boss, but I have some influence because I represent 1/3 of the entire software team and I have more knowledge of coding standards than either of the other two developers due to previous experience at a different company.

At my recommendation we've recently started doing code reviews for code that is getting pushed to a few core libraries which other projects depend on. The other team members are happy to have their code reviewed and to review code (unlike these questions) - my problem is simply that when either of them reviews my code, they invariably look it over for a minute or two, occasionally make a couple of comments, and then approve it without suggesting any changes.

Not once since introducing code reviews have I had to make any changes prior to approval. I know they're missing things, because I've caught bugs myself in things I've committed, but I can't point out flaws that I haven't noticed.

What specific things can I do to encourage the rest of my team to be more effective during code review?

I've tried the following:

  • Simply asking them to be careful and critical
  • Pointing out after each overly quick review that it was very quick
  • Drawing attention to things that I'm uncertain about when submitting code for review
  • Asking questions about the code I'm submitting to force them to study it
  • Reviewing their code according to the standards which I would expect them to review mine
Improve this question
4
  • In what ways are code reviewed? For example are they in source control, like github? Do they comment by coming to you or writing it in the code or how?
    – Dan
    Mar 15, 2018 at 18:21
  • We submit pull requests in Bitbucket. Generally we discuss code reviews in person, since we all sit close to one another.
    – walrus
    Mar 15, 2018 at 18:23
  • 6
    Reading code is not the same skill as writing it. Many people who are perfectly good professional developers cannot read someone else's code and really understanding what it does without running it in a debugger. You cannot expect people to find bugs by eye-reading your code.
    – Affe
    Mar 15, 2018 at 19:22
  • Regarding your five suggestions, they are completely wrong. the sentence you're looking for is "This code review is not thorough enough." or just "That was a bad code review, it needs to be done properly" or "Your code reviews are not acceptable. We need full industry standard code reviews."
    – Fattie
    Mar 15, 2018 at 21:00

6 Answers 6

Reset to default
7

Causing change in a software development process can be difficult. Especially when you are not inherently the person who "owns" the process (I'm assuming you aren't, given the way you described the organization's structure).

Look at the culture and strategy of the leadership. Do they care about software quality? I know that's a hard question to answer but you likely will not be able to push a quality-related initiative if there isn't a culture of embracing quality.

Any time I make a suggestion for a change, or want to bring up a performance indicator or metric, I look for three things to help me decide if my suggestion will stick or not:

  1. Is it actionable? Is there a reasonable assumption that the code review will actually result in specific changes? Do your team mates know what they're looking for? Is there a software standard the team works against? Will there actually be specific improvements as a result of the process you're suggesting?
  2. Is it measurable? Do you want the software to be "better" after the review? Or do you specifically want it to have fewer bugs? Are you tracking bugs?
  3. Is it strategic? Does (steadily improving in a measurable way) software quality matter to the company? Is there some implicit or explicit expectation from leadership, or some clearly defined end goal or path forwards? Can you show that software quality will have a direct impact on helping the company meet its goals?

Lots of people have lots of great ideas, and there are always ways that every process (software development included) can be improved - but, IME, it's rare for suggestions to be incorporated into a group unless they pass these three tests.

I've tried the following: Simply asking them to be careful and critical

No surprise that didn't work. You probably need to be much more specific. "Be careful" isn't helpful. "Can you help me out by checking my code for XYZ specific mistakes?" "Can you make sure I commented appropriately?" etc.

Pointing out after each overly quick review that it was very quick

See above

Drawing attention to things that I'm uncertain about when submitting code for review

Probably your best idea!

Asking questions about the code I'm submitting to force them to study it

Were the questions specifically related to improving the code quality, or were you just trying to trick them into actually reading the code? I feel like you meant the latter, which doesn't help you pass the three tests I mentioned above.

Reviewing their code according to the standards which I would expect them to review mine

What standards? Do you have an actual, documented standard? Or are you just expecting them to catch on to the things you're finding in their code?

Also, I'd just like to say- welcome to the real world! I find that it's common for recent grads (who are often used to developing in a vacuum, and are used to their work being inspected/graded) to struggle with the lack of formality in the real world, especially when working on small teams and at newer companies.

Improve this answer
4
  • idk. i'd be pretty unimpressed if someone dragged me into a meeting every week and said "i wrote some code, i don't know what it does, can you check please?". i'd think "why didn't you check beforehand?" that whole "i need help" should be done before the code is written, not after the fact.
    – bharal
    Mar 15, 2018 at 18:52
  • I'm not sure which (if any) of your questions aren't rhetorical, but I realise now that although we do have actual standards I've done a poor job of communicating the fact that code reviews should be helping to enforce that standard, and a poor job of communicating expectations in general. Regarding the company culture, there are only three of us, so I'm trying to instil a culture before we grow(?) a culture that doesn't care about it.
    – walrus
    Mar 15, 2018 at 22:00
  • 2
    @walrus I think you're doing the right thing, you just need to be more specific and push in a direction that will be accepted. Think of your colleagues as computers. When a computer isn't doing what you want, you don't just sit there and say "try harder!" Instead, you give the computer specific instructions on what you want it to do. Often, you have to "program" employees in the same way. "try harder!" doesn't work. The complication is, computers do what you tell them no matter what, whereas people only do it if they're motivated to - which is why strategy and culture are important.
    – dwizum
    Mar 16, 2018 at 13:15
  • 1
    Also - if this wasn't clear, the other part of my answer was that sometimes, a "good" idea isn't the "right" idea. You can walk in to any software shop and find dozens of ways to make the process better, but that doesn't mean any of those potential changes would actually stick, no matter what tactic you use. Your ultimate goal is creating a culture where quality is important. Code review is probably in the end game of that culture change, not the very first play. Before you try to improve quality, you need people to understand the value of quality, and how it's measured or tracked.
    – dwizum
    Mar 16, 2018 at 13:20
4

One point - things are different for all teams, so if something doesn't work try it a different way

Two points

  • different people are good at and like doing different things. You can make people do things, but it is a waste of their talents
  • it seems you are using code reviews but have no purpose for them

One

In the first case, you should perhaps think about how code reviews are executed. In the past, I've seen it done separately, by people at their desk, and they just "ok" or "decline" some code as they see fit. This is more efficient, however it does mean there's less communication. You could also have people do this task "solo", and then once/twice a week go over the code reviews as a group to improve communication.

Two

The second point begs* the question: Why are you expecting your team-mates to catch your bugs in code reviews? I've never really seen them do this unless it's a pretty obvious bug.

They're not a fail-safe for testing (which is how your bugs should be caught, before the code review even happens), it's more

  • knowledge sharing
  • stylistic consistency

I won't go over why you've tried failed, it boils down to "you're not giving a concrete goal", although I will note that :

  • Reviewing their code according to the standards which I would expect them to review mine
    • Sure. This might work - if you annoy someone, they'll annoy you back the same way, because implicitly they're trying to show you how annoying you are (show, don't tell). But I don't think this is an ideal way to get code reviews working.

Your failure is that nobody on the team understands what the code review is for, what the objective is, and why they're doing it.

However, they are doing it, so they understand it is important. That's the biggest obstacle, and you're past it. Now you need to give them an understanding that code review is for knowledge sharing.

But maybe your team all own their separate areas, so you own the database, Danny owns the front end and Sam runs the back end. You'll struggle to get code reviews working in this system, because nobody really cares, or knows, what the other person is doing.

So perhaps you need to start sharing the work cross-functionally, instead?

You need to think why you wanted the code reviews. If its to catch bugs, then you need proper testing first and foremost. If its to improve team communication, then they'll naturally spend less time looking and more time communicating.

Doing code reviews "because i read it in a blog" isn't helpful, nor is it convincing. Doing them "because it improves code clarity" is, but if you cannot measure code clarity, then it's hard to show they're working, isn't it?

  • Sorry language puritans! You lost this one.
Improve this answer
1
  • 1
    My thoughts exactly. In my version of code reviews, I'm checking broad strokes: no obvious injection vulnerabilities, unit test code coverage, no obviously poor patterns, no broken unit tests, etc. I often don't even execute the code -- if I'm expected to dig that deep to find an issue, then that goes WAY beyond code review, IMO. Sounds like OP has a much higher set of expectations.
    – bvoyelr
    Mar 30, 2018 at 16:30
2

It's not high school, say these words:

This code review is not thorough enough.

Note - say those words either to your manager, or, the colleagues.

"You're not at high school", as they say.

It's a place of work, the sole purpose of the company and the employees, is to decisively make money. Speak up clearly.

BTW this may help you:

I always suggest ..

What would Richard Branson say?

(You can substitute the national business-hero in your own country or culture.

Let's say precisely this happened to Richard Branson (in the hypothetical, he's a young programmer.) What would he say to his colleague?

This code review is not thorough enough. Let's redo it.

What would he say to his boss?

Jack's code review is not thorough enough. Let's redo it.

Couldn't be simpler. Do you think for half a second he'd waste time thinking about ......... anything else? No, he'd say the sentences above.

You can too!

Improve this answer
2

Finding bugs in a code review is hard. The reviewer has to work through all the logic of the code to find some obscure mistake - with no certainty that there is a mistake at all.

It's easy to check if comments look reasonable, if names are sensible, and if the code is well-structured. You can also spot mistakes such as un-initialized variables, though a static code analysis tool could do that as well.

If you want to find bugs, do more testing and do better testing. Save the people for doing creative stuff.

Improve this answer
1

We have a form (a Word doc) that specifies the things to look for in a code review. Things such as:

  • Appropriate naming conventions and comments are used
  • Minimize the presence of dead source code
  • Strong data types are used whenever possible
  • Calculations with NULL parameters or zero denominator not handled properly, allowing invalid value returns
  • Avoidance of embedded SQL or DB connections
  • Readability of the source code with regards to indenting, function naming, data flow
  • And so on (40+ items on the list)

For each item there is a Pass/Fail/NA checkbox with a space for comments for that item. Then there is an overall pass, pass with exceptions or reject section with a comment area.

For us, as a regulated industry, a proper code review is required before moving any code - web, sql script, etc. - into a production environment. This is part of the release documentation. When we are audited if this is not part of the release package there can be fines and/or people losing their jobs.

Good luck on this. Code reviews are something that some developers don't like and some managers don't emphasize. After they have a problem with code that was not adequately reviewed they will get the point.

Improve this answer
0

What I recommend is pitching to your boss to use github. It has a nice community feature in it where it encourages comments and reviews. They have a enterprise account available.

Things you can enforce (this worked when we had github):

  1. Have one person who approves pull requests but only if there are two thumbs up and all "TODO" or questions answered
  2. Assign one or more individuals who reviews pull requests. It can be random or by area experts.
  3. All questions must be answered before the pull can be accepted into the master branch

Also drafting a code manual encourages compliance. Such as function names, tabbing, variable names, framework, if-else level restrictions, function size restrictions, etc. That way everyone can at least comment about unacceptable code and on top of that everyone agrees on a easy-to-read format. Github can enforce this in many ways.

Improve this answer

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .