CodeSOD: A Private Code Review

Jessica has worked with some cunning developers in the past. To help cope with some of that cunning", they've recently gone out searching for new developers.

Now, there were some problems with their job description and salary offer, specifically, they were asking for developers who do too much and get paid too little. Which is how Jessica started working with Blair. Jessica was hoping to staff up her team with some mid-level or junior developers with a background in web development. Instead, she got Blair, a 13+ year veteran who had just started doing web development in the past six months.

Now, veteran or not, there is a code review process, so everything Blair does goes through code review. And that catches some... annoying habits, but every once in awhile, something might sneak through. For example, he thinks static is a code smell, and thus removes the keyword any time he sees it. He'll rewrite most of the code to work around it, except once the method was called from a cshtml template file, so no one discovered that it didn't work until someone reported the error.

Blair also laments that with all the JavaScript and loosely typed languages, kids these days don't understand the importance of separation of concerns and putting a barrier between interface and implementation. To prove his point, he submitted his MessageBL class. BL, of course, is to remind you that this class is business logic", which is easy to forget because it's in an assembly called theappname.businesslogic.

Within that class, he implemented a bunch of data access methods, and this pair of methods lays out the pattern he followed.

public async Task<LinkContentUpdateTrackingModel> GetLinkAndContentTrackingModelAndUpdate(int id, Msg msg){ return await GetLinkAndContentTrackingAndUpdate(id, msg);}/// <summary>/// LinkTrackingUpdateLinks/// returns: HasAnalyticsConfig, LinkTracks, ContentTracks/// </summary>/// <param name="id"></param>/// <param name="msg"></param>private async Task<LinkContentUpdateTrackingModel> GetLinkAndContentTrackingAndUpdate(int id, Msg msg){ //snip}

Here, we have one public method, and one private method. Their names, as you can see, are very similar. The public method does nothing but invoke the private method. This public method is, in fact, the only place the private method is invoked. The public method, in turn, is called only twice, from one controller.

This method also doesn't ever need to be called, because the same block of code which constructs this object also fetches the relevant model objects. So instead of going back to the database with this thing, we could just use the already fetched objects.

But the real magic here is that Blair was veteran enough to know that he should put some thorough" documentation using Visual Studio's XML comment features. But he put the comments on the private method.

Jessica was not the one who reviewed this code, but adds:

I won't blame the code reviewer for letting this through. There's only so many times you can reject a peer review before you start questioning yourself. And sometimes, because Blair has been here so long, he checks code in without peer review as it's a purely manual process.

[Advertisement] ProGet's got you covered with security and access controls on your NuGet feeds. Learn more.