Category Archives: Code Qualities

Alternative to AlternatingItemTemplate Redundancy in ASP.NET

One of the facets of good coding practices that almost everyone seems to agree on is that redundancy in code, usually via copy and paste, is a dangerous thing.

I saw a great example of this in some ASP.NET code I was working with recently. A tester had reported that some of the items in a display were being truncated inappropriately, while others weren’t being truncated at all. I looked at the screen and could immediately tell what was wrong. Every other row in the table was truncated. It corresponded perfectly with the ledger-paper-style alternating background colors.

Looking at the code, I saw the following things (simplified):


<asp:Repeater id="repeater" runat="server">
<ItemTemplate>
<tr bgcolor="#FFFFFF"><td><%# DataBinder.Eval(Container.DataItem, "Title")%></td></tr> </ItemTemplate>
<AlternatingItemTemplate>
<tr bgcolor="#CCCCCC"><td><%# DataBinder.Eval(Container.DataItem, "TruncatedTitle")%></td></tr>
</AlternatingItemTemplate>
</asp:Repeater>

 
The vast majority of the code in AlternatingItemTemplate has the same intention as the code in ItemTemplate. Whenever you have duplicated code (or near-duplicated code) you will eventually forget to update one of the two things that need to be updated in parallel. Hence the bug.

An alternative, would be to not have the duplicated/duplication-heavy AlternatingItemTemplate, but instead, call a method to get the row color, like this:


<ItemTemplate>
<tr bgcolor="<% = GetCurrentRowColor()%>">
<td><%# DataBinder.Eval(Container.DataItem, "TruncatedTitle")%></td></tr></ItemTemplate>

In the code behind (or even someplace more generalized, if you are going to use this layout more than once, of course), you could have this simple logic:


private const string initialRowColor = "#FFFFFF";
private const string alternatingRowColor = "#CCCCCC";
private string lastRowColor = initialRowColor;
public string GetCurrentRowColor()
{
 if(lastRowColor == initialRowColor)
{
lastRowColor = alternatingRowColor;
return alternatingRowColor;
}
else
{
lastRowColor = initialRowColor;
return initialRowColor;
}
}

 
One Courtesy Note: in the off-chance that my comrade who introduced this problem is reading this, I don’t mean to single you out or pick on you. It’s the sort of mistake that I’ve made a billion times myself, and wanted to capture what I felt was a reasonable solution to the AlternatingItemTemplate redundancy problem.

Also, in case any ASP.NET gurus are out there, I generally don’t like to use the DataBinder.Eval syntax, this is a simplified example, remember?

Code Smells, Correlations, and Poisoned Coffee

Winston Churchill had a much faster and sharper wit than I have. Consider this* famous exchange:

Lady Astor: “If you were my husband, I would put poison in your coffee.”

Churchill: “If I were your husband, I would drink it.”

I, on the other hand, always think of the correct thing to say in an argument a few days later. It’s not just for ex-post-facto arguments, either. I’ve been doing some technical trainings lately, and I try to have a more conversational style than just reading lots of bullet points from slides. I figure that if I don’t understand a topic well enough to speak about it extemporaneously, than I have no business talking about it.

In the last presentation I made, I talked about the practice of refactoring and the concept of code smells.  I gave examples of a few of high-value smells and then gave this little wishy-washy disclaimer.

“When you encounter these code smells, it doesn’t mean that you have to change it, it just means that you should look at your code closely here, as you may have problems.”

It’s not so bad, it’s pretty much what everyone says about refactoring. What I should have said, was this:

Code smells are correlations to quality problems. Heavily commented code blocks aren’t necessarily bad, but lots of comments very strongly correlate to readability problems. You don’t fix it by deleting the comments. You fix it by making your code readable enough to stand without the “how does it work” comments.

Long methods aren’t necessarily bad, but long methods very strongly correlate to cohesion problems. It’s possible, and sometimes required, to have a long method that’s perfectly cohesive, but it’s outside the norm.

And, of course, you don’t fix the problem based on the correlation to the problem, you fix the actual underlying problem. Breaking a long ProcessThings() method into three arbitrary methods called StepOne(), StepTwo(), and StepThree() doesn’t actually make the code any better.

You see, that’s not wishy-washy at all, and it appeals to the distinction between correlation and causation. It’s not as funny as poisoned coffee, but it has some concreteness to it.

*After looking up the Astor/Churchill exchange, I found that it’s very possibly an apocryphal story. Oh well, it’s still funny.

More C# Partial Class Testing Strategies

I can’t take credit for this approach, and even if I could, I probably wouldn’t, because it makes me feel kind of icky.

