DaedTech

Stories about Software

By

CodeIt.Right Rules Explained, Part 2

Editorial Note: I originally wrote this post for the SubMain blog.  You can check out the original here, at their site.  While you’re there, have a look at CodeIt.Right to help with automated code review.

A little while back, I started a post series explaining some of the CodeIt.Right rules.  I led into the post with a narrative, which I won’t retell.  But I will reiterate the two rules that I follow when it comes to static analysis tooling.

  • Never implement a suggested fix without knowing what makes it a fix.
  • Never ignore a suggested fix without understanding what makes it a fix.

Because I follow these two rules, I find myself researching every fix suggested to me by my tooling.  And, since I’ve gone to the trouble of doing so, I’ll save you that same trouble by explaining some of those rules today.  Specifically, I’ll examine 3 more CodeIt.Right rules today and explain the rationale behind them.

Mark assemblies CLSCompliant

If you develop in .NET, you’ve no doubt run across this particular warning at some point in your career.  Before we get into the details, let’s stop and define the acronyms.  “CLS” stands for “Common Language Specification,” so the warning informs you that you need to mark your assemblies “Common Language Specification Compliant” (or non-compliant, if applicable).

Okay, but what does that mean?  Well, you can easily forget that many programming languages target the .NET runtime besides your language of choice.  CLS compliance indicates that any language targeting the runtime can use your assembly.  You can write language specific code, incompatible with other framework languages.  CLS compliance means you haven’t.

Want an example?  Let’s say that you write C# code and that you decide to get cute.  You have a class with a “DoStuff” method, and you want to add a slight variation on it.  Because the new method adds improved functionality, you decide to call it “DOSTUFF” in all caps to indicate its awesomeness.  No problem, says the C# compiler.

And yet, if you you try to do the same thing in Visual Basic, a case insensitive language, you will encounter a compiler error.  You have written C# code that VB code cannot use.  Thus you have written non-CLS compliant code.  The CodeIt.Right rule exists to inform you that you have not specified your assembly’s compliance or non-compliance.

To fix, go specify.  Ideally, go into the project’s AssemblyInfo.cs file and add the following to call it a day.

But you can also specify non-compliance for the assembly to avoid a warning.  Of course, you can do better by marking the assembly compliant on the whole and then hunting down and flagging non-compliant methods with the attribute.

Specify IFormatProvider

Next up, consider a warning to “specify IFormatProvider.”  When you encounter this for the first time, it might leave you scratching your head.  After all, “IFormatProvider” seems a bit… technician-like.  A more newbie-friendly name for this warning might have been, “you have a localization problem.”

For example, consider a situation in which some external supplies a date.  Except, they supply the date as a string and you have the task of converting it to a proper DateTime so that you can perform operations on it.  No problem, right?

That should work, provided provincial concerns do not intervene.  For those of you in the US, “03/02/1995” corresponds to March 2nd, 1995.  Of course, should you live in Iraq, that date string would correspond to February 3rd, 1995.  Oops.

Consider a nightmare scenario wherein you write some code with this parsing mechanism.  Based in the US and with most of your customers in the US, this works for years.  Eventually, though, your sales group starts making inroads elsewhere.  Years after the fact, you wind up with a strange bug in code you haven’t touched for years.  Yikes.

By specifying a format provider, you can avoid this scenario.

Nested types should not be visible

Unlike the previous rule, this one’s name suffices for description.  If you declare a type within another type (say a class within a class), you should not make the nested type visible outside of the outer type.  So, the following code triggers the warning.

To understand the issue here, consider the object oriented principle of encapsulation.  In short, hiding implementation details from outsiders gives you more freedom to vary those details later, at your discretion.  This thinking drives the rote instinct for OOP programmers to declare private fields and expose them via public accessors/mutators/properties.

To some degree, the same reasoning applies here.  If you declare a class or struct inside of another one, then presumably only the containing type needs the nested one.  In that case, why make it public?  On the other hand, if another type does, in fact, need the nested one, why scope it within a parent type and not just the same namespace?

