3
\$\begingroup\$

I'm working on a page where a user can create new records, but not all values are required. When creating the SqlCommand with potentially null values, I'm wondering what's the best way, from a code readability standpoint, to check if there's any valid/useful information in a given field. If there is, add that information to the command, if there's isn't, add a DBNull.

Let's say we have a textbox, Address, which is non-compulsory (comm is the SqlCommand):

if(String.IsNullOrEmpty(Address.Text))
{
    comm.Parameters.AddWithValue("@Address", (Object) DBNull.Value)
}
else
{
    comm.Parameters.AddWithValue("@Address", (Object) Address.Text);
}

That seems like a lot of unnecessary typing. But trying to get clever and condense it into a single statement:

comm.Parameters.AddWithValue("@Address",
     String.IsNullOrWhiteSpace(Address.Text) ?
     (Object) DBNull.Value :
     (Object) Address.Text
     );

That seems a little too condensed, harder to parse and understand. Not too difficult to figure out, but (at least to me), also not immediately apparent what's going on. So a trade-off:

Object addressParam = 
    String.IsNullOrEmpty(Address.Text) ?
    (Object) DBNull.Value :
    (Object) Address.Text;

 comm.Parameters.AddWithValue("@Address", addressParam);

(Or the above in a if/else block).
Another option that just occurred as I wrote this question:

Object addressParam = (Object) DBNull.Value;
if(!String.IsNullOrWhiteSpace(Address.Text))
{
     addressParam = (Object) Address.Text;
}

Are any of these methods prefered? I am going about this completely the wrong way? Am I just way over-thinking this? It's the latter, isn't it?

\$\endgroup\$
  • \$\begingroup\$ From a readability standpoint, Entity Framework wins - do you have to do this with a SqlCommand? \$\endgroup\$ – Mathieu Guindon Oct 16 '13 at 22:08
  • \$\begingroup\$ @retail I don't have to do it this way, but I am working with limited resources (just me), a somewhat limited knowledge set (junior dev is being generous), and a very tight deadline. While I'm open to suggestions, trying to learn and implement new things might not be the best idea for deliverabilty purposes. \$\endgroup\$ – MikeTheLiar Oct 16 '13 at 23:24
  • 1
    \$\begingroup\$ maybe not for this project then, but I think your productivity could greatly increase if you learn about LINQ and Entity Framework - less code, more working stuff :) \$\endgroup\$ – Mathieu Guindon Oct 17 '13 at 0:32
5
\$\begingroup\$

I personally prefer your 3rd example

Object addressParam = 
    String.IsNullOrEmpty(Address.Text) ?
    (Object) DBNull.Value :
    (Object) Address.Text;

 comm.Parameters.AddWithValue("@Address", addressParam);

It easily reads that you are evaluating and initializing your Object and directly after you're using your newly instantiated Object. It also doesn't take up as much space as your if-statements and it doesn't really slow you down when reading the code -> you see an Object being initialized and then used, and when needed you can always take an extra second to see what you're evaluating.

\$\endgroup\$
3
\$\begingroup\$

If LINQ sounds like a foreign language, this might not be your most immediately productive option, but Entity Framework isn't that new, and you'd be astonished at how neatly it abstracts away all this boilerplate code. This isn't really a review, because it's a completely different approach, but for what it's worth I find the first listing perfectly readable and much better than casting anything to Object, even if AddWithValue actually takes an Object - I prefer these conversions implicit.

So here it goes - say you have an entity like this:

public class Client
{
    public int Id { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public DateTime? DateOfBirth { get; set; } // this one is nullable, DateTime? is shorthand for Nullable<DateTime>
    public string Address { get; set; }
}

You can easily find lots of tutorials online about using Entity Framework, but for the sake of displaying your case, you'll need a class that derives from DbContext to hold all your DbSets, which hold your entities:

public class MyContext : DbContext
{
    public MyContext(string connectionString)
        : base(connectionString)
    { }

    public IDbSet<Client> Clients { get; set; }
    // ...

    public override void OnModelCreating(DbModelBuilder modelBuilder)
    {
        // customize entity mappings here
        base.OnModelCreating(modelBuilder);
    }
}

There are multiple ways to do this, but whatever you do EF needs to know how entities map to tables and properties map to columns - hence Object/Relational Mapper, and if you don't do anything about it EF will try to make it work by convention, and this means you only write the mapping/ModelBuilder code you need to write.

Now that you've got your entity and a context (you'll need proper configuration in app.config, and the connection string to use can be also inferred from config/context), you're ready to use it.

First thing you'll need is to decouple you UI from your data. This may seem redundant, but you'll want a ViewModel for use in your UI, rather than directly using the entity type (although that would still work). The idea is to not expose stuff that doesn't belong on the UI, notably the Id:

public class ClientViewModel
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public DateTime? DateOfBirth { get; set; }
    public string Address { get; set; }
}

Now this is nice if all you need is to create new clients or display existing ones, but at one point you'll want to update an existing client and you'll want that Id back. I've posted my idea for converting between data and presentation types here on CR, and got good feedback - it was meant for use with WPF, but it can't hurt to take a look ;)

