How do you make wrong code look wrong? What patterns do you use to avoid semantic errors?

Question

Ever since I first made the mistake of doing an assignment in an if I've always written my ifs like this:

if (CONST == variable) {

to avoid the common (at least for me) mistake of doing this:

if (variable = CONST) { //WRONG, assigning 0 to variable

And since I read Joel Spolsky's essay Making Wrong Code Look Wrong I've been trying to put his advice into practice.

So what other patterns do you use to make wrong code look wrong, or to force syntactic errors if you make a semantic mistake?

Answer 1

Don't use variable names that differ only by 1 or 2 chars. Don't use very short variable names. (Except for loops.)

Use as little global variables as you can. It's really a big problem when you have "int i" as a global (yeah, I've seen something like this in a real life project :))

Always use brackets.

Answer 2

One practice I use (and one that not everyone would even agree with) is always surrounding code blocks (in C++) with { and }. So instead of this:

if( true )
    DoSomething();
else
    DoSomethingElse();

I would write this:

if( true ) {
    DoSomething();
} else {
    DoSomethingElse();
}

This way, if I (or someone else) comes back to this code later to add more code to one of the branches, I won't have to worry about forgetting to surround the code in braces. Our eyes will visually see the indenting as clues to what we're trying to do, but most languages won't.

Answer 3

I always use braces in my code.

While it may be acceptable to write:

while(true)
   print("I'm in a loop")

It is was easier to read with the braces in tact.

while(true){
   print("Obviously I'm in a loop")
}

I think this mainly comes from Java being my first language.

beat to the punch

Answer 4

Along the same lines as your original example is this string literal comparison trick. If you compare a string reference variable to a string literal you have a chance of throwing a NullPointerException.

if(variable.equals("literal")) { // NPE possible
    ...
}

You can avoid that possibility if you flip things around and put the literal first.

if("literal".equals(variable)) { // avoids NPE
    ...
}

Answer 5

I find it important to make wrong code look wrong to the compiler. In practice (and only when using strongly typed languages), this means omitting any kind of variable prefixes (even Apps Hungarian) in favour of distinct types. To use Joel's example, if there are two distinct types to represent raw strings and sanitized strings, and there's no implicit conversion between the two, then the problem that Apps Hungarian addresses can't even occur.

The same goes for the Word document coordinates. In a way, Apps Hungarian is only a workaround for compilers/languages that have no strict enough type checking.

Answer 6