You may have some reason for doing this — something specific to your code and your implementation.  But understand that this is weird, and will tend to create awkward, hard-to-discover code.  For this reason, your static analysis tool flags your code.

Until Next Time

As I said last time, you can extract a ton of value from understanding code analysis rules.  This goes beyond just understanding your tooling and accepted best practice.  Specifically, it gets you in the habit of researching and understanding your code and applications at a deep, philosophical level.

In this post alone, we’ve discussed language interoperability, geographic maintenance concerns, and object oriented design.  You can, all too easily, dismiss analysis rules as perfectionism.  They aren’t; they have very real, very important applications.

Stay tuned for more posts in this series, aimed at helping you understand your tooling.

By

How to Perform Effective Team Code Reviews

Editorial Note: I originally wrote this post for the NDepend blog.  You can check out the original here, at their site.  While you’re there, check out all of the new tech debt-related features in the newest version of NDepend.

I’ve heard people say (paraphrased) that teams succeed uniformly, but fail each in its own unique way.  While I might argue the veracity of this statement, it evokes an interesting image.  Many roads, lined with many decisions, lead to many different sorts of failures.

Code review presents no exception.  Teams can fail at code review in myriad, unique ways.  And, on top of that, many paths to broader failure can involve poor code reviews (doubtless among other things).

How can I assign such importance to the code review?  After all, many would consider this an ancillary team activity and one with only upside.  Done poorly, code review catches no defects.  Done well, it catches some defects.  Right?

How Code Review Can Go Wrong

Simply put, code review can have worse than zero effect.  Ineffectual code review suffers mainly the opportunity cost of the participants’ time.  But toxic code review creates morale problems, counterproductive team dynamics, and damaging distractions.

So the first order of business is to avoid a net negative effect.  To do this, one simply has to remove the potential of toxic culture from the process of code review.  Of course, that’s a bit easier said than done, but a lot of it just means following basic rules of human decency.  Start by treating one another with respect.  Then ensure that all participants feel comfortable getting and receiving feedback.  Enlist the help of even-tempered, charismatic folks to lead by example.

Once you’ve insulated yourself against the most damaging effects, it’s time to guard against ineffectual code reviews.  It is toward that end that I’ll be focusing for the remainder of this post.  Ineffectual code reviews can ways time, as I mentioned earlier.  But they can also create a false sense of security and lead to poor choices.

So what makes code review effective?  How can your team get the most out of this activity?  I’ll offer some thoughts based on firsthand experience across a wide number of organizations.

Read More

By

What Technical Documents Should You Review?

Editorial Note: I originally wrote this post for the SmartBear blog.  You can check out the original here, at their site.  While you’re there, have a look at the posts by some of the other authors.

If you’re anything like me in your programming proclivities, there’s a kind of singular impatience that leaps into your mind around the subject of documentation.  On the consuming side, whether it’s an API or a product, you have a tendency to think to yourself, “I don’t want to read about it, I just want to start hacking away at it.”  On the flip side, when it comes to producing documentation, that seems an onerous process; you’ve already finished the work, so why belabor the point?

Those are my natural impulses, but I do recognize the necessity for producing and consuming documentation surrounding the work that we do.  Over the years, I’ve developed strategies for making sure I get it done when it needs to be done.  For instance, even as someone who makes part of my living writing and teaching, I still segment my days for the sake of efficiency; I have writing days and I have programming days.

In order for this work not to get shortchanged in your group, it’s important to develop similar strategies or commitment devices.  The work needs to get done, and it needs to get done well, or else it won’t be useful.  And peer review is a vital part of that process.  After all, you create process around peer review for code — the developers’ strategy for sanity checks and spreading of knowledge.  Doesn’t it stand to reason that you should also do this with the documents that you create around this process?

Let’s take a look at some documentation that your group may be producing, and explore the idea of having peer review to go along with it.  We’ll look at an answer to the question, “what technical documents should you review?”

