Blog

Code Reviews in Iambic Pentameter

A few months ago, I tried doing an experiment: Writing my code reviews in iambic pentameter. When you write in iambic pentameter, you can't write nasty things—and if you tried, it would come off as humorous.

In the end, I stopped doing it though because people weren't responding to my reviews.

Anyway, preserved for posterity are the following 42 actual code reviews in iambic pentameter.


1. A 404 is when the story's gone.
But if the problem's that you're not allowed
To see it, then you want a 403.
ForbiddenException, that it should be.

2. To make SOLR respect a private field,
getBooleanSearchFields helps you to be healed.
Mode videos, without this, were all concealed.

Oh, but I see the same thing you did wield.

3. isPrivatePublic makes but little sense;
isPrivacyEnabled has defense.

4. You speak after you sleep, but if announce
That you will sleep, t'were best announced before.

5. IDs are not sequential - oft they jump
From high to low, then high again, then low,
createdDate were better for this flow.

6. Wherefore psw? I think you password mean,
Abbreviating's tasty as benzene.

7. .ban-top-center-cta as a class
Refers to "top" and "center" which are not
Semantic - no they're presentational.
Would that you chose .ban-publish-add-image
Then there'd be no semantical scrimmage.

8. Sort by createdDate, but you should know
ID does not increase in the same way.
Consider:

> select from_unixtime(_created_date/1000), _id from _collection
    -> order by _created_date asc
    -> limit 10;
+-----------------------------------+-------+
| from_unixtime(_created_date/1000) | _id   |
+-----------------------------------+-------+
| 2012-11-08 20:17:29.4910          | 17499 |
| 2012-11-09 01:07:31.3610          | 17610 |
| 2012-11-09 01:09:04.3650          | 17618 |
| 2012-11-09 01:18:07.1550          | 17637 |
| 2012-11-09 01:18:57.7690          | 17556 | <- Jumps down
| 2012-11-09 20:06:21.7750          | 17574 |
| 2012-11-09 20:08:05.8680          | 17579 |
| 2012-11-09 20:08:54.1530          | 17654 | <- Jumps up
| 2012-11-09 20:33:14.9920          | 17593 | <- Jumps down
| 2012-11-10 00:24:31.0810          | 17695 | <- Jumps up
+-----------------------------------+-------+
10 rows in set (0.15 sec)

beginFrom should be *date*, I'd recommend,
else you'll have to account for some loose ends.

9. This use of :not is not the greatest thing.
Can we step back and think what's going on?
.l-cnt-topad - is that content or not?
If no, .l-content should from it be gone.
If yes, .next-bg needs another class,
Perhaps .l-next-bg-content would pass.

10. Instead of papering o'er the problem here
It would be best to ask, what's the root cause?
allOptions.smarty's $defaultMsgKey
On line 282's not specified.
If you put there the $defaultMsgKey 
Then this line will not be needed by thee.

11. You added $defaultMsgKey right here
Beside $newMsgKey, like man and wife.
But in collect.js, I'm 'fraid to say
$newMsgKey, without her partner, lay.
Two places in collect.js she stands
Alone, and quite rejected by her man.

If $defaultMsgKey were put beside,
She and YOKO-6863 be bride.

12. For keeping it top level, I you thank.
The more we do, it's money in the bank.

And these module prefixes are real swank.

13.                             These strings
Should be in en_US's list of things.

14. For every other field, it ends with "-field",
Why was the suffix from this field repealed?
I'd recommend you change to "site-url-field".

15. How this did ever work, I do not know.
The initImagePicker function calls
To processImages, which I'm sure sets
this.images. Unlike the comment says,
We can have images and videos both
Which you can see if you do bookmark this:

http://mightymega.com/2015/07/14/star-wars-tie-fighter-music-box/

16. I thought the powers at be did think it best
To put all z-indexes in _defs.less.

17. I wish we did not have to nest so deep,
Nest here, nest there, it multiplies like sheep.

18. No value for the href? Please explain
Why you thought href="" worthy to disdain.

