| Spontaneous symmerty breaking |
[09 Apr 2009|10:25am] |
| [ |
mood |
| |
Looking forward to Easter |
] |
I've been doing integrating / code review / bug fixing work for drop 2 this past week and a bit; making sure the UI code the outsourcers wrote ties in with the rest of the system.
<aside>
I've never used thought of myself as a "dot the i's, cross the t's" kind of person but when I was doing the drop 1 bug fixes a month or so ago I got real satisfaction from ticking off the bugs one by one. It wasn't as though there were lots of bugs, it was there were a few that each had a few steps to solve in order to fix the bug. It was this step-by-step approach that I found satisfying, it was like you were layout out a table cloth and as you sweep your hand forward the ripples get pushed forward until the fall off the end of the table and you're left with a perfectly smooth table cloth. Perfectly mown lawn kind of satisfaction.
</aside>
Where was I ... ah yes, the re-occurring thought I had as part of the code reviewer / integration work was the importance of symmetry. For example
if (....)
{
a.b = true;
x.y = false;
}
else
{
a.b = false;
m.n = false;
}
just makes me wonder why m.n isn't set in the upper block and similar for x.y. It might be correct, but as a review / bug fixer I don't know that it is. So if I had to have a "number 1 rule" it would be to write symmetrical code and if you have to break symmetry add a comment as to why*
else
{
a.b = false;
m.n = false;
// No need to set x.y here because ...
}
My favourite email exchange with one of our outsourcers, when asked why something was being cached in one branch of the if statement but not the other, ended with them saying sorry for the delay in getting back to me, they knew there was a reason they'd done it that way but it took them 10 minutes to remember so "I'll add a comment next time".
:-)
* As a colleague pointed out, if all you are doing in the if statement is toggling stuff, you can make the code even more obvious by setting a boolean variable
needToBlah = (....);
a.b = needToBlah;
m.n = needToBlah;
c.d = !needToBlah;
p.q = !needToBlah;
// Only ever need to set x.y to false because ...
if (needToBlah)
{
x.y = false;
}
where (....) is the condition from the if statement. When I used that pattern I liked to put all the = needToBlah together and the = !needToBlah together.
|
|