323

I can give you an example of a corner case that could never occur that caused a disaster. When the Ariane 4 was being developed the values from the lateral accelerometers were scaled to fit into a 16-bit signed integer and because the maximum possible output of the accelerometers, when scaled, could never exceed exceed 32767 and the minimum could never fall ...


306

The premise of the question is, frankly, astounding. We suppose that there is a large change to fragile, complex code, and that there is simply not enough time to review it properly. This is the very last code you should be spending less time on reviewing! This question indicates that you have structural problems not only in your code itself, but in your ...


274

Ask for a test case that fails without the change that succeeds with the change. If he can't produce one, you use that as justification. If he can produce one then you need to explain why the test is invalid.


251

If it cannot be reviewed, it cannot pass review. You have to understand that code review isn't for finding bugs. That's what QA is for. Code review is to ensure that future maintenance of the code is possible. If you can't even follow the code now, how can you in six months when you're assigned to do feature enhancements and/or bug fixes? Finding bugs right ...


227

an obscure case that is extremely unlikely to happen--in fact I'm not sure it is even possible to occur Not having untested behaviors in code can be very important. If a piece of code is run e.g. 50 times a second, a one in a million chance will happen approximately every 5.5 hours of runtime. (In your case, the odds seem lower.) You may talk about the ...


219

Sometimes, the best way to know, is to come back to code you wrote six months ago and try and understand what it was written to do. If you understand it quickly - it's readable.


175

The truth is that probably in 2 years when you will see your current code you will agree that it was a mess. Learning programming is a never ending process and there will always be someone who is better at it than you. So if person who said that your code is a mess is not just mean and it is not another case of "I would do it better" disease common among ...


160

Double-check your motivation. If you think the code should be changed, you ought to be able to articulate some reason why you think it should be changed. And that reason should be more concrete than "I would have done it differently" or "it's ugly." If you can't point to some benefit that comes from your proposed change, then there's not much point in ...


152

Reviewers should be objective. It's clear that you've formed an opinion about the code in question before you've even reviewed it, and it sounds like you and the fixer have staked out positions. If that's so, then you're going to have a difficult time appearing objective, and an even more difficult time being objective. None of that helps the process, and ...


139

As a reviewer, your job is to check if a piece of code (or a document) meets certain objectives that have been agreed upon before the review. Some of these objectives will typically involve a judgement call whether the objective has been fulfilled or not. For example, the objective that code must be maintainable typically requires a judgement call. As a ...


124

How to find positive things in a code review? After some serious quality problems in the last year, my company has recently introduced code reviews. Great, you have a real opportunity to create value for your firm. After the first few weeks my colleague started to let things slide, to not cause trouble with the co-workers (she told me herself, ...


120

Any sense? Yes. Even if you know nothing about the semantics of a programming language, you can still read characters and notice inconsistent formatting, missing comments, badly chosen identifiers, obvious duplication etc. Much sense, or enough sense to repay the cost of your time? I'm not sure. This depends on your position, the importance of code reviews ...


108

The first rule of any professional software engineer is to write code that is comprehensible. The second example looks like an optimized example for an older, non-optimizing compiler or just someone who happens to want to express themselves with bitwise operators. It's pretty clear what's going on if we are familiar with bitwise operations but unless you're ...


104

Don't bother picking something good unless its a solid concise example and is directly related to the focused issue. I won't sugar coat it - from the sounds of it you are dealing with at least one person who is insecure with their abilities and is handling being challenged about their work in an immature way. They are also likely bad at their job - a good ...


96

One of the primary goal of a code review is to increase quality and deliver robust code. Robust, because 4 eyes usually spot more problems than 2. And the reviewer who has not written the additional code is more likely to challenge (potentially wrong) assumptions. Avoiding peer reviews would in your case only contribute to increase fragility of your code....


95

Code reviews can be toxic, time wasting, will to live-sapping nerd wars. Just look at the divergence of opinion on things like clean code vs comments, naming conventions, unit and integration testing, check in strategies, RESTfulness, etc., etc. The only way to ensure you avoid this is to write down the rules for passing a code review. Then it's not a ...


93

It is: maintainable if you can maintain it. easily maintainable if someone else can maintain it without asking you for help readable if someone else, on reading it, correctly understands the design, layout and intent The real test for 1. is (as Alex in Paris and quant_dev say) that you can pick it back up after a few months doing something else. The test ...


92

First of all, make use of tools to check as much as you can. Tests (backed up with some reasonable code coverage) will give you some confidence of the correctness of the code. Static analysis tools can catch a lot of best practice things. There will always be issues that you need human eyes on to determine though and you will never do as good a job reviewing ...


87

Feature envy is a term used to describe a situation in which one object gets at the fields of another object in order to perform some sort of computation or make a decision, rather than asking the object to do the computation itself. As a trivial example, consider a class representing a rectangle. The user of the rectangle may need to know its area. The ...


85

There are several questions that you raise. 1) Is this a clear sign that the coder is not cut out for professional programming? No. Developers often go through stages where they learn about an idea and want to apply it. Do they always apply these ideas efficiently and/or effectively. No. Mistakes are made, and it is part of the learning process. If ...


84

I agree with your code reviewers, but with an asterisk. Each statement that you write in your code is a technical liability -- it's a potential failure point. If you write a method with 10 statements and your coworker writes one that achieves the same functionality with 5 statements, his is likely to be 'better' as measured by likelihood of issues (there are ...


84

Since it wasn't handled before, it's out of scope for your effort. You or your colleague can ask your manager if it's worth the effort to cover this case.


81

Should junior programmers be involved as code reviewers in the projects of senior programmers? Yes they should. It is a good learning experience to read other peoples' code. (And that applies both for good code and bad. Though one would hope that a senior developer's code wouldn't be bad ...) Obviously, it is unwise to only have juniors doing the code ...


76

There are multiple reasons why you would want to conduct a code review: Education of other developers. Ensure that everyone sees the modification associated with a defect fix or enhancement so that they can understand the rest of the software. This is especially useful when people are working on components that need to be integrated or on complex systems ...


75

The problem with measurements, no matter how well intended they are, is the very act of measuring the item makes it important, and the corollary, the act of not measuring an item makes it unimportant. It is absolutely essential to measure what is important, and not measure what is unimportant. Measuring SLOC (Which is effectively what your reviews are ...


75

The problem with this approach is that while exceptions never get thrown (and thus, the application never crashes due to uncaught exceptions), the results returned are not necessarily correct, and the user may never know that there is a problem with the data (or what that problem is and how to correct it). In order for the results to be correct and ...


70

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


70

I'm going to offer a different take from my fellow answerers. They are right - be involved if you want to see how things go. If you want more tracability, there are tools for that. But in my experience, I suspect that there's something else going on. Have you considered that your team may feel that the process is broken/stupid/ineffective for most commits? ...


69

What do you say in a code review when the other person built an over complicated solution? You say: "you built an overly complicated solution." So then I suggest that he delete all the unnecessary complexity. The answer I usually get is "well it's already done." If it's too late to change anything, why are you doing a code review?


Only top voted, non community-wiki answers of a minimum length are eligible