Anyway, I recently heard about a legacy code testing strategy where you mark your class as “partial”, and in another file, you add whatever public properties/methods you need for your tests. You make the contents of the second (testable file) conditionally compiled (the classic #IF DEBUG) so the encapsulation is still there for any release builds.

It’s kind of like endo-testing, but you’re extending the class “sideways” instead of “downwards”.

Basically, it’s breaking encapsulation in a controlled way, and for the most part, I think it’s a bad idea if you’re working with a new design. If, however, you’re trying to get some meaningful coverage for your legacy code (which wasn’t designed for testability) it can be a good stop-gap in dealing with the legacy code refactoring catch-22: where you don’t want to make changes without tests, but you can’t make tests without making changes. 

Any strategy, such as this one, which allows you to get the first layer of tests down before further refactoring, should be embraced as a good thing.  If you find that you need to do this for any new/original classes, my guess is that your class is too big and needs to be decomposed further into more cohesive and testable classes.

Testability in Isolation: Not just for automated testing

One of the code qualities that I’ve been advocating is “Testability in Isolation”. I’ve expanded the name from the NetObjectives-blessed quality of just “Testability” because I find it more explicit and useful.  Everyone can say “sure, my code is testable, I test it all the time”, and be done with it. If you ask, “Can you test your business logic in isolation from your other layers” you get a more meaningful understanding of the quality of their code.

Usually, I discuss testability in the context of unit tests,  using the strict definition of unit tests: automated, developer-authored, testing one thing in isolation without external dependencies. I’ve found recently that testability in isolation is a virtue when doing manual testing too.

I’ve been working on a Windows Forms application recently, and I found myself drilling down to the same part of the UI over and over while I tweaked one of the user controls. I felt the feedback loop growing, so I created a separate winforms project which I called “Playground” and made a way to get to that particular control I was testing with just one click.

In the playground UI, I use the same test doubles for things like persistence that I use in my unit tests (I’ve got a really nice in-memory-db fake to replace my file-based persistence layer). When I need to, I have the test harness pre-load enough fake data to work with, so when I click the “one button”, it’s set up to exercise what I want it to exercise. Tightens my feedback loop and speeds me up immensely.

It’s weird to think that I haven’t done this explicitly before (although in some web apps, it’s easy to just jump directly to the right page). I’m sure that I’m going to do it again, especially when working with testers, it can give them a way to test just one interaction/user interface component by itself without feeling blocked because parts of the app aren’t ready yet.

Autogenerating C# classes? Add the “partial” keyword

I’m not a big fan of code generation, but I recently thought up a scenario of making it a little more manageable. Here’s the scenario, let’s say you’ve got some tool that generates a lot of C# code for you. Perhaps it’s a “clever” db-to-CRUD code translator. They exist out there. You can’t really change those classes, as your changes can be over-written by whatever tool is generating the code.

Using those types hard-code a database dependency into your domain classes, so you can’t really test in automation anymore.

Two strategies for working with this problem.

1. The usual wrap+fake maneuver. You can encapsulate the auto-generated class into some abstraction (interface or abstract class) and have your domain objects couple to that wrapper.

2. Change the tool to add the “partial” keyword to the class (or change the output of the tool, realizing that you may have to change it again when it gets over written, but it’s what, seven characters?)

Now that this auto-generated type is partial, you can extend it without changing the file. It’s open-closed at the file level. The original example I had was to add just the following in a distinct file.

public partial class AutoGeneratedType: SomeInterface{}

public interface SomeInterface
{
}

Now you can use ReSharper’s “pull Member up” automated refactoring to build the abstraction, couple your domain objects to the interface, and make fake versions for testing in isolation. You are using ReSharper, aren’t you?

And, of course, just like any time that you break a concrete type dependency, you can do things like substitute a different version, add a proxy, etc.

More anti-region sentiments.

Around this time last year while I was working with Kent Skinner at thePlatform and were ripping out the #REGION sections wherever we found them in legacy code, we Googled for the phrase “Regions are Evil” and didn’t find any matches.

Now I’m delighted to say that in addition to my rant from a few months ago, I’ve recently found a few other anti-region arguments. including the specific “Regions are Evil” phrase we predicted.

Mind you, in the comments of one blog, someone calls the anti-region sentiment “a load of elitist horsecrap”. I think I can live with that, though. I’ve been called worse.

Links:

http://blog.jayfields.com/2005/05/do-youregion.html

http://www.oobaloo.co.uk/articles/2007/6/6/visual-studio-regions-are-evil