Today I got a bug report saying that an error occurred while trying to save a record. That there was an error in this code I inherited isn’t unusual. It was odd, though, to encounter an error in saving a record. That particular bit of code executes hundreds of times per day and I’d never seen a bug report. A few minutes looking at the logs led me to this snippet:

    var outlets = outletRepository.GetOutlets();
    var outletName = outlets.First(outlet => outlet.Id == myRecord.OutletId).Name;

For those of you who don’t speak C#, all that’s doing is getting the list of Outlet records from the database and locating the one that’s referenced by myRecord. We do it this way rather than just looking up the single outlet because we use the returned list of outlets later in the method.

At first I couldn’t figure out why that failed. After all, our database has referential integrity, so myRecord can’t possibly (as far as I know) reference an outlet that doesn’t exist.

It turns out that GetOutlets is defined as:

    public List<Outlet> GetOutlets(bool activeOnly = true)

So a call to GetOutlets() by default returns just the active outlets. Because the outlet referenced by this particular record was marked as inactive, that outlet was not returned by GetOutlets, and as a result the call to Enumerable.First threw an exception.

The most glaring error here is the use of First, which I contend should be used only if you know that what you’re looking for is somewhere in the list. Otherwise you probably should call FirstOrDefault and then check for a null return value.

A more insidious error is the design of the GetOutlets method. In the first place, using a Boolean value to determine whether the method returns all outlets or just the active outlets is a bad idea. true or false are rather ambiguous here. You can’t tell by looking at the value passed what attribute is being tested, or if the flag is used for something else entirely. Maybe true means “sort the records.” Programmers are somehow supposed to “know” what it means. In any case, if the parameter is required the programmer knows that he has to pass true or false in order for it to work. And one would assume that an unfamiliar programmer would check the parameter name and pass the appropriate value. But when you give the parameter a default value, you’ve effectively removed any roadblock to using the method incorrectly. Certainly the statement outlets = outletRepository.GetOutlets(); doesn’t convey any information about the parameter–or that it even exists.

Don’t use Boolean values like this. If you want two forms of the GetOutlets method, then write them.

    public List<outlet> GetAllOutlets();
    public List<outlet> GetOnlyActiveOutlets();

Another option, less preferred but possibly acceptable, would be to define an enumeration that explicitly states what you’re looking for:

    enum OutletFilterType
    {
        All,
        OnlyActive
    }

    public List <outlet> GetOutlets(OutletFilterType filter);

And to call it:

    outlets = GetOutlets(OutletFilterType.All);
    outlets = GetOutlets(OutletFilterType.OnlyActive);

But do not give that parameter a default value.

If you really must take shortcuts in your private implementation–a practice I strongly discourage, by the way, because private code has a distressing tendency to become public–then fine. Make a mess in your own house if you wish, but keep your public interfaces clean and unambiguous.

Every time I think I’m getting a handle on this code, I uncover another little code bomb. The code is littered with this GetXXX(true/false) pattern, and the use of First to get things from untrusted lists. I suppose you could call it job security, but I’m getting tired of it.