Amen to that.
Also, "I can't use someone else's library. It doesn't use my stylistic conventions for naming things, I don't know where it's been, and it'll take longer to find the bugs in it than to write my own from scratch."
cheers, Rich.
-- rich walker | Shadow Robot Company | rw@shadow.org.uk technical director 251 Liverpool Road | need a Hand? London N1 1LX | +UK 20 7700 2487 www.shadow.org.uk/products/newhand.shtml
I rather glossed over that point didn't I ?
I really was referring to syntax checking in this case though. That portion of analysis that each does (if for different purposes) to confirm the code is valid and catches the simple typos (extra closing brackets, dropped ; etc..). Some compilers do a truly lousy job of identifying the original error. Lint (PC Lint at least) is as good as the best compilers I've worked with at that identification.
Lint has many more important strengths (especially given good compilers) but it's a very practical advantage with compilers that have had little thought given to their error detection.
Robert
It's true that reviewing the code can be good. I see two reasons this:
"martin griffith" skrev i meddelandet news: snipped-for-privacy@4ax.com...
In my experience (and I have a *lot* of experience with code reviews), not improving the quality of the code is a symptom of an improperly-run code review.
Here are some ways a code review can go astray:
It can morph into a design review. The reviewer must avoid redesigning the program. There is a time and a place for that (requirements and initial design phases) and this is not the time or the place. Code reviews morphing into design reviews are especially likely when the reviewer has a much higher skill level than the programmer.
It can morph into a teaching session. This happens when the reviewer has a much lower skill level than the programmer.
It can morph into a management tool with the goal of employee evaluation rather than product improvement. Having the boss sit in or (worse) be the reviewer causes this, as does having a reviewer who is the company snitch.
It can be too early. If the project is late, there is a temptation to schedule the code review before the code is done, and to use that as the deadline - and as a club to beat the programmer with to make him work faster. That's the job of the ship date, not the code review date.
It can be too late. There is little point in having a code review if you know that you will be saying "that's a real problem, but we don't have time to fix it."
The reviewer may come unprepared. This is not the time or the place for him to learn what the product does or what the customer requires, yet he has little motivation to take time from his own projects to study a requirements document for someone else's project.
It can become a pissing contest. The reviewer may prefer a different methodology but he is there to help the programmer, not to get into a fight with him.
The programmer may see it as a threat and resist the process. The problem is that he is often right and it really is a threat. I always tell the programmer and reviewer that "what happens in the code review room stays in the code review room." I also get the buy-in of the programmer or I simply refuse to have a code review.
It can become a committee meeting. The best number of participants is three: programmer, reviewer, moderator. It is acceptable to let a junior member of the staff sit in and write down action items, but he should not participate.
If you avoid the above pitfalls, code reviews can do more to improve the quality of the code than any other one-day activity can.
-- Guy Macon
Can't say I know the book :>)
Still, I can't imagine a technique to review 20.000 - 25.000 line of (commented) C code in 4 hours. We found out the hard way that the review process wasn't good enough. During integration testing we found lots of errors that should have been detected in the reviews.
But my main point was, that reviewing has to be taken into account during the planning stage, and not just a token to please management.
Regards, Hans Bus
Absolutely correct. Code reviews must be run properly to be worthwhile in terms of quality improvement for the system and its software.
The primiary purpose of a review is to identify and list the problematic issues within the design. No reccommendations for adopting alternative design strategies may be made as part of the review comments. Just what the problem is and how it is exhibited or perceived to be exhibited.
Selection of appropriate reviewers is necessary. However, using members of the same team, members of other similar teams or even an outside entity (person or company) that has been breifed on the requirements would also suit.
Technical reviews are not for the boss to attend (unless you can trust him to wear just his technical hat).
The policy I have is to review early and review often. However, the number of lines in many of the functions I am writing are small by comparison to many who post here (my granularity for a subroutine tends to average less than 16 lines including the comments and statement of functional requirements - then I use Forth quite a lot). At each review I will have a number of units of code ready to be reviewed. Once the code has passed the review it is subjected to the testing phases. Pass that and it enters the trusted library.
I know of companies that leave their reviews until they have completed at least 80% of the coding. After several days of review meeting they discover that they have really huge problems.
If the review material has been pre-posted to the reviewers they should have prepared. So, question one of the review is of the preparedness of the review team. If anyone is not prepared then reschedule the meeting but reprimand the unprepared member (he buys everyone pizza for the next meeting out of his pocket).
Agreed.
The programmer whose code is being reviewed has to be present in the room but his main job is to listen to the views of the others and to answer specific questions directed at him for the purpose of clarification only.
Minimum of three maximum of seven I think is the rule. All reviewers should have knowledge of the domain being dealt with (not necessarily all from programming). Meeting posts are Reader, Moderator, Recorder, Reviewer, Developer (programmer or engineer whose work is under review). More than one post can be filled be filled by the same person but be careful with such job sharing.
True, so true. See my book reccomendations (Freedman and Weinberg) mad a few days ago for the proper way to organise reviews. It is all good advice.
-- ******************************************************************** Paul E. Bennett .................... Forth based HIDECS Consultancy ..... Mob: +44 (0)7811-639972 .........NOW AVAILABLE:- HIDECS COURSE...... Tel: +44 (0)1235-811095 .... see http://www.feabhas.com for details. Going Forth Safely ..... EBA. www.electric-boat-association.org.uk.. ********************************************************************
That is just too much of a big chunk all at once. Wasn't this code in a series of smaller sub-routines? The main bulk of the review work is actually conducted outside of the meeting room. Pre-circulate the material to be reviewed (the moderators) and the meeting collects the list of problems areas to be revisited by the developer.
Agreed. It should be part of the scheduled time and significantly so. No review should meeting should be longer than 2 hours. Many of mine take less than an hour but I do have a lot of them.
-- ******************************************************************** Paul E. Bennett .................... Forth based HIDECS Consultancy ..... Mob: +44 (0)7811-639972 .........NOW AVAILABLE:- HIDECS COURSE...... Tel: +44 (0)1235-811095 .... see http://www.feabhas.com for details. Going Forth Safely ..... EBA. www.electric-boat-association.org.uk.. ********************************************************************
"Guy Macon" skrev i meddelandet news: snipped-for-privacy@corp.supernews.com...
Sure, if you don't know what you are doing you always have a good chance of failing.
What I question is the value of the code reviews (given that you know how to do it properly). Code reviews will improve the quality of the code, no question about that, and most projects should have them. But how much will they improve the code? Not that much I say. In many of the projects I have worked in it would have been better to spend less time on code reviews and more time on design and testing. These projects did have a good design and good test cases but still I think too much time was spent on code reviews.
Not that it was my comment originally but I prefer to find out about problems as early as possible. There can be enormous improvements made with the right approach and iverview.
There are sometimes elements of design going on during the coding period concurrent with the coding effort. This may be for a number of reasons and designs are never fully done and dusted until the implementation is complete. I know it would be nice if the design phase could be fully completed before coding starts but that is not always possible. This, however, is not a problem with decent configuration and change management practices being employed.
I do see, though, that you probably understood my drift from the comments you made and your list of likely issues is a reasonable starting point.
I also noted that seemed to be the general leaning of the group but I still maintain that it is no excuse for not having a review. If you are a one man outfit you need to hire or aquire the help you need. I expect that many one man businesses hire in an accountant to assist with getting the books to atax-department acceptable standard (I know I always did). So it is the same that you obtain the assistance of someone else to do the review with you. A simple NDA covers the issue of IP that may be in the material.
I guess you haven't been working with outfits where the boss is also one of the programmers. As I said, he has to promise to wear only his technical hat in the review meeting and everyone else will remind him if he steps out of that mode. Otherwise, if you can keep the boss out that will be to the better.
....and may that evolutionary process be continue to be accellerated!!!.
My software education has never been formalised. I did my apprenticeship in Electronic systems engineering and learnt the software side when we started using microcontrollers for various things. My first programming job was actually performed by placing diodes in the correct place in a matrix. That was back in 1970. I was also let loose on General Automation based systems (hardware maintenance (but we had to write our own test routines to check the kit out). My first microprocessor was the 6800 which I hand crafted the machine code for an application that only ran issue 1 for 25 years with zero errors (4kbytes of hand entered code). To me, whether it is the hardware or the software, it is all just good engineering practice.
We shall try and push your buttons again sometime then ;>
-- ******************************************************************** Paul E. Bennett .................... Forth based HIDECS Consultancy ..... Mob: +44 (0)7811-639972 .........NOW AVAILABLE:- HIDECS COURSE...... Tel: +44 (0)1235-811095 .... see http://www.feabhas.com for details. Going Forth Safely ..... EBA. www.electric-boat-association.org.uk.. ********************************************************************
Paul E. Bennett wrote:
I second that....First step is to educate the participants about what a code review is and does...etc... Also, not sure how to interpret 'improve the quality of code..much'...Where would you prefer to find problems? coding or integration (or worse yet, fielding?)? Code inspections are static and should focus on issues that can be uncovered in a static setting, leaving those nasty concurrency issues to be caught at integration time. Allows the focus of integration testing to be on dynamic issues (concurrency, latency, performance, etc...)
"...issues with design"? Systems already has been designed...code is implementation (Unless your one of those folks who practice voodoo software development from the 60s.). Code reviews evaluate the code itself (although it may kick up issues with design that should be used to clarify the coders understanding of it or, kick the problem to another level). Depending on the company/department/group, the objectives of code reviews fall into the following categories: conformity (to a coding standard/style), correct translation of specs to functionality, language specific nuances (i.e. ptr boundary checking in C, etc.)that can cause a failure of the system. A failure is defined as the deviation of service being provided as compared to the intended (e.g. specified) service. As an aside, the thinking that one does not do code inspections unless 'its safety related software' is exactly the thinking that causes billions of dollars of lost time and data in just about all software project. Don't know about you, but I go ballistic every time Windoz decides to do a GPF when I am writing a doc in Word. Then again, it might be safety related if it keeps my from putting my fist through the monitor.....and it costs me tons of time to reload an OS or an application, and it costs my employer money for me to do this. So you (collectively) think code inspections don't apply to one person operations? Take a look at publications by Watts Humphries about the Personal Software Process...they are techniques and guidelines that help individuals develop a (personal) sw dev process to improve their coding. One of the techniques is self-reviews...hmmmm don't got time to do self reviews? How much time ya got to fix problems later on? Sorry...I digress...
I guess I am a bit stronger on that point...I'd say never, no matter what size the organization. To my way of thinking, a CR produces no usable metrics for project management, other than 'coding of module xxx is complete." If having to go through rework again and again, thats a sign of other problems that should be handled by a process team/person to minimize those reworks. That involves keeping track of the types of nonconformities that cause failures in code reviews which can be presented to the supervisor without 'finger pointing' but in a way to motivate improvement.
I's say that company won't be in business very long given their current rate.
Problem is, managers, thinking that they are the only ones incapable of making mistakes, look for 'measures' to evaluate a coders performance. I've seen too many cases where code review results are used like a 'test' to grade a programmer. Can you say 'pointy haired manager'??? Then again, this is one of the signs of a truly bad organization that probably won't be in existence very long, and thats a good thing...
One of the functions is a 'process' observer to keep things on the straight and narrow so it does not turn into any one of the problems cited above (committee meeting, bash the developer, tutor the developer, etc...)
There are tons of literature about the good effects (and side effects) of code reviews. Problem is most electrical and computer engineers (and some computer science programs) have taken 2-4 'software' courses where they learn programming languages, structure, and environments and then go off and write code for their embedded robotics controllers (or whatever) to get a good grade (or maybe not) and get the degree. That
*may* (but I have some problems with that too) be OK, but unfortunately they aren't exposed to good sw development processes and practices in their training (particularly true of traditional engineering students), so when they hit the real world, they aren't adequately prepared for the way to do things better in a more global setting. But things are slowly changing. Oh well, sorry for the rant...I saw a few things that pushed my buttons and had to respond... John
Point well taken. What I was getting at with my comment about
3 or 4 being ideal was the undesirable "one developer, five reviewers" situation. 3-7 would work well with the division of labor you list.Needless to say, if there are seven people in the room, any splitting off into side conversations must be suppressed with an iron fist.
Post you replied to: Tue, 23 Nov 2004 23:18:24 +0000
:)
My experience is completely opposite. The reason, I think, is because we have reused a lot of code, and same code again and again for several years.
Obviously I have no clue how much time you did spent in code reviews. But still I have never ever seen a single properly commented code. And I do not think I will ever see.
-- @jhol Jotkut pitävät armeijaa parhaana järjestelmänä joissakin tapauksissa. He ovat oikeassa, armeija on paras järjestelmä Helvetin jälkeen.
I prefer to find the problems myself and I do this by testing the code before integration.
Writing test code that tests the greater part of the code you have written will find much more problems than you can ever do by code reviews. Actually, I see testcode as a form of code review for finding problems in the code. The tests can be made automatic so you can test the whole product once every day i.e. a code review every day of almost all code. Of course testcode will not find ten levels of nested if-statements so therefore I use code reviews to find problems with the structure of the code (and not so much to find real errors because they have already been found in the testing).
Just to clarify; by testing I mean unit testing, module testing or wathever you call it. Not integration testing.
That's what I mean, for the "main bulk of the review", we had 4 hours to do it in. We had special tools to prepare the material, so each reviewer got the same prepared package. Reviewing was done page by page, collecting and noting comments an a page basis. DIfferent codes for different "errors" etc. ranging from "fatal" (MUST change) to "nice to see" (at the discretion of author). Too many fatals meant a new review, otherwise a "check updates according to review notes" by
2 or 3 selected reviewers. Everything was archived under change control (we were ISO-9001 certified:>) ).The meeting normally took about 2 hours.
The one aspect I haven't seen specifically mentioned in this thread is the human one of assigned reviewers not having or taking the time to properly prepare for a review. If the review is an added task for an otherwise fully-occupied reviewer, review preparation may become a lower priority and may get omitted if other work is more pressing. This can be even more so if the reviewer's regular work is perceived to be where success/failure is determined and the review work will be mentioned in passing if at all.
I will mention in passing a technique I once saw used for spec review. A copy of the spec was fastened to the walls of a room and everyone was supposed to make a trip to the room and make notes on the posted copy. This was supposed to be a more efficient way to gather everyone's comments. I don't know about others, but I'm not going to do nearly as thorough a review if I have to stand on my feet for extended periods to do it. [Requests for personal copies of the spec were refused.]
Perhaps you missed this:
| From: Guy Macon | Newsgroups: comp.arch.embedded | Subject: Re: Code reviews? | Date: Mon, 22 Nov 2004 21:23:48 +0000 | Message-ID: | | Here are some ways a code review can go astray: | | The reviewer may come unprepared. This is not the time or the | place for him to learn what the product does or what the customer | requires, yet he has little motivation to take time from his own | projects to study a requirements document for someone else's project.
-- Guy Macon
Always a good idea...this is the dynamic counterpart to CRs (static) However unit testing will likely NOT uncover integration related problems..i.e. assumptions made about services running/available, shared variable out-of-order (i.e. read before write-stale data) problems...etc.
There are many published articles that put forth contrary experience. When I find my list I'll post it..
Thanks. I would appreciate that. I think most people (including me) that have a strong opinion about code reviews are baseing their opinion on a gut feeling and not actual measurements. If someone can show me that a lot of code reviews will make the software cheaper (in long term) I´m willing to try to convince my intestines.
Rule Number one. BE SURE what you're reviewing!!!
Horror Story: Did a project to convert an FPGA to an ASIC. Did design reviews out the wazoo. ASIC came out EXACTLY like the spec. Only problem was that somebody forgot to roll the version at the last FPGA update. So the timing mod to compensate for the bad display drivers didn't get included. Classic example of GIGO. $17K down the drain. mike
-- Return address is VALID. 500MHz Tek DSOscilloscope TDS540 $2200 http://nm7u.tripod.com/homepage/te.html Wanted, 12.1" LCD for Gateway Solo 5300. Samsung LT121SU-121 Bunch of stuff For Sale and Wanted at the link below. http://www.geocities.com/SiliconValley/Monitor/4710/
ElectronDepot website is not affiliated with any of the manufacturers or service providers discussed here. All logos and trade names are the property of their respective owners.