Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

When I work with SqlDataAdapter, I usually follow this pattern. Especially, please note the using/fill combination. It's basically a one-liner. The other stuff is just the facility around it.

private void PopulateData(DataTable table, long index)
{
  SqlCommand command = new SqlCommand(
    commandText.Replace("{ID}", "" + index), Connection);

  using (SqlDataAdapter adapter = new SqlDataAdapter(command))
    adapter.Fill(table);
}

Now, I'll be getting a number of scalars, so naturally I'm prone to use the appropriate method for that. However, I'm not certain how to make the code shorter without affecting readability.

private String GetString(long index)
{
  String output;
  using (SqlCommand command = new SqlCommand(CommandText, Connection))
  {
    command.Connection.Open();
    output = command.ExecuteScalar() as String;
    command.Connection.Close();
  }
  return output;
}

Preferably I'd like to get using/returning of the scalar value to be done as a one-liner too. Is it at all possible?

Or is the pattern of open-get-close inside the using-scope the best way to go? Perhaps it depends on the different circumstances? In such case - which?

share|improve this question
    
Why do you care about the length of a method that's this short? Or are you planning to do this for every type you need? –  svick Aug 14 at 15:35
    
You want it to be shorter, golf it!! private String g(long i){String o;using(var c=new SqlCommand(t, m)){c.Connection.Open();o=c.ExecuteScalar() as String;c.Connection.Close();}return o;} :P –  TopinFrassi Aug 14 at 17:36
    
@svick I'm going to have a single type only. And if that ever changes, I can go generic. –  Konrad Viltersten Aug 14 at 19:33
    
@TopinFrassi "Shorter" as in "fewer statements. I was hoping to get something like using/single-statement-in-it. –  Konrad Viltersten Aug 14 at 19:34
1  
@TopinFrassi Ah, missed that. It's quarter past darn late here in Viking time. :) –  Konrad Viltersten Aug 14 at 21:09

3 Answers 3

Just continuation to @Malachi an here is a generic version of ExecuteScalar,

so it is good so you dont have to cast it again in code.

private T ExecuteScalar<T>(long index)
{
    using (var command = new SqlCommand(commandText, connection))
    {
        var output = command.ExecuteScalar();
        if (output != null)
        {
            return (T)Convert.ChangeType(output, typeof(T), CultureInfo.InvariantCulture);
        }
        return default(T);
    } 
}
share|improve this answer
    
I think they are opening the connection outside of this method, so I don't think we need to worry about using the connection. so we probably don't need to open it either –  Malachi Aug 14 at 14:35
    
yes good point editing it –  paritosh Aug 14 at 14:41
    
Why using Convert.ChangeType instead of as T? –  TopinFrassi Aug 14 at 17:27
1  
as T is only applicable for classes not for primitives type. private T ExecuteScalar<T>(object value) {return value as T; }This will throw the compliation error –  paritosh Aug 14 at 17:37

I am not sure about the Private/Public scope of this method, someone else will have to talk about that. but you can return inside a using statement, so instead of this:

private String GetString(long index)
{
  String output;
  using (SqlCommand command = new SqlCommand(CommandText, Connection))
  {
    command.Connection.Open();
    output = command.ExecuteScalar() as String;
    command.Connection.Close();
  }
  return output;
}

you would just return like this

private String GetString(long index)
{
  using (SqlCommand command = new SqlCommand(CommandText, Connection))
  {
    return (command.ExecuteScalar()).ToString();
  }
}

The Command is automatically taken care of so you don't have to worry about explicitly disposing of it.

I assume that the connection is opened and closed by the piece of code that calls this private method, so I left out the connection open and close.

I am kind of on edge about doing it that way, but it seems like you would only want to open one connection for all the SqlCommands that you need to run during the course of the application run.

share|improve this answer
    
Disposing SqlCommand won't dispose the connection, so this code isn't the same as the original. –  svick Aug 14 at 15:29
    
And ExecuteScalar() returns object, so the cast is necessary. –  svick Aug 14 at 15:31
    
@svick, the connection isn't declared inside of this method, I assume that the connection Connection is taken care of outside of the method, so really the connection shouldn't even need to be opened here, the command should be able to be executed without the open command. –  Malachi Aug 14 at 15:49
    
@svick, in that link for the disposing of the connection, the connection wasn't called using the connection method of the command either. I would think that this would change things a little bit because in OP's(Here) code the connection is created by the Command object and not a connection object, so disposing of the Command object should in theory dispose everything created by it. –  Malachi Aug 14 at 15:57
1  
@KonradViltersten, it all depends on your application. the way I suggest may not be the best way for you. –  Malachi Aug 14 at 21:26

Since Close() is the same as Dispose(), you can use using on Connection too. This allows you to get rid of the output variable:

private string GetString()
{
    using (Connection)
    using (var command = new SqlCommand(CommandText, Connection))
    {
        command.Connection.Open();
        return (string)command.ExecuteScalar();
    }
}

I also changed as into a cast: if there is a mistake and ExecuteScalar() actually returns something that isn't a string, you want to know it right away clearly, not learn it later when you get a confusing NullReferenceException.

One more thing I changed is to use var: you can clearly see what the type is, you don't need to repeat it.

share|improve this answer
    
Is there a way to open the sucker inside the using head? It'd be great if I had only one statement to follow the innermost using. I'd skip the brackets, then, hehe. +1 for the first using and how simple that is. Never thought of not declaring the object inside there. However, I'm gravely aversed to var so I'll keep that. The cast is always to String, luckily. And I'll make it generic otherwise. –  Konrad Viltersten Aug 14 at 19:45
    
@KonradViltersten I guess you could write an extension method like OpenAndReturn() and then use using (Connection.OpenAndReturn()), but I don't think that would be a good idea. –  svick Aug 14 at 19:49

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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