I sometimes use braces to isolate a block of code to avoid using by mistake a variable later. For example, when I put several SqlCommands in the same method, I frequently copy-paste blocks of code, ending by mixing the names and executing twice some commands. Adding braces helps to avoid this situation, because using a wrong SqlCommand in a wrong place will result in an error. Here's an illustration:
Collection<string> existingCategories = new Collection<string>();
// Here a beginning of a block
{
SqlCommand getCategories = new SqlCommand("select Title from Movie.Category where SourceId = @sourceId", sqlConnection, sqlTransaction);
getCategories.Parameters.AddWithValue("@sourceId", sourceId);
using (SqlDataReader categoriesReader = getCategories.ExecuteReader(System.Data.CommandBehavior.SingleResult))
{
while (categoriesReader.Read())
{
existingCategories.Add(categoriesReader["Title"].ToString());
}
}
}
if (!existingCategories.Contains(newCategory))
{
SqlCommand addCategory = new SqlCommand("insert into Movie.Category (SourceId, Title) values (@sourceId, @title)", sqlConnection, sqlTransaction);
// Now try to make a mistake and write/copy-paste getCategories instead of addCategory. It will not compile.
addCategory.Parameters.AddWithValue("@sourceId", sourceId);
addCategory.Parameters.AddWithValue("@title", newCategory);
addCategory.ExecuteNonQuery();
}
Now, StyleCop displays a warning every time a block follows an empty line. On the other hand, not putting an empty line would make the code much harder to understand.
// Something like:
Collection<string> existingCategories = new Collection<string>();
{
// Code here
}
// can be understood as (is it easy to notice that semicolon is missing?):
Collection<string> existingCategories = new Collection<string>()
{
// Code here
}
So,
Is there something wrong in using braces to create blocks of code just for variable scope purposes?
If it's all right, how to make it more readable without violating StyleCop rules?
I don't think there's anything wrong with using braces purely to delimit scope - it can be quite useful at times.
Case in point - I came across a profiling library once that used Profile objects to time sections of code. These worked by measuring the time from their creation to destruction, and therefore worked best by being created on the stack and then being destroyed when they went out of scope, thus measuring the time spent in that particular scope. If you wanted to time something that didn't inherently have its own scope, then adding extra braces to define that scope was probably the best way to go.
As for readability, I can understand why StyleCop doesn't like it, but anyone with any experience in C/C++/Java/C#/... knows that a brace pair defines a scope, and it should be fairly evident that that's what you're trying to do.
There's nothing wrong per se with blocking off code, but you need to consider why you're doing it.
If you're copying and pasting code, you're likely in a situation where you should be refactoring the code and producing functions that you call repeatedly rather than executing similar but different blocks of code repeatedly.
Use the using statement instead of bare brace blocks.
This will avoid the warnings, and also make your code more efficient in terms of resources.
From a larger perspective, you should consider splitting up this method into smaller methods. Using one SqlCommand followed by another is usually better done by calling one method followed by another. Each method would then use their own local SqlCommand.
I think such blocks is good idea, I'm using their often. It is useful when you need to separate blocks of code that are too small to be extracted into method, or when method consists of few code blocks looks like each other, but not with the same logic. It allows to give variables the same names without naming conflicts, and this makes method body more readable.
By the way, my opinion StyleCop has default rule set with more rules which expediency is debatable.
I'd have to say if I were to work on this code after you I'd be a little offput by your use of scope. Its not, afaik, common practice.
I'd consider it a smell that you'd be doing this. I would think the better practice would be to break off each scope into its own method with fully descriptive names and documentation.
