Coding Pet Peeves

Any experienced coder has pet peeves when it comes to reading other people's code or writing code. It might be that you don't like regions or that every method should have comments. But here are my two biggest pet peeves when it comes to C#.

 

Var abuse

WAY too many people are using var instead of a type. This might save your hand, but it doesn't do much for readability of your code. Don't get me wrong, it is VERY usefully at times. But only about 10% of var use is actually productive. The rest is just abuse!

 

Angle Bracket Abuse

Yes, we all know that C# has angle brackets, get over it :). If you have one statement inside of an if, while, do, for, foreach, or using, you DON'T NEED ANGLE BRACKETS! Take the following code sample I found in a starter kit for example:

lock (SyncLock)
{
    if (_cancelRequested == false)
    {
        _cancelRequested = true;
        if (_uploads != null)
        {
            foreach (FileUpload upload in _uploads)
            {
                upload.UploadAsyncCancel();
            }
        }
    }                
}

That same thing can be accomplished through this:

lock (SyncLock)
{
    if (_cancelRequested == false)
    {
        _cancelRequested = true;
        if (_uploads != null)
            foreach (FileUpload upload in _uploads)
                upload.UploadAsyncCancel();
    }                
}

 

Isn't the second better to read? Plus this saves lines. For those of you who get paid by line, go ahead and put those in there, fine, but for those of you who don't, make your code more readable!

18 Comments

  • acctually in Angle Bracket Abuse the first is better to read :)

    it all depends on your own preference so there's no "right way to do this".

  • Surely they're curly brackets, not angle brackets?

  • I wholeheartedly agree on the first count--C# ain't a dynamic language, even though it is starting to feel quite dynamic.

    On the second issue--I respectfully disagree. Bracing all statements helps readability and reduces errors as it is impossible to slip up and do something like have a multi-line loop only execute over a single line for the forgotten braces.

  • If I had to pick between your 2 choices I would pick the 1st, but luckily there is a 3rd alternative which is retains the best of both:

    lock (SyncLock) {
    if (_cancelRequested == false) {
    _cancelRequested = true;
    if (_uploads != null) {
    foreach (FileUpload upload in _uploads) {
    upload.UploadAsyncCancel();
    }
    }
    }
    }

  • You mean Braces (sometimes called Curly Braces), not angle brackets which are used for less than and greater than comparisons and surround XML elements.

    I prefer the first layout example, as it is more clearer. You could do wonders to the second example by only removing the braces from the last nested statement, and only if it is one line.

    e.g.

    lock (SyncLock)
    {
    if (_cancelRequested == false)
    {
    _cancelRequested = true;
    if (_uploads != null)
    {
    foreach (FileUpload upload in _uploads)
    upload.UploadAsyncCancel();
    }
    }
    }

    * crosses fingers on comment formatting

  • Imposing coding stadards it is easier to "always" use braces since there are no "conditions" where to use and where not to use. According to iDesign C# stadards, braces are to "always" be used.

    It truly is preference, but I feel that "always" using does increase readibility and in simpler to abide to a standard. There aren't any questions or scnarios where you may ask yourself, should there be braces...

    Just my angle...
    RA

  • Another vote for preferring always using curly braces.

    I can understand people skipping them for a single conditional statement, but i can't stand it when people nest conditionals and ESPECIALLY loops inside each other without braces. Definitely way LESS readable.

  • I agree on the first one. var is required for anonymous types, of course, and for deeply nested generic types, it's helpful, but for simple types, just use the simple type.

    var x = new { .FirstName="Joe", .LastName="Chung" }; // OK
    var y = new List<KeyValuePair>; // OKish
    var z = "a simple string"; // Not OK

    I disagree on the second. What benefit is there to "saving lines"? It's particularly useful to use braces always with nested if-then-else's.

    Also, I tend to think of angle brackets as , not { and }. I call { and } curly brackets or braces.

  • I never said that I didn't understand WHY people *always* use curly braces (sorry for the error in the post on that one)... but I just find it easier to read. The saving lines thing was a joke :)

    But one common thing people say is that it is opinion. To that, I agree. I was stating my opinion :)

  • Sorry, I disagree on the curly braces. I prefer the first one. Sometime later you might want to add another line and the curly braces will avoid confusion. It is IMO also easier to read than the second.

    Raj

  • I am with you on the secound one I find it easier to read personally :) and also curly brace on the same line not below is good too...

    My 1.5 cents.

  • First Count yes.

    Second Count No.

  • I'm pretty sure McConnell of Code Complete disagrees with your second example, for what it's worth. It's largely for the reason Wyatt Barnett pointed out, as a strategy for error reduction.

  • Your first abuse case I agree with.
    However in your second case I think curly brackets should always be used even if not needed for readability purposes.

  • if (_cancelRequested == false)

    That right there is my pet peeve. Why is an evaluation of a boolean occuring in a conditional?

  • I almost agree with your braces abuse pet peeve, but I'd like to take it a step further, I've got the line break pet peeve on top of that, I mean, you could write it all on one line (everyone knows that saving lines is a virtue): "lock (SyncLock){ if (_cancelRequested == false) { _cancelRequested = true; if (_uploads != null) foreach (FileUpload upload in _uploads) upload.UploadAsyncCancel(); } }".

    OK, if someone didn't get it already, I'm sarcastic, omitting curly braces should be illegal, it's a very effective way to introduce obscure bugs down the road.

    Also I totally agree with Kowalskec about the evaluation of a boolean, makes it harder to read.

    Come to think of it one of my absolute top pet peeves is null checks where the statement is "turned around": if (null == someExpression) {... To me that's so semantically wrong it's crazy.

  • I think using "var" is right 90% of the time rather than 10.

    It's bad if you don't give proper names to your variables. But if you name them properly, then "var" certiainly adds to its readability and writability.

    var customers = new List();

    rather than:
    var c = new List();

  • If I had to choose between the 3 choices i would choose the 3rd where the first curly bracket is placed at the end of the line.
    It's better as extra lines of code are often added later yet it reduces the lines used.

    Luckily there is another choice which I have made and others who don't like all these brackets can make:
    use VB.net

    another thing i have is a 20" monitor in portrait orientation. a great idea for seeing more lines of code at once.
    steve :)

Comments have been disabled for this content.