4 posts tagged “coding”

Programmers write code and code review is often required by managers to be done prior to code submission. I tend to dislike the process since there's a tendency for reviewers to place an emphasis on certain trivial problems and little attention is made to important qualities.

Here's my list of what a code reviewer should look for, from most important to least important:

1. Validation by additional or changed unit tests.
   
2. Duplication of functionality available elsewhere. Ironically, a lot of programmers like to rewrite things, even though it's obviously more work for them.
   
3. Sections of code or idioms repeated.
   
4. Clarity in what the new functionality does.
5. All code is clearly required. Programmers love to make things that "might be useful someday" and usually it's never used. In Java, I often see people implementing "equals/hashCode", serialization, etc. for classes that never use these methods.
   
6. Signs of junior programming practice: "Utility" classes, "Constant" classes, multiple layers (see 5), singletons, and other "Design Patterns Book" nonsense, etc. Note that utility classes aren't really bad per se, it's just that they are often written instead of supplying the appropriate functionality to an existing class or writing a new class.
7. Untyped or too many parameters. Often the same sequence of parameters appear in multiple places, see 3.
   
8. Premature optimization. See: 2. Actually this doesn't really matter as much, assuming this optimization is well tested.
9. Class names that repeat the current namespace. Names that are too long. See 3.

The above common problems are pretty easy to spot. The longer you spend on code reviews and generating comments the more it seems you're being overly critical, so always comment on the most important areas only.

Here are things that aren't as important and almost worth ignoring:

1. Code (in)correctness. It's fairly impossible to find bugs just by reading code and unit testing is hopefully going to prove the code is correct. If you do spot some problem, mention it, I just don't see why you should try to "run" the program in your brain, that's what automated testing is for.
   
2. Missing boiler-plate information, such as legal or copyright notices. This can be corrected using a tool, if needed. We eventually stopped adding boiler-plate to code at m-Qube since our code wasn't shared.
   
3. Lack of code comments. The problem with most comments is they take up space, are often wordy, and don't explain anything new. Comments grow more and more inconsistent over time, so it's almost better to leave them out until the end of a project.
   
4. Improper formatting. Batch tools exist to fix formatting, so there's no need to address formatting concerns in the middle of a project. But since converting line endings causes trouble for code merging, if you see line endings change mention it.
5. Lack of parameter checking. It's a good idea to check parameters when constructing objects, but especially in Java it's not so important for every function.
   
6. Not following naming conventions. Some reason people I work with insist "enum' type names should end in "Type", but everything is a "Type" already so I don't see how it clarifies things. I also worked with people who wanted Java interfaces prefixed with "I", which is not done in the standard Java library. If 90% of programmers don't follow your naming strategy, then maybe it's not necessary.

JBoss hired some new guy who's off adding new methods to a Java interface in a point release of their product. This is because they had no generic expansion point to add information ("meta-data" as it's called) to the existing implementation. Pretty much every coder knows that in an incremental release of a product, you don't change your Java interfaces because any change can break existing customer code.

I pointed out some months ago that adding a generic meta-data interface is a good idea, but maybe I just confused all of them by pointing out that you could also more efficiently internally represent and consolidate the flags in a bit fields. For example, many desktop computers are 32 bit, and you could have 32 attributes represented in a single byte, which turns out to be 32 more space efficient than using a byte per attribute, though a bit harder to code for. So, in my mind my suggestion was a "two-for-one deal", as my friend Kevin Steffa likes to say.

Good design is like this: You have to know what to put in, but most importantly know what to leave out. And this brings me into my talk about cycling.

I was at Perfect Wheels a few months back and a friend (customer?) of Larry's was in and I was in my bicycle. He was intrigued by the Ritchey Break-Away on the wall, and I helpfully offered to let him ride mine. The one on the wall was geared as a fixed gear and mine as a 10x2 speed. He said, I assume jokingly, that gears "confused his mind" and implied that my (in my view) minimal road bicycle was somehow overly elaborate.

I didn't have a strong rebuttal. I got his point.

My old friend Mike Bloch once talked about, on some hike so many years ago, how certain members of Swiss army rode bicycles that didn't have gears. I was rather incredulous and couldn't imagine why the country of the "Swiss Army Knife" wouldn't have a military that would issue bicycles with gearing to climb up mountain passes. Having ridden a fixed gear myself up some fairly steep hills, I understand now that it's quite possible, and not as arduous as it sounds, and additionally for those in the army would be a fair bit more dependable, with less moving parts and a stronger chain. People on the front lines don't want their chain to break or fall of. (I still don't know if his story was entirely true or an urban legend.)

As to computer parts: Most keyboards have dozens of keys even a hacker like me don't need or use: Hackers now can have their own minimalist 60-key keyboard, one with no letter markings. I would get one, except I use a laptop not a desktop computer. The cheapest keyboards, ironically, have the most buttons. For $3 online you get 109 keys with web browser navigation buttons or "media" keys, though I imagine most people with them don't ever use them. The aforementioned blank keyboard retails for $267.53.