So, assuming we don't care about the Id of an existing Client yet, and only want to create a new one. What your UI code will do is fill up a ClientViewModel instance, with all the required and missing optional values.

I'm not very familiar with web dev (I know, I really should get into that), but I'd try to databind my ViewModel with the form fields (I'm sure it's supported), again to minimize boilerplate code.

Then you can have a "Model" class that works with your DbContext to do all the gruntwork - which is nowhere near the kind of gruntwork you're :

public class MyModel
{
    private readonly MyContext _context;

    public MyModel(MyContext context)
    {
        _context = context;
    }

    public void AddClientAndSave(ClientViewModel client)
    {
        var entity = new Client 
                          { 
                              FirstName = client.FirstName,
                              LastName = client.LastName,
                              DateOfBirth = client.DateOfBirth,
                              Address = client.Address
                          };
        _context.Clients.Add(entity);
        _context.SaveChanges();
    }
}

Just an example, but you get the point: no one gives a rat's ass about which field maps to a nullable or required column, except the mapping code. When you save your new client, give it a null value if that's what you need, provide an actual value if that's what you want. That's all! No SqlCommand, no parameters, no inline sql or anything. Just the code that gets it done.

\$\endgroup\$
2
\$\begingroup\$

If you are going to run into a number of optional values, another alternative is to extract the logic into a separate method. In this case, an extension method may be easiest to work with:

public static class SqlParameterExtensions
{
   public static void AddWithOptionalValue(this SqlParameterCollection col,
                                           string name, string value)
   {
      object param = string.IsNullOrEmpty(value)
                     ? DBNull.Value
                     : (object) value;

      col.AddWithValue(name, param);
   }
}

I only implemented an overload for string values, but you can decide what you want to support and what the method name should be.

Then in the code issuing the command, you can simplify your lines to something like the following:

cmd.Parameters.AddWithOptionalValue("Name1", val1);
cmd.Parameters.AddWithOptionalValue("Name2", val2);
cmd.Parameters.AddWithOptionalValue("Name3", val3);
cmd.Parameters.AddWithOptionalValue("Name4", val4);

As a separate note, I would strongly suggest separating your database and UI logic. The ideal would be to have your view code in one class, database code in another class, and a data object you can both pass to the database class and data bind within your view:

public class BoundObject : INotifyPropertyChanged
{
   public event PropertyChangedEventHandler PropertyChanged;

   public string Address
   {
      get { return _address; }
      set
      {
         _address = value;
         OnPropertyChanged("Address");
      }
   }

   ...

   private void OnPropertyChanged(string prop)
   {
      var evt = PropertyChanged;
      if(evt != null)
         evt(this, new PropertyChangedEventArgs(prop));
   }
}

public class CoolInputForm : Form
{
   private void save_Clicked(object sender, EventArgs args)
   {
      _dbHelper.Save(_bound);
   }

   ...
   private BoundObject _bound;
   private DatabaseHelper _dbHelper;
}

public class DatabaseHelper
{
   public void Save(BoundObject obj)
   {
      ...
      cmd.Parameters.AddWithOptionalValue("@Address", obj.Address);
   }
}

You would then use the UI designer to bind your Address text box with _bound.Address.

Note1: ellipses are only there as filler for other code *Note2: the example is a quick-and-dirty winforms example for simplicity, but the concept works equally well for web projects*

\$\endgroup\$
  • \$\begingroup\$ I'm going to be brutally honest with myself here, but I don't really understand the second half of your answer (that's on me, not you). It's an ASP.NET Web Application, if that makes any difference. It's also fairly basic, it pretty much just exists as a CRUD wrapper for the database. \$\endgroup\$ – MikeTheLiar Oct 16 '13 at 18:16
1
\$\begingroup\$

Another option is to define a static method for the if statement:

static object DbNullable(string s)
{
    if(String.IsNullOrEmpty(s))
    {
        return (Object) DBNull.Value;
    }
    else
    {
        return (Object) s;
    }
}

... which you can use like ...

comm.Parameters.AddWithValue("@Address", DbNullable(Address.Text));
\$\endgroup\$
  • \$\begingroup\$ Personally I would've changed it to IsDbNullValue. But yeah, this is a good and simple way of doing it and it removes a couple of lines whenever OP needs to check if it's null. \$\endgroup\$ – Max Feb 28 '14 at 10:13
  • \$\begingroup\$ I'd expect IsSomething to return a boolean. \$\endgroup\$ – ChrisW Feb 28 '14 at 10:14
  • \$\begingroup\$ That's right... Don't know what I was thinking but you're right. I still think that DbNullable could be changed to something else. Maybe GetDbValue? Or CheckDbNullable? Just throwing some examples out there... \$\endgroup\$ – Max Feb 28 '14 at 10:19
  • \$\begingroup\$ Or maybe ToDbValue. \$\endgroup\$ – ChrisW Feb 28 '14 at 10:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.