Just Let It Go
I was reading a blog post (Narcissism of Small Code Differences) today, and something rang true.
Basically the post (or what I got from it), is that the differences in ways people program are just that, differences. Just because a bit of code maybe isn’t written the way I would write it, doesn’t mean it’s wrong. As depicted in the post, there may be reasons why code is done in a certain way!
I mention this now, as I’ve just come across some code from a co-worker that in the past, I probably would have re-written, it’s a simple thing, and really more style based than efficiency etc.
The co-workers code reads like:
if(!xxx)
{
/*Do some stuff*/
return false;
}
else
return true;
Here I would itch to get rid of the ‘else’, either:
if(!xxx)
{
/*Do some stuff*/
return false;
}
return true;
as the ‘else’ is irrelevant in this case. Or even:
if(xxx)
return true;
/*Do some stuff*/
return false;
Neither would change the way the method works, but both are purely style changes that look more pleasing to me. But, hey! What if the original coder prefers it his way – it’s wrong for me to force my style on someone else.
So today I’ve decided to let go any style changes unless they significantly make the code easier to read / debug etc… Yay for narcissistic me!
I was reading Raganwald the other day as well, great stuff. Your post is another good example.
At an earlier point in my career, I often changed code in similar ways. Worse, I reformatted nearly everything I touched. Ultimately, this is a lot of “fire and motion” without real benefit. It is very rare that someone formats their code in ways which are truly difficult for me to read.
In working with others and using source control, I’ve come to appreciate commit logs which are “clean” and communicate only the “significant” changes. If I have a really good reason for wanting to reformat or rearrange a bit of code, I reserve such changes for separate commits.
That said, there is a line of reasoning you can take behind performing some of the latter changes you’ve described above, beyond style. I believe it’s explained in “Code Complete” (highly recommended reading!)
The short of it is, you put your gating factors (trivial rejections, etc.) at the top of a function, to quickly return or escape from unnecessary processing. Then you allow the common case to occupy the “root level” of your function. The idea being that the normal flow of control should not be nestled away in conditionals (especially deeply nested ones!) Any given function has a specific purpose (if you adhere to SRP) and that purpose should read as simply as possible in the “most expected” location.
Surely
return xxx;
is the way to go
Sorry – hadn’t raed the code carefully enough – ignore me
Yeah, see, now, I personally really like keeping the else clause in, even if it isn’t strictly necessary, because I like how it reads — the indentation, for example, makes it clear that the part meant to execute when the expression is true and the part meant to execute when it’s false are related in that they represent two outcomes of the same test.