Read More

By

Improve Your Code Review Game with NDepend

Editorial Note: I originally wrote this post for the NDepend blog.  You can check out the original here, at their site.  If you like posts about static analysis, have a look around while you’re there.

Code review is a subject with which I’m quite familiar.  I’m familiar first as a participant, both reviewing and being reviewed, but it goes deeper than that.  As an IT management consultant, I’ve advised on instituting and refining such processes and I actually write for SmartBear, whose products include Collaborator, a code review tool.  In spite of this, however, I’ve never written much about the intersection between NDepend and code review.  But I’d like to do so today.

I suppose it’s the nature of my own work that has made this topic less than foremost on my mind.  Over the last couple of years, I’ve done a lot of lone wolf, consultative code assessments for clients.  In essence, I take a codebase and its version history and use NDepend and other tools to perform extensive analysis.  I also quietly apply some of the same practices to my own code that I use for example purposes.  But neither of these is collaborative because it’s been a while since I logged a lot of time in a collaborative delivery team environment.

DevOpportunityCost

But my situation being somewhat out of sync with industry norms does not, in any way, alter industry norms.  And the norm is that software development is generally a highly collaborative affair, and that most code review is happening in highly collaborative environments.  And NDepend is not just a way for lone wolves or pedants to do deep dives on code.  It really shines in the group setting.

NDepend Can Automate the Easy Stuff out of Code Review

When discussing code review, I’m often tempted to leave “automate what you can” for the end, since it’s a powerful point.  But, on the other hand, I also think it’s perhaps the first thing that you should go and do right out of the gate, so I’ll mention it here.  After all, automating the easily-automated frees humans up to focus on things that require human intervention.

It’s pretty likely that you have some kind of automation in process for enforcing coding standards.  And, if you don’t, get some in place.  You should not be wasting time at code review with, “you didn’t put an underscore in front of that field.”  That’s the sort of thing that a machine can easily figure out, and that many, many plugins will figure out for you.

The advantages here are many, but two quick ones bear mentioning here.  First is the time-savings that I’ve discussed, and second is the tightening of the feedback loop.  If a developer writes a line of code, forgetting that underscore, the code review may not happen for a week or more.  If there’s a tool in place creating warnings, preventing a commit, or generating a failed build, the feedback loop is much tighter between undesirable code and undesirable outcome.  This makes improvement more rapid, and it makes the source of the feedback an impartial machine instead of a (perceived) judgmental coworker.

Read More

By

Should You Review Requirements and Design Documents?

Editorial Note: I originally wrote this post for the SmartBear blog.  You can check out the original here, at their site.  While you’re there, have a look around at posts and knowledge from other authors.

I remember working in a shop that dealt with medical devices some years back.  I can’t recall whether it was the surrounding regulatory requirements or something about the culture at this place, but there was a rule in place that required peer review of everything.

Naturally, this meant that all code was reviewed, but it went beyond that and extended to any sort of artifact produced by the IT organization.  And, since this was a waterfall shop, that meant review (and audit-trails of approval) of the output of the requirements phase and the design phase, which meant requirements and design documents, respectively.  I can thus recall protracted meetings where we sat and soberly reviewed dusty documents that made proclamations about what “the system” shall and shan’t do.  We also reviewed elaborate sequence, flow, hierarchy, and state design artifacts that were probably obsolete before we even reviewed them.

If I make this activity sound underwhelming in its value, that’s because I routinely felt underwhelmed by its value.  It was a classic case of process over common sense, of ceremony over pragmatism.  Everyone’s attention wandered, knowing that all bets would be off once the development started.  Sign-offs were a formality — half-hearted and cursory.

But is it worth throwing the baby out with the bathwater?  Should the fact that waterfall shops waste time on and around these documents stop you from producing them and subsequently reviewing them?  Is it worth reviewing requirements and design documents?

LotsLeftToDo

Read More