And to bring this discussion back to the JBoss coder: He didn't intuitively understand that the existing meta-data functions and new ones were related, so he didn't see any purpose in generalizing. His approach was along the lines of extending a 109-key keyboard by adding new keys. My suggest approach was trying to design features under the 60-key paradigm, by suggesting along the lines of "Leave the existing keyboard alone and add new features with a specialized device." The analogy doesn't fit perfectly but maybe many non-coders who design things can see the difference.

I left work early. I was not particularly engaged in the goings on this week because of the training. I skipped out on two meetings today.  I skipped a number of meetings this week. One was regarding engineering practices and responsibilities. There is this notion of solutions lead and product lead, where products are software packages, and solutions which are customer-specific bundles of individual products. The "lead" part doesn't actually mean I am building or specifying said product or solution, it's just that I am the technical expert. I suppose I also have responsibility in the implementation details, but this is not clear.

Management simply wants to assign responsibility but not necessarily give control. They don't say that, but they would have if that was the point of our new roles.

And not to dwell on my work too much more, I was asked to handle a bug today which was caused by a race condition. I asked the product lead what to do.  He cautiously suggests the simplest fix, with the promise that the underlying cause for the bug (carelessly passing around volatile structures) would be addressed cleanly in a following revision.

I am eager to get a fresh start with the codebase in the next revision.

I left work early to head to Banff.  Andrew K. -- who I met from my Thailand trip -- kindly took me. Banff is a resort town tucked on the east side of the Canadian Rocky Mountains, about an hour from Calgary.  The town and surroundings reminded me simultaneously of Whistler (yuppie ski resort), the North Cascades (dramatic pointy ridges of rock), the Colorado Rockies (dry vegetation, diminutive pine trees), Alaska (flat, marshy valleys), and parts of southeastern B.C (Canadians).

It was late and we were not intending to do much more than hike and eat.  I admit that despite being on a nearly three week trip I didn't really get to know him in Thailand. But we had a good dinner at "the bison" (Rocky Mountain Comfort Food). I had too much beer and rambled on about random topics in the car on the way back.

Andrew talked about possibly visiting the Pacific N.W. I sort of missed or forgot the selling points about coming to Seattle.  San Fransisco or Vancouver seems superior for tourists. Personally, I'd rather be bicycling north out of San Fransisco, or kayaking out around Vancouver than doing either in Seattle.

After a few rescheduling events, I was finally contacted by Google for a phone screening.

A phone screen interview is where you answer a bunch of involved technical questions, usually related to computer science algorithms or general thought problems.

I got to bed quite late last night and was woken up early (I was talked into some sort of Nintendo Wii testing) and was busily working up until the call. And Rei (the cat) was running around screaming his head off. So, perhaps due to my poor preparation I might get passed up. I didn't really understand what sort of CPU scheduling issues would result in starvation, if there wasn't a resource lock of some sort. Jessica sort of through out a bunch of CS questions which you wouldn't really parse unless you were recently out of CS or a domain expert. (That's my excuse if I fail.)

One question on code reviewing I sort of blandly answered: As long as I understand what is being fixed (bug number or requirement), and there is a test, I don't really give them too much thought. Here's basically why: (some I mentioned) If the purpose of a code review is to catch stupid errors or logic errors, then the tests should catch those. If the purpose is to cover style issues, the next person to edit the code can more easily fix them. Humans might provide some help if something is sub-optimal (a better algorithm exists), but premature optimization being the source of evil, I don't really care. I guess they get into the way of getting things done and don't really add too much value compared to the ceremony level they have. I've found "developer focused" companies such as JBoss don't have code reviews, though the code changes are sent out to the list.

At the end, I asked about "why no shuttle service for Kirkland" and if their company culture was anything like Microsoft's. (Microsoft being some sort of lord-of-the flies haven for immature nerd boys who spend all their life at work. Steve "chair throwing" Ballmer being their leader.) The answer to the first question was that only a few people took the shuttle, so they dropped it. Jessica said that crossing the bridge was no problem, I pointed out that it wasn't the crossing, it was the exiting and entering and there was no bus solution from North Seattle. The second answer for me was "of course we aren't like that" -- which is what you would expect to hear.

Assuming they continue talking to me, I actually don't think I will pursue them. First of all, I don't want to drive and I don't see myself taking a bus regularly to Kirkland downtown (two buses, or a bike to Montlake and bus trip) every single day. Secondly, for some reason I don't think they have the salary or other incentives (bonus, stock options, gourmet food, etc.) at my level. Lastly, I don't know how I would fit in there. It feels very lonely being a grunt in a large company; perhaps as a technical manager or something I might not feel as oppressed. We'll see.

I walked away from an Amazon offer in 2002 and had their VP call me. I wonder what happens to people who turn down Google's offers? Do they follow you in a black van and eventually haul you away?