Tag Info

Hot answers tagged

267

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.


173

Code with spelling and grammar errors is unmaintainable. People won't remember the bad grammar, so they'll try to call the function as it should have been written, and that's how bugs happen. You can't grep for something in the code if you don't know how it's spelled. Most people who make grammar/spellings do so inconsistently, so they'll introduce many ...


163

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


145

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


137

I've found that people who don't like code reviews will do their best to work around whatever you put in place. The best way to make sure that the code you work with is code reviewed properly is to work somewhere that treats that as the normal way of coding, and that only hires developers who are likely to fit into that environment well. If you can't ...


103

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


101

I personally think that every piece of code should go through a code review, it doesn't matter if you are junior or senior developer. Why? For starters your title doesn't state anything about how you develop, and a senior developer could learn something from the junior. At our company we shift around so one of the other members of the team review your ...


77

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


73

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


73

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


65

Don't take pride in how well you code. Take pride in how well you learn. Then learning that your code needs improvement provides you with an opportunity to demonstrate how good you are at learning, instead of coming across as criticism of how bad a programmer you are. Read http://www.perlmonks.org/?node_id=270083 if you have no idea what I'm talking ...


62

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


61

Unless you will be making changes to this "old code" to fix bugs or add features, I wouldn't bother improving it just for the sake of it. If you do want to eventually improve it, make sure you have unit tests in place that will test the code before you start refactoring it. You can use the "old code" to learn better practices. If you do this, it's a ...


59

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


54

This seems to be a pretty common prevailing attitude among some developers. Everyone seems to feel that a code review is some challenge to their work, and that makes no sense to me. A code review is a quality assurance mechanism that has the added bonus of education to go along with it. We implement code reviews extensively where I work, and I've fostered ...


51

The primary purpose of a code review is to find defects or potential problems. The required participants in the review should be the people who are best suited to identify these problems, regardless of their title or seniority. As an example, if an application is being developed in Python and the junior engineer has more experience with the Python language ...


50

Code reviews are a great practice. It is probably the best way to learn from mistakes and to see how certain problems are solved by others. It is also one of the best way to maintain quality in a code base. Code reviews happen in many companies, though it is difficult to say that there is a specific process that they all follow. In more formal code ...


47

Like Simon Whitehead mentions in his comment, it depends on your branching strategy. If the developers have their own private branch for development (which I'd recommend in most situations anyway), I'd perform the code review prior to merging with the trunk or main repository. This will allow developers to have the freedom to check in as frequently as they ...


46

Since it's not clear from your question, I just want to point out that a gatekeeper workflow is by no means required with git. It's popular with open source projects because of the large number of untrusted contributors, but doesn't make as much sense within an organization. You have the option to give everyone push access if you want. What people are ...


46

It's important to highlight positives as well as negatives. I know if I were reviewing the refactor of a particular hellish subsystem into something neat and clean, I'd probably buy the programmer a pizza for his efforts. If you're using reviews as training, it's doubly important - highlighting a good piece of code will be helpful for the junior programmers ...


45

Bob Martin told this story at Agile 2008 (and it might be in his Clean Code: A Handbook of Agile Software Craftsmanship book). His example was making dinner. The easiest way to make dinner is to never clean up afterwards. You just leave the dishes unwashed. Next time you want to make dinner, you just find some new dishes and repeat. Pretty soon the ...


39

A formal code review could go like this: Programmer sends material to reviewer Reviewer goes over material Programmer and reviewer sit down go through the material. Programmer takes notes. Design discussion are postponed. The programmer corrects whatever came out of the review. The programmer should always talk to the reviewers, otherwise way too much ...


39

Improve Quality and Morale Using Peer Code Reviews http://www.slideshare.net/SmartBear_Software/improve-quality-and-morale-using-peer-code-reviews Things Everyone Should Do: Code Review http://scientopia.org/blogs/goodmath/2011/07/06/things-everyone-should-do-code-review/ Both of these articles state that one of the purposes of code review is to share ...


39

Write down your reasons for rejection and defenses for likely counter arguments. Then discuss rationally with the fixer and if necessary escalate to management. If you have sufficient documentation (including code listings, test results, and objective arguments), you'll be well covered. You will cause some ill will with the fixer, so be sure the issue is ...


38

Imagine you are designing an aircraft. You've got a single engine jet. It sells well. Now you are going to design a 4 engine aircraft for long hauls across the ocean. Now, you don't create a full set of engineering specifications and drawings for each individual engine, do you? No, you use the same engine in all four places. Now imagine if you had 4 sets ...


38

After 20+ years I know that some of my own code is still a mess. What it comes down to is making a decision between writing the best possible code versus writing what's required to get the job done. Getting the job done within an agreed timeframe trumps the never-ending quest for technical perfection any day. The trick is to learn to accept it. Learn to ...


37

"Working fine" is indeed a great metric, but if you are the only one in the team able to decipher what you wrote, and thus maintain it, the code is close to worthless for the company for the mid or long-term. A good code is at least : working as intended human-readable / clear easily maintainable easily extensible for future changes safe without unneeded ...


36

I have worked at a job where check-ins were limited to team leads only (and team leads couldn't check in their own code). This served as our mechanism to enforce code reviews, largely because of a number of incidents where bad commits got into the codebase even around gated check-ins and static analysis. On one hand, it did it's job. A number of bad commits ...


35

Yes definitely. It's easier to remember the name if it's grammatically correct. Trying to remember the name and grammar mistakes is another thing entirely.


35

It's like this... you have one clock in your house. Great! You know what time it is, but you always have to go to that one room to look at it. But of course you want to know what time it is without going to that room all the time, so you buy some more clocks, and you distribute them around your house. Each of these clocks are independent. They all keep ...



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