ansaurus

Question

Improve readability of a short snippet while keeping StyleCop happy.

Answer 1

+1 A:

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;

Byron Whitlock 2010-07-26 23:22:10

But now 2 lines has gone to 8 (or 9, if you count the blank)

PostMan 2010-07-26 23:29:30

@PostMan so what? It can be understood by even the most junior of programmers. Any time you do something clever to save a few bytes of space, you are doing yourself or someone else a disservice. Leave that stuff for code golf. Trust me I've been bitten in the butt by this type of stuff before. And yes, it was my code :P

Byron Whitlock 2010-07-26 23:34:21

So we should always expand code out to it's fullest to make it easier for juniors? I guess it's a matter of preference, as I prefer to see as much as possible without scrolling

PostMan 2010-07-26 23:38:06

But to be fair, my boss would agree with you :)

PostMan 2010-07-26 23:39:51

@Postman, It isn't so much a matter of preference but good programming practice. Junior developers always get the crappy maintenance jobs, and you don't want them to be confused since they have never seen a `?:` operator. Nor do you want to spend more than 1 second figuring out what a statement does (as the op had too). No reason to expand everything, but you want to make your and others lives easier.

Byron Whitlock 2010-07-26 23:45:41

I'd much rather have someone who understands what the code is doing, and who spends time reading it, to make changes that someone who doesn't spend more than a second reading it. And if juniors are never introduced to `?:`, how will they ever learn?

PostMan 2010-07-26 23:50:56

But we are drifting a bit of subject here :)

PostMan 2010-07-26 23:51:36

This is too long. I don't think jimmies will have too hard a time if they don't get coddled by unnecessary `if` statements.

Rafe Kettler 2010-07-27 00:28:01

StyleCop wants brackets :)

Hamish Grubijan 2010-07-27 00:32:22

I'm confused. Where is the ternary operator? The original code is just setting a boolean property to the evaluation of a simple boolean expression. Although, admittedly when I do that in my code, I wrap the expression in parenthesis for clarity. bool x = (value1 == value2);

Rob Cooke 2010-07-27 00:34:58

@Rob, @PostMan Perhaps this example is too simple, but my point in general is to avoid doing things that can make your code even the slightest bit hard to understand unless there is a clear benefit. I'd have no problem with the code as it was originally, but the OP said it took him a bit figure what was going on. You want to avoid this as much as possible.

Byron Whitlock 2010-07-27 18:48:42

Answer 2

+3 A:

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

PostMan 2010-07-26 23:26:16

Doesn't improve it much IMHO The crux of the problem is that the `= value == ...` semantic is weird at first glance.

Byron Whitlock 2010-07-26 23:30:50

Right, but it can be smoothed with two lines of comments perhaps. The optimizing compiler will make it fast, I am sure.

Hamish Grubijan 2010-07-27 00:33:30

@Hamish, if you read 1 byte from a file or do 1 SQL query, the performance of this code is irrelevant, optimizing compiler or no. Just a (irrelevant) comment.

Byron Whitlock 2010-07-27 18:51:03

@Byron - ok, but this would be helpful if there was no IO involved.

Hamish Grubijan 2010-07-27 19:03:49

Answer 3

+1 A:

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.

Rafe Kettler 2010-07-27 00:19:22

Thanks for taking the question seriously. I did improve the formatting.

Hamish Grubijan 2010-07-27 00:35:14

ansaurus

tags:

views:

answers:

Improve readability of a short snippet while keeping StyleCop happy.

related questions