I've been coding for 25 years, primarily as a consultant. I've been in exactly one shop that tried to do code reviews, one of which I actually know happened.
I think they are a great idea, but has anyone worked in a shop that has the discipline to actually implement them?
Discussion on:
View:
Show:
where they were going to be used, loads of places where they chould have been used and even more where they should have been used. In the real world however options for writing quality code get thrown straight out of the airlock in favour of quantity code. Personally I try to stick to a coding standard all the time, a common one, or achieving it across a legacy code base or even building it in to a multiple developer greenfield project. No management buy in terms of resource and criticality, don't waste your breath for more than a ten minute meeting for a few do's and don'ts.
I constantly review my code, since I'm always learning and ( hopefully ) improving my skills the review can often allow for significant improvment in functionality.
since I code for myself, I'm a shop that does implement code reviews.
since I code for myself, I'm a shop that does implement code reviews.
I use to sit with Code Review Process before each milestone delivery, to avoid conceptual or procedureal troubles in application, but this happened when i worked for ERP Application development. Now as a web application project manager, i trained my team to do a self test code review process before sending the application to QC dept
The article addresses how a code review could be done (an automated tool), but fails to address the why.
The only apparent justification in "Enforce development standards with code reviews" is "standards should prevent each developer from using their own preferred style". Frankly, I've seen far too much time wasted debating the proper placement of braces or the correct way to indent code to have any desire to enforce such standards. I have worked on fairly large programs where a vast mix of styles were employed and have found a very minimal set of things are necessary to promote the readability of software. The key issues are method size, organization, and naming and I do not know of any automated tool that will help with any of them.
Before I would buy an automated tool, I would require a lot more justification than presented in this article.
The only apparent justification in "Enforce development standards with code reviews" is "standards should prevent each developer from using their own preferred style". Frankly, I've seen far too much time wasted debating the proper placement of braces or the correct way to indent code to have any desire to enforce such standards. I have worked on fairly large programs where a vast mix of styles were employed and have found a very minimal set of things are necessary to promote the readability of software. The key issues are method size, organization, and naming and I do not know of any automated tool that will help with any of them.
Before I would buy an automated tool, I would require a lot more justification than presented in this article.
People talk about the number of spaces to indent. I just figure they forgot to take their butt plug out.
Coding standards to me is all methods that construct an instance should have create in their name. Or all form methods that look up the information from a store to display are called WriteDataToForm or something equally meaningful. Coding standards should n't be how to write the code as such, given you haven't been a complete knobhead. What they should do is give another coder or yourself some time later a bit of confidence of how the code is layed out so they can find the bit they need in order to debug/enhance/refactor.
Annoying things such as poor names, 35 page functions that do 50 things, doing a common task a completely different way to confuse the crap out of someone are far more important than whether your opening and closing braces line up. Though the latter does help.
This software is just a prettifier to me.
Coding standards to me is all methods that construct an instance should have create in their name. Or all form methods that look up the information from a store to display are called WriteDataToForm or something equally meaningful. Coding standards should n't be how to write the code as such, given you haven't been a complete knobhead. What they should do is give another coder or yourself some time later a bit of confidence of how the code is layed out so they can find the bit they need in order to debug/enhance/refactor.
Annoying things such as poor names, 35 page functions that do 50 things, doing a common task a completely different way to confuse the crap out of someone are far more important than whether your opening and closing braces line up. Though the latter does help.
This software is just a prettifier to me.
a good standard for coding is in your opinion..
for me it's that:
usefull comments are used appropriately in the code,
that operator overloading be used as little as possible
that variable names are descriptive and logical for their use. [ single character variable names for loop variables, otherwise something more obvious ]
wrap lines at 80 characters
use spaces rather than tabs for indenting, as they will display the same for all
other than that I don't care where you place braces, how many spaces you indent, just as long as both are consistent.
for me it's that:
usefull comments are used appropriately in the code,
that operator overloading be used as little as possible
that variable names are descriptive and logical for their use. [ single character variable names for loop variables, otherwise something more obvious ]
wrap lines at 80 characters
use spaces rather than tabs for indenting, as they will display the same for all
other than that I don't care where you place braces, how many spaces you indent, just as long as both are consistent.
Also a standard way of doing things . If you're doing three tier. A class the same name as the table for the record, an aggregation (pluralised) for a set. ReadFromDB, WriteToDB. Just simply things like that can save you a heck of a lot of time.
If If Else (no bracketting) very compiler dependant
Some others are particularly useful in drag and drop IDEs
No With statements, especially no nested or compound ones
If then else all on one line, put a debug on it and you never get the else.
Comments should be why, perhaps what if in business jargon, never how. If you need to comment that your naming of variables nd procedures is crap.
They also should also be brief, very annoying when they interrupt the flow of the code.
Only one other rule I always stick to along with no ifs. The only type of method that contructs an object should be a constructor. I find this sort of thing really annoying.
Procedure GetListOfClients(aClientList : TClientList; aClientType : TClientType);
Begin
If Not Assigned(aClientList) Then
aClientList := TClientList.Create;
aClientList.ReadFromDBByClientType(aClientType);
End;
Obvious what it does when you take the lid off the box, but unless you already know what's inside and you are sure no one has changed it since you last looked, distinctly not obvious that you now have another instance of the class to manage and a good chance at leaking even more valuable memory.
If If Else (no bracketting) very compiler dependant
Some others are particularly useful in drag and drop IDEs
No With statements, especially no nested or compound ones
If then else all on one line, put a debug on it and you never get the else.
Comments should be why, perhaps what if in business jargon, never how. If you need to comment that your naming of variables nd procedures is crap.
They also should also be brief, very annoying when they interrupt the flow of the code.
Only one other rule I always stick to along with no ifs. The only type of method that contructs an object should be a constructor. I find this sort of thing really annoying.
Procedure GetListOfClients(aClientList : TClientList; aClientType : TClientType);
Begin
If Not Assigned(aClientList) Then
aClientList := TClientList.Create;
aClientList.ReadFromDBByClientType(aClientType);
End;
Obvious what it does when you take the lid off the box, but unless you already know what's inside and you are sure no one has changed it since you last looked, distinctly not obvious that you now have another instance of the class to manage and a good chance at leaking even more valuable memory.
Regarding Code Review, which we use within a medium sized development team for an established product.
If you are using a sourcecontrol system (VSS, PVCS etc) then reviewing the changes is more than purely comparing the "before and after" code. It is also confirming that the code meets the specification, that programmer-QA has been done and stored as a documented record of the fact.
The "whether braces go here or there" is something to be instilled as company practice, so the code is recognisable to any one of the team.
An important point is that of RESPONSIBILITY.
It is a given that occasionally people make coding mistakes, but one of the key aspects of CR is to sense-check any changes from a functionality viewpoint, PRIOR to them being checked-in to source control. By CR-ing a peers code change, you are sharing their responsibility that the code is correct and that it meets the business specification - not that of course we would have a witch-hunt following an error introduced - two pairs of eyes are better than one, and may consider an aspect of the job which has been missed by the coder.
If you are using a sourcecontrol system (VSS, PVCS etc) then reviewing the changes is more than purely comparing the "before and after" code. It is also confirming that the code meets the specification, that programmer-QA has been done and stored as a documented record of the fact.
The "whether braces go here or there" is something to be instilled as company practice, so the code is recognisable to any one of the team.
An important point is that of RESPONSIBILITY.
It is a given that occasionally people make coding mistakes, but one of the key aspects of CR is to sense-check any changes from a functionality viewpoint, PRIOR to them being checked-in to source control. By CR-ing a peers code change, you are sharing their responsibility that the code is correct and that it meets the business specification - not that of course we would have a witch-hunt following an error introduced - two pairs of eyes are better than one, and may consider an aspect of the job which has been missed by the coder.
Functional Review
Code Review
Two different animals that when you get damn lucky are walking fairly close to each other at teh same time in teh same direction.
Code Review
Two different animals that when you get damn lucky are walking fairly close to each other at teh same time in teh same direction.
lots of different purposes in ding code review.
does it meet the specification for the project
is it readable
is it secure
what has changed in it since last review
[ changelogs are helpfull for this ]
are there conflicts in this part of the project with any other part of the project
every single thing you need to verify before release is a code review. by targeting specific subjects during a review you are more likely to find peroblems relating to that subject. [ like checking for security issues only, forget all other areas and make sure that it is secure when being used ]
does it meet the specification for the project
is it readable
is it secure
what has changed in it since last review
[ changelogs are helpfull for this ]
are there conflicts in this part of the project with any other part of the project
every single thing you need to verify before release is a code review. by targeting specific subjects during a review you are more likely to find peroblems relating to that subject. [ like checking for security issues only, forget all other areas and make sure that it is secure when being used ]
- Keyboard Shortcuts:
- Prev
- Next
- Toggle
