Archived

This topic is now archived and is closed to further replies.

mordell

Code Reviews, A Call to Action

Recommended Posts

This is a little diatribe on how code reviews can help, not hurt. This draws heavily on the article A Case for Code Review . This is a real case scenario that happend recently and I am attempting to clue my manager in that code reviews are a good thing and don't necessarily waste time.. I was wondering if I could get any feedback: During the last several months of their development cycle we find that Tom wrote a CSV class, Dick wrote a CSV class, and Harry wrote a CSV class. How did it come that these three all spent time doing the same thing? Lets see… [No Code Review: The CSV Scenario] Tom writes a cool CSV class. It works well, is a little awkward to work with, but it gets the job done admirably. Tom begins to use this code in a production environment. Dick is given the task of wrapping Tom’s CSV class into a COM object that could be used server side. This also means that we get binary reuse from Tom’s code in clients such as Visual Basic and Visual Basic Script, in addition to C++. Dick realizes that he has an issue with Tom’s implementation. Tom seemingly used an additional class library to develop the CSV code (the code is not independent), and Dick’s server objects can’t use any additional libraries. Static linking is not really an option. Dick also finds that Tom’s CSV class isn’t as intuitive as it could be and feels clumsy using it. In the interest of progress, Dick decides to use his own CSV code to drive the wrapper and begins using that religiously (Dick was in serious error here. What he should have done was gone to Tom and asked him to make the necessary modifications to his CSV class to remove the additional dependency). Harry, not knowing that either of Tom or Dick had visited this particular problem, also wrote his own CSV code. Then, he dropped it in favor of using the Microsoft ADO CSV driver, which has a very consistent look and feel and is ready made (no development time). That means (for those of you keeping count!) that in the last couple of months, that CSV code has been re-visited THREE different times by THREE different individuals resulting in THREE different implementations. [Code Review: The CSV Scenario] Tom begins developing the CSV code. He starts with a class outline that essentially defines the responsibilities of the class and how it would be used. This CSV code is probably implemented as a simple class with methods to open/close/read/write fields in a CSV file. Maybe even it defines a custom delimiter so we aren't left high and dry with just the ability to decode comma-delimited files. Pretty cut and dry. He offers it up for review. Tom, Dick and Harry all verify that it appears to behave as it was defined, maybe Dick and Harry offer suggestions to how they would interface with the class (ease of use) and be sure that everything was commented adequately. Now, during this discussion they figure out that they need to add additional responsibilities: namely the ability to query the CSV file and return a result set. This means that it would probably be beneficial to further enhance the design by offering a Row(s) class, which is nothing more than a collection of the fields that make up that row and some method to iterate them. It comes up that one of the intended uses of this code is in server side logic. This allows them to quantify how server side code should be crafted. They also figure "Hey, wouldn't it be great to be able to use this code in other environments? Like VB or VBScript?". Now the decision is made that the code needs to be distributed as a COM object, which entails further discussion on how the COM objects published interface will look. Sometime in the discussion, it comes up that Microsoft distributes an ADO CSV driver. The CSV driver is implemented as a COM object, and would work in each of the required environments. Further discussion ensues on the merits of using ADO or their own CSV code. A decision is made Tom either continues his progress on the CSV code or they opt to use the ADO CSV driver. The plus side being that every member of the team knows the tool(s) that are available to them, and when they should use one over the other. They all meet at the same bat time, the same bat channel next week, or at the next coding milestone for another review. Edited by - mordell on 4/3/00 11:35:21 AM

Share this post


Link to post
Share on other sites
*gives standing ovation*

What an article! Code Reviews are kind of like posting to the GDNet message boards in fast-motion, and with less ego

The second case of your story is definitely better, IMO. Not only did they learn how a CSV class works, but they also shared ideas. Although they ended up using an already-built CSV in both cases, that is certainly not always the case (especially in game programming). The general equation is:


group_experience += (member1_experience ^ member2_experience ...)


(I''m using the exponent operator here.)


Or, perhaps an even more realistic equation would be as follows:


group_experience += ((member1_experience ^ member2_experience ...) - (member1_ego ^ member2_ego ...));



I feel that inflated egos are a team''s worst nightmare. But there''s something more important than just ego. Ego affects a team most because it affects communication. Code Reviews seem to be an excellent source of good communication. Anything that increases the amount of good communcation in a team increases the team''s chance for success. Anyway, that''s enough of my opinions