The following is a very good read from Juval Lowy for how to style your code (C#). you can find it here: http://www.idesign.net/ on the right hand side under "Resources"

or here is a direct link (it is a zipped PDF): http://www.idesign.net/idesign/download/IDesign%20CSharp%20Coding%20Standard.zip

The IDesign C# Coding Standard, for development guidelines and best practices by Juval Lowy

Table of Contents:

Preface
1. Naming Conventions and Style
2. Coding Practices
3. Project Settings and Project Structure
4. Framework Specific Guidelines
- 4.1 Data Access
- 4.2 ASP.NET and Web Services
- 4.3 Multithreading
- 4.4 Serialization
- 4.5 Remoting
- 4.6 Security
- 4.7 System.Transactions
- 4.8 Enterprise Services
5. Resources

Answer 7

Konrad Rudolph wrote:

Apps Hungarian is only a workaround for compilers/languages that have no strict enough type checking.

So, you're saying that rather than using a prefix naming convention to instead create and always use two new classes: SafeString and UnsafeString?

Sounds like a much better choice to me. Compile errors are much better than run-time errors.

Answer 8

Hey Bill_the_Lizard, you are missing some closing parens!

Answer 9

If you are playing in HTML land, try to get validating code - there has been a few times the little red x from the HTML validator plugin has given me a handy shortcut to fix a problem that I hadn't even noticed yet.

It may not always be possible to get valid HTML, but it is usually worth aiming for.

Answer 10

One other measure of defensive programming is always to use a break statement in each switch code block (i.e. do not let a case statement "fall through" to the next one). The only exception is if multiple case statements are to be handled identically.

switch( myValue ) {
    case 0:
        // good...
        break;
    case 1:
    case 2:
        // good...
        break;
    case 3:
        // oops, no break...
        SomeCode();
    case 4:
        MoreCode();   // WARNING!  This is executed if myValue == 3
        break;
}

There are times when this may be the desired behavior, but from a code-readility standpoint, I would argue it would be better to refactor that "shared" code to prevent this ambiguity.

Answer 11

Thanks, Matthew! The markdown editor didn't give me a squiggly red underline for leaving out the close parens. :)

I guess that would be a good example of something that should have just looked wrong to me.

Answer 12

@Zack:

So, you're saying that rather than using a prefix naming convention to instead create and always use two new classes: SafeString and UnsafeString?

Sounds like a much better choice to me. Compile errors are much better than run-time errors.

Exactly. Bruce Eckel wrote an essay arguing that static typing is superfluously because, hey, you're writing test cases anyway, right? Wrong. Of course I'm writing test cases but writing good test cases is hard and a lot of work. It's good to take all the help you can get and compile-time type checking is pretty much the best help you can get. Additionally, when using tests, even when using an automated test-on-checkin process, the failed test will be signalled much later than a compile-time error, resulting in delayed error correction. Compilers can give a more direct feedback.

That's not to say that I don't see the advantages of interpreted languages – I do – but dynamic typing can be a huge disadvantage. I'm actually disappointed that there is no modern interpreted language with static typing, because as Joel shows, this makes writing correct code so much harder and forces us to resort to second-class hacks like Apps Hungarian.

Answer 13

@Matt Dillard:

One other measure of defensive programming is always to use a break statement in each switch code block (i.e. do not let a case statement "fall through" to the next one). The only exception is if multiple case statements are to be handled identically.

Sometimes handling case X and case Y means doing almost the same thing, in which case falling through to the next case makes for code that's easier to read. It's a good idea to specifically indicate that with a comment though:

switch( type ) {
case TypeA:
   do some stuff specific to type A
   // FALL THROUGH
case TypeB:
   do some stuff that applies to both A and B
   break
case TypeC:
   ...
}

If you use this convention, all case statements should then have a break, a return, a continue or a comment indicating that it's falling through. An empty case without the comment is OK though:

case TypeA:
case TypeB:
   // code for both types

Answer 14

If the programming language allows it, only declare variables at the point where you initialize them. (In other words, don't declare them all at the top of every function.) If you consistently declare and initialize in the same place, you have a lot less chance of having uninitialized variables.

Answer 15

Always declare variables to be "const" (or the equivalent in your programming language) if there is no reason for the value to change after initialization.

Answer 16

Avoid nested loops, and avoid indenting code more than a couple of levels.

Nested loops or deeply-nested code can generally be refactored by extracting the nested code into a function/method. This usually makes the code easier to reason about.

Answer 17

heh, I was half way through typing this and wondering if it was really that useful, but since Matt and Graeme have both posted answers about this I'll continue.

A few days ago while adding a new case to a switch I forgot to end the case with a break. Once I found the error I changed the indentation of my switch statement from:

switch(var) {
   case CONST1:
      statement;
      statement;
      statement;
      break;  
   case CONST2:
      statement;
      statement;
      statement;
   case CONST3:
      statement;
      statement;
      break;  
   default:
      statement;
}

(which is how guess most people would normally indent) to this:

switch(var) {
   case CONST1:
      statement;
      statement;
      statement;
   break;  
   case CONST2:
      statement;
      statement;
      statement;
   case CONST3:
      statement;
      statement;
   break;  
   default:
      statement;
}

To make the missing break stand out, and to make me more likely not to forget to add one when I'm adding a new case. (of course you can't do this if your breaking conditionally in more than one place, which I've done on occasion)

If I'm only doing something trivial like setting a variable or calling functions from the case statements then I often structure them like this:

switch(var) {
   case CONST1: func1();  break;  
   case CONST2: func2();  break;  
   case CONST3: func3();  break;  
   default: statement;
}

That makes it super obvious if you miss a break. If your statements aren't the same length add whitespace till the breaks align, along with anything else that makes sense:

switch(var) {
   case CONST1:          func1("Wibble", 2);  break;  
   case CONST2: longnamedfunc2("foo"   , 3);  break;  
   case CONST3: variable = 2;                 break;  
   default: statement;
}

Although if I'm passing the same parameters to each function I would use a function pointer (the following is actual code from a work project):

short (*fnExec) ( long nCmdId
  , long * pnEnt
  , short vmhDigitise
  , short vmhToolpath
  , int *pcLines
  , char ***prgszNCCode
  , map<string, double> *pmpstrd
  ) = NULL;
switch(nNoun) {
 case NOUN_PROBE_FEED:  fnExec = &ExecProbeFeed; break;
 case NOUN_PROBE_ARC:  fnExec = &ExecProbeArc;  break;
 case NOUN_PROBE_SURFACE: fnExec = &ExecProbeSurface; break;
 case NOUN_PROBE_WEB_POCKET: fnExec = &ExecProbeWebPocket; break;
 default: ASSERT(FALSE);
}
nRet = (*fnExec)(nCmdId, &nEnt, vmhDigitise, vmhToolpath, &cLines, &rgszNCCode, &mpstrd);

Answer 18

Best things in my books is noise reduction. This is why I love separation of concerns with exceptions being a good example of this (ensuring the handling of error cases isn't inline).

If the noise is reduced then the code you are looking at is only there to perform the particular purpose that the method/function name suggests, this makes it much easier to spot bad code.

Answer 19

I've played with the (0 == variable) trick but there is a loss in readability -- you have to switch things mentally to read it as "if variable equals zero."

I second Matt Dillard's recommendation of putting braces around single-line conditionals. (I'd vote it up if I could!)

One other trick I use when performance isn't critical: I'll define

void MyClass::DoNothing()
{

}

and use it in place of null statements. A bare semicolon is easy to lose. One can add the numbers 1 to 10 (and store it in sum) like this:

for (i = 1; i <= 10; sum += i++)
    ; //empty loop body

but this is more readable and self-documenting IMO:

for (i = 1; i <= 10; sum += i++)
{
    DoNothing();
}

Answer 20

One thing that I try to use, if possible. is to avoid using boolean types as parameters to functions, especially if there are multiple parameters.

Which is more readable...

compare("Some text", "Some other text", true);

...or...

compare("Some text", "Some other text", Compare.CASE_INSENSITIVE);

Admittedly, this can be a bit of overkill sometimes, but it not hard to set up, improves readability and reduces the chances of the author incorrectly remembering whether 'true' means 'Yes, do to comparison in a case-sensitive manner' or 'Yes, do to comparison in a case-insensitive manner'.

Of course, cases such as...

setCaseInsenstive(true);

...is simple and obvious enough to be left alone.

Answer 21

In my engineering applications I make units of measurement and reference frames part of variable names. That way I can easily spot inconsistencies. Examples:

r1_m  = r2_ft; //wrong, units are inconsistent (meters vs feet)
V1_Fc = V2_Fn; //wrong, reference frames are inconsistent 
               //(Fc=Local Cartesian frame, Fn=North East Down frame)

Answer 22

Use a bit of Hungarian

It can be helpful to label variables. For example, if you're sanitizing user inputs, you might do something like this:

$username = santize($rawusername);

That way, if you're about to say echo $rawusername, it will look wrong, because it is.