19. The only thing that the jira required
Is “return path.replace(/.*\/\/[^/]*(.*)\?.*/, '$1')”. Or am I tired?
That regex will return the stuff that's parked
Between the domain and the question mark.

20. After a year, the meaning of "SB"
Will be but a forgotten memory.
Could you rename: collectStoryBuilder?
Then it will surely never bewilder.

21. Why nest these classes in l-bookmarklet?
l-bmk prefix does not suffice?
Removing all this nesting would be nice.

22. You import a big file into a small,
Why not just make this file contain it all?
It would be turning Saul into St. Paul.

23. In recent conversation we agreed
That underscore CSS class names suck.
Please use a hyphen; underscores, please chuck.

24. The top of this file says the prefix is
ems-. But you put ath- here.
Should you not use ems- to cohere?

25.                               I do not understand
Why this has to be so deeply nested.
If it were not nested, would it not work?
If we stopped nesting, it would be a perk.

26. "$googleAnalyticsSecondaryId"
Don't match "googleAnalyticsSecondary"
I would make them same for consistency.

27. This is not the "number of stories viewed",
It is the "IDs of the stories viewed".
I think this comment needs to be renewed.

28. Uma, why did you add _.storyChainedCount,
When we already have _.shownStoryCount?
    Are they not the same?
    Just a different name?
I'd recommend that you remove your count.

29. I'm sorry, but I don't quite understand.
Isn't the status 'active' or 'optout'?
How does the new code differ from the old?

30. If I do showOnBoarding($user, true),
Then showOnBoarding($user) should be true
But it is false. Is not this a boo-boo?

31. A boolean var name begins with "is"
But this is string and not a boolean.
Please rename this to $downloadBtnClass,
And then this name will be as clear as glass.

32. There is no need to strtolower this,
storeLink() already does it, like a kiss.

33. These offsets will be messed up either when
- pruneWaterfallBoxes kills earlier stuff
- reverse infinite scroll adds earlier stuff

How you will deal with these things will be tough.

34. <span class='ban-help-text'> should be changed to be
<span {FEATURED_LINK_ATTRS}>, right?, to agree.

35. newOffsetIndex is indeed array,
So it should be "newOffsetIndexes".
_.offsetIndex too is a list of things;
"_.offsetIndexes" would have better wings.

36. selector ain't a string - it's jQuery
If you rename to "$node" it would be good,
To make it clearer what's under the hood.

37. The activeKeys are keys, so why not make
Them actually keys in _.itemsList? I ache
To see _.getActiveElements looping
Through all the items. No! If they are keys,
activeElements = _.itemsList[activeKeys[i]].
O(n^2) becomes O(n). Gods are pleased.

38. I recommend that you make unit tests
For RedisImitatorService please.
It's easy to make bad bugs when you sneeze.

39. This kind of thing leads to XS attacks.
Could you encode title using this way?
http://stackoverflow.com/a/1219983/2391566
You'll make malicious users cry, "Mayday!"

40. This no longer inits imagePicker;
I'd rename: "getDocumentImages",
The wrong meaning then won't be contagious.

41. I do not grok this logic; woe is me.
.bnd-cvr-featured-author, if parent,
Causes the buttons to be hidden. Why?
Connection I don't see, and so I sigh.

42. I do not know why this is called $events.
It's just the current target, is that right?
So wouldn't $currentTarget be more tight?
comments powered by Disqus

Did you know?

I'm a software engineering consultant. This means I can help your company with your software engineering needs:

  • providing temporary manpower for short-staffed software projects

  • helping new software projects get off to a good architectural start

  • improving the performance and reliability of old, legacy software systems

  • doing an important investigation or small project that you've always wanted to do but haven't had time for

Since 1999, I have done software engineering projects for the Canadian government, for Silicon Valley startups, and for established Bay Area companies, for small companies and medium-sized companies, for successful commercial projects and open-source projects. 

Currently accepting small projects. If you have one, email or call me.