- null_pointer
Sabre Multimedia

Share this post


Link to post
Share on other sites
I guess there is always the chance of bruising an ego...and to be honest, I used to NOT be open to having a group of people picking through my code. It always seemed so invasive..kinda like a physical exam.

But, after reading that article on gamasutra, I have been in high gear trying to get some sort of code review process implemented on our team.

I think that the biggest draw back in our scenario is that every one is always soo busy that they will automatically try to shootdown anything that might take away from development time.

Anyways...for those of you who haven''t read the article, check it out.

Share this post


Link to post
Share on other sites
quote:

I think that the biggest draw back in our scenario is that every one is always soo busy that they will automatically try to shootdown anything that might take away from development time.



That is the whole point, it saves time in the long run. Though I suppose many people are only able to see the short term loss that will happen instead of the overall savings in time that could (and probably will) occur.

Why wouldn''t people want other people to fix bugs instead of having to hunt them down themselves? I hate hunting down bugs.

bcj

Share this post


Link to post
Share on other sites
I really don''t like people seeing my code; the urge to just copy a little is there. If someone is reviewing my code, and he sees something he likes (which happens often in my code ), he might go, "Well, I''ll just copy a little". Then somewhere else, same thing. And the truth is, once they see a way of doings things that they like, they''re gonna copy it almost regardless; why waste time making your own when the better way to do it is right it front of you?
I know this is a worse case scenario, but admit it: if you see something you like, you do have maybe the slightest urge to just "take a little". I admit it happens to me, and the urge is there, but I don''t steal code. It just doesn''t seem fair (also, most peoples coding styles is way different than mine, since mines the best anyway ).

Share this post


Link to post
Share on other sites
Zipster you''re missing the point of a code review. I suggest you read the gamasutra article. The whole point of it is to learn from others work and to make sure you know whats going on in their code so if they ever leave someone else can pick up where they were without wasting time trying to figure out what their code does. It also helps kill bugs and bad design before it starts.

Josh

Share this post


Link to post
Share on other sites
Zipster, do you know how arrogant that makes you sound?

Code reviews are a great thing. No matter how good you think you are, someone will think of something better, or something different that you may not have thought about.

You definately do not have the attitude of a good team member. I for one wouldn''t want to work with you. Team members share information, they don''t horde it.

As for stealing code... Do you use DirectX? Win32 API? I guarantee that you didn''t learn the Win32 API or DirectX without "stealing" examples other people wrote. Oh no! You''re not the coding god you think you are! You thief!!! If you are the one person who learns everything from only the documentation (with no code snippets), then I apologize.

<< most peoples coding styles is way different than mine, since mines the best anyway >>

::rolls eyes::


Books to check out:
Game Architecture and Design
Code Complete


Josh
http://www.jh-software.com

Share this post


Link to post
Share on other sites
quote:
As for stealing code... Do you use DirectX? Win32 API? I guarantee that you didn''t learn the Win32 API or DirectX without "stealing" examples other people wrote. Oh no! You''re not the coding god you think you are! You thief!!! If you are the one person who learns everything from only the documentation (with no code snippets), then I apologize.


Do you know how rude that makes you sound?

I''m giving my honest opinion (not harming anyone), but assholes like you can''t wait to pounch on someone that doesn''t agree with you!! Oh yeah!! YOU''RE always right now, huh? YOU''RE way is ALWAYS the best way, huh?!?! Because YOU think sharing code is the best way, EVERYONE should, or they''re arrogant!?!?!!? If you could of just told me I misunderstand the principle and tell me to read the article, then fine, but instead you had to be rude. Its people like you I WOULDN''T share code with, if all I got "constructive criticism" like yours!

Next time, I suggest you straighten out your attitude before you attack innocent posters.

Share this post


Link to post
Share on other sites
ahem...now that i calmed down, i would like to say something:
quote:
This forum isn''t about flaming each other and calling each other arrogent (ahem), its about helping others, and imforming them of their misunderstandings.


I would like to apoligize for being rude in my last post, Josh, as I don''t want to hold a grudge against anyone. But what i said in the last parahragh was 100%. if you don''t agree, say so, and maybe tell the person yor view: perswaysion (however you spell it ).
as for not sharing code, i didn''t clarify, and thats my mistake . What i meant was that i don''t like sharing code with those NOT INVOLVED on the project. If im on a team, well then no shit id have to share code if i wanted to stay on a team.

