Tutorial :Improve readability of a short snippet while keeping StyleCop happy



Question:

The code below looked ok to me when I wrote it, but when I came back to it again, it was pretty hard to grasp what is going on. There used to be parenthesis around value == ..., but I had to remove them after StyleCop became mandatory (I cannot really control this). So, how can I improve this section of code? I was thinking: x = value == y ? true : false;, but that probably is even more confusing, plus silly, although compiler will optimize that.

set  {      Debug.Assert(value == ConfigType.DATABASE || value == ConfigType.FILE,          "Configuration type must be either 'File-based' or 'Database-based'; it was: "          + value.ToString());        // HG TODO: The following is concise but confusing.      this.fileBasedRadioButton.Checked = value == ConfigType.FILE;      this.databaseBasedRadioButton.Checked = value == ConfigType.DATABASE;  }  


Solution:1

bool isFile = value == ConfigType.FILE;  bool isDatabase = value == ConfigType.DATABASE; // or isDatabase = !isFile    Debug.Assert(isFile || isDatabase,   "Configuration type must be either 'File-based' or 'Database-based'; it was: "   + value.ToString());     this.fileBasedRadioButton.Checked = isFile;  this.databaseBasedRadioButton.Checked = isDatabase;   

This makes it a little more readable (explicitly declaring the bool), you know it has to be true or false.

And this way, if you need to (maybe in the future) change settings based on file/database in the same method, you already have the bool handy, instead of checking each time


Solution:2

If you don't want to use the ?: operator use if..else. Sure it is a little more verbose, but you wont spend more than a few seconds figuring it out.

A few months from now when you revisit this code you will be glad you took an extra 5 lines.

Making code easy to maintain should be your #1 priority.

if (value == ConfigType.FILE)     this.fileBasedRadioButton.Checked = true;  else     this.fileBasedRadioButton.Checked = false;      if (value == ConfigType.DATABASE)     this.databaseBasedRadioButton.Checked = true;  else     this.databaseBasedRadioButton.Checked = false;  


Solution:3

Indent the second and third line of the Debug.Assert() method. It should then look like this:

Debug.Assert(value == ConfigType.DATABASE || value == ConfigType.FILE,      "Configuration type must be either 'File-based' or 'Database-based'; it was: "      + value.ToString());  

I know this is really a minor stylistic alteration, but I've always found when I have to pass a lot of arguments or have some really long statement, when I carry over to a newline I should indent before the ;.

It prevents the Debug.Assert() from looking like 3 lines.

As for the value==, I agree with the previous poster. You should make a bool isDatabase and isFile to prevent calling a field from ConfigType twice in your first arg.


Note:If u also have question or solution just comment us below or mail us on toontricks1994@gmail.com
Previous
Next Post »