write(this); //not that

While reviewing some XAML a few days ago, I ran across this:

<Button Margin="1, 1, 1, 1" ... />

This XAML respectively sets the left, top, right and bottom margins of  the button element to 1.  While this technically works, WPF provides a handy shortcut for setting all margins to the same value:

<Button Margin="1" ... />

If you only provide one value, WPF will assume that you want to set all margins to 1.  Check this out:

<Button Margin="1, 2" ... />

This time, we’re setting the left and right margins to 1 and the top and bottom margins to 2.

I rarely find myself needing to set different values for the left, top, right and bottom margins for a given user interface element in WPF.  If you only need to set one margin, you still need to use the original syntax.  For example, if I was setting only the left margin to 1, I would have to do something like this:

<Button Margin="1, 0, 0, 0" ... />

Note that these syntax rules apply to the Padding property as well.  After wrestling with CSS for years, the layout model in WPF is a welcome breath of fresh air.

You ever run across some source code that you wish you could unsee?  Yeah, me too.  It happened to me yesterday.  Take a look at this:

if (View.ExistingQuantity > 0)
{
   Model.UpdateInventoryAdjustmentItem();
}
else if (View.ExistingQuantity < 0)
{
   // Don't do anything if existing quantity
   // is less than zero.
}
else
{
   Model.AddInventoryAdjustmentItem();
} 

So, if the existing quantity is less than zero… read this comment.  Genius!  Listen folks, business requirements have a time and a place.  This is neither the time or the place.  Interestingly enough, the only way the final else condition will be met is if the existing quantity is zero.  So, although I feel the correct solution here is pretty obvious, let’s go ahead and take a look for good measure:

if (View.ExistingQuantity > 0)
{
   Model.UpdateInventoryAdjustmentItem();
}
else if (View.ExistingQuantity == 0)
{
   Model.AddInventoryAdjustmentItem();
}

Remember, comments aren’t code.  The CLR won’t touch them.  Seriously.  No kidding.  If you don’t need to do anything, then just… don’t do anything.

What up, everybody?

You may have noticed a row of stars at the top and bottom of each post.  You can use these stars to rate content submitted by other users.  Before you ask, no, you can’t rate your own posts and no, you can’t vote for the same post twice.  Nice try.

In the next couple of months we’re going to be rolling out a contest based on these ratings.  The highest rated user at the end of each month will have their name posted prominently on the site and will get some cool swag, courtesy of WriteThisNotThat.com.  For right now I think we’ll probably be doing t-shirts but we are in the process of courting some sponsors so we can toss in some books, development tools, etc.

Next time that you find some particularly fugly source code, share it with us!  There could be a little something in it for you.

Remember, in order to post, you’ll need to register first.  Don’t worry, we promise not to share your information with anyone.  If you don’t believe me, take a look at our privacy policy.

As always, if you like this site, please don’t keep it a secret!

A few months ago, I ran across this abstract class in a project that I was heading up:

public abstract class AuditableEntity
{
  [Column]
  public DateTime DateCreated { get; set; }

  [Column]
  public DateTime DateLastUpdated { get; set; }

  [Column]
  public string CreatedBy { get; set; }

  [Column]
  public string LastUpdatedBy { get; set; }
}

The idea was that by rolling some common audit properties into a base entity class we could keep everything consistent and make auditing at the database level a lot simpler.  For example, if I wanted to make the Person entity auditable, all I would have to is make sure that the Person database table was set up correctly and that my entity class inherits from AuditableEntity:

public class Person : AuditableEntity
{
  [Column]
  public string FirstName { get; set; }

  [Column]
  public string LastName { get; set; }
}

When the Person entity is persisted to the database, a low-level repository class would recognize that the class inherits from AuditableEntity and fill in the appropriate audit information.

I dig the idea of building in some automatic auditing functionality, but, unfortunately, the OR/M tool we were using, LINQ-to-SQL, isn’t too keen on column-mapped properties defined within the base class of an entity.  As a matter of fact, it can’t see them at all.

To get this to work, we would have to pull the auditing stuff back up to the actual Person entity class.  We still wanted to automatically plug in the audit information so I thought that we could just define the properties in an interface that the Person entity could implement; I was thinking something simple like IAuditableEntity.  After a brief conversation with the developer that originally built all of the auditing stuff he agreed to make the changes so we could get back down to business.

The next day, I opened the project back up again to see what he had come up with.  I was surprised to see not only that the base entity class still existed, but it had been updated:

public abstract class AuditableEntity
{
  [Column]
  public virtual DateTime DateCreated { get; set; }

  [Column]
  public virtual DateTime DateLastUpdated { get; set; }

  [Column]
  public virtual string CreatedBy { get; set; }

  [Column]
  public virtual string LastUpdatedBy { get; set; }
}

I was even more surprised to find the updated Person entity class:

public class Person : AuditableEntity
{
  [Column]
  public string FirstName { get; set; }

  [Column]
  public string LastName { get; set; }

  [Column]
  public override DateTime DateCreated { get; set; }

  [Column]
  public override DateTime DateLastUpdated { get; set; }

  [Column]
  public override string CreatedBy { get; set; }

  [Column]
  public override string LastUpdatedBy { get; set; }
}

Alright, folks.  If you’ve ever interviewed for a C# position, you’ve probably been asked the difference between an abstract class and an interface.  In this case, the AuditableClass has been effectively rendered useless.  LINQ-to-SQL won’t be able to see the virtual properties so you have to override them in the Person entity class.  Even if you were going to go this route, which you absolutely should not, you should at least mark the properties abstract so that you’re forced to override them by the compiler.  If you forgot to override any one of the properties, chances are your application would choke when it tried to persist to the database.  Again, you really shouldn’t go this route.

The right answer here is to create the IAuditableEntity interface and have the Person class implement it.  Sure, it’s a little more work since you have to define the audit properties for each entity class, but, until LINQ-to-SQL learns to dig down into a base class it’s your only option.  You could always use stored procedures or default column values at the database level, but we won’t get into that now.

Interfaces are good.  Trust me.  If you’re not defining any actual, concrete base functionality you don’t need a base class.

Take a look at this code that I ran across this morning.

            bool isCommentRequired = false;

            foreach (MembershipChange mc in GetMembershipChanges())
            {
                if (!isCommentRequired)
                {
                    if (mc.PendingAction == MembershipChange.CANCEL)
                    {
                        isCommentRequired = true;
                    }
                }
            }

This code sets a boolean variable to true if any members of a collection meet a given condition.  Once the value of the variable is set it can not be changed within the context of the loop.  Once the given condition has been met, there’s no point in continuing to iterate over the collection so we need to break out.

                bool isCommentRequired = false;

                foreach (MembershipChange mc in GetMembershipChanges())
                {
                    if (mc.PendingAction == MembershipChange.CANCEL)
                    {
                        isCommentRequired = true;
                        break;
                    }
                }

The above code sample will break out of the loop once the given condition has been met.  There are no more extra iterations.  While this code technically solves our problem, LINQ gives us an even cleaner option for refactoring this code.  Take a look.

bool isCommentRequired =
  GetMembershipChanges()
  .Any(mc => mc.PendingAction == MembershipChange.CANCEL);

Delicious.  Remember, don’t iterate over a collection any more times than is necessary.  Those extra iterations can potentially slow down your application especially under heavy volume.  If you’re using C# 2.0, you could always use an anonymous method but, personally, I find those a little ugly.  If LINQ isn’t an option, I’d stick with the for-each loop.