I just hope we can all be friends and not jump to conclusions and be rude.

Share this post


Link to post
Share on other sites
zipper, youre still missing the point by a large margin, its not about HAVING to show your code, its about everybody learning and benefitting in the process of sharing ideas and skills.
Look at the formulae for experience (* NULL) derived above. As you see even your experience(if not less than one) could contribute to overall with a lot.
Ever heard of open source software? What do you think in what state todays software would be if nobody ever would share their ideas ? Thats why this messageboard exists after all.
Better go over and read the article, and try understanding for a change.

-kertropp

C:\Projects\rg_clue\ph_opt.c(185) : error C3142: 'PushAll' :bad idea
C:\Projects\rg_clue\ph_opt.c(207) : error C324: 'TryCnt': missing point

Share this post


Link to post
Share on other sites
Yes, i know its about not HAVING to share, but sharing to be able to get work done faster and to find better ways to do things. I never meant to imply that one would have to, but i guess i did.

And if one more person offends me, im gonna be pissed. I said im sorry, and i don''t want any more mean messages towards me.

Share this post


Link to post
Share on other sites
Zipster:
First of all, I apologize. My first post was rather harsh. I was in a pretty sour mood when I wrote it.

However, I still stand behind what I said, I just should have reworded it a little better.

Anyone reading your first post will think that you are an arrogant programmer who thinks he can do everything and doesn''t want to help others. Not saying that you are, but that was my first impression.

You did not make it clear that you are against sharing code with people *not* involved with the project. Most teams don''t show code to people not involved. Code reviews are internal and the code stays within the company/team.


Josh
http://www.jh-software.com

Share this post


Link to post
Share on other sites


I just wanted to respond to Zipster. I can understand his reservation. Its a common reaction and he shouldn''t feel guilty for it.

But, there is a big difference between sharing code that you slaved over for your own projects and code that you slaved over that other people rely on to get their jobs done.

That "urge" to copy? I would encourage that of any team members that found any code I had written to be worthwhile. I would much rather spend quality time writing quality software than wasting time re-writing everything.

However you look at it, if you are getting paid to write software, the code you write isn''t yours (unless you pay your own salary). You may have written it, but you don''t "own" it and it is to everyones benefit to offer it up for review. Thats one of the issues the article discusses, losing that "its mine" mentality (not to be confused with responsibility!).

Some benefits include:

- establish team communication
- polished design
- maintain code integrity
- introduction of new team members to code base
- team members are aware of each others projects

bcj:

Unfortunately, our team has had such a closed box mentality for so long it will be hard to break some habits.

For instance, a "lead" on one project is so swamped with enhancements he has to make on an older project, he isn''t making any progress on a new project.

He justifies this by stating "I can still get it done faster than anyone else could do it". He is missing the point, that he needs to be sharing that information that would allow someone else to get up to speed and take over the older project so that he wouldn''t have to revisit it again and could focus on the new project.

Its actually a pretty nasty development cycle that I have endured the last two years (almost). Anyways...I hope to make it better, not by forcing something like a code review down anyones throat, but by providing the information so that they can make an informed decision on their own (hopefully, the right one!).



Share this post


Link to post
Share on other sites
mordell -
I can see that and I can imagine that starting to do something like a code review in the middle (or the end stages) of a project would be hard to get people to do. Though, at that point, would it even help? If I''m able to look at the first 1000-2000 lines of code written I am much better prepared to check the incementally larger and larger amounts of code that will be developed. If I have to jump in and review 10''s of thounsands of lines of code, that would just be too daunting of a task. I hope that your team decides they want to have code review in your next project.

At my job we don''t make big applications, it is most database and web stuff. As a fairly junior programmer I find it very cool when one of the "higher ups" asks me to or lets me read through his/her code. It is even cooler when they ask me to check it or just say "Is that right?" Any day now I''m going to be extending an in-house app that someone wrote and I''m sure I would be able to do it in less than a day had I been involved with the development process, but it will probably take me a little longer since I have to figure out how the thing works first.

happy coding everyone.
bcj

Share this post


Link to post
Share on other sites