In the database, the ClosedFlag field can consist of “Y”, “N”, or “I”, which tells us whether an auction is open, closed, or in progress.  Unfortunately, this naming convention made it up through the data access layer and several service tiers, leaving us with the following:

public class Auction
    public bool ClosedFlag { get; set; }

Somewhere along the way, the purpose of this field got confused, and instead of being mapped to an enumerator or something sensible, it ended up as a badly named boolean property which is impossible to decipher.

I’m generally against this type of naming convention, and prefer the self-documenting style of IsSomething, so we could have IsOpen, IsClosed, etc.  In this specific scenario though, there are multiple states and an enum would be more appropriate.  The oddly named naming convention from the database should be restricted to the data access layer, where we’re already converting data to business objects (using AutoMapper), but it seems like it was ignored in this case.


Redundant Property

October 30, 2012

There’s some real incompetence on my current team. WTF would anyone add this code and check it in:

public class BizObject
    #region private fields
    private Boolean _liveBid;

    // More properties here...

	public bool LiveBid {
		get { return State == AuctionState.SaleActive; }
		set { _liveBid = value; } 

Makes no fucking sense.

This developer added a new property to the class, with a setter that sets a private field which is never used. So setting the value doesn’t error, but the getter uses a completely different mechanism for determining what to return.

This could almost (but not quite) be forgiven if this screw up happened as part of the refactoring process, but both the new property and backing field were added in the same fucking checkin.

If the setter is irrelevant, remove it. Why there’s a need to leave redundant or shitty code sitting around is beyond me. Everything is source controlled.

Disaster waiting to happen.