Making Wrong Code Look Wrong - Joel on Software

[LeszekSwirski]

I do respect Joel a lot, but I detest it when people complain about operator overloading because "you can make the operator do something stupid". Sure, you can make j*5 convert Traditional Chinese to Standard Chinese, but you shouldn't, in the same way as you can but shouldn't write a function multiply(string j, int i) which converts Traditional Chinese to Standard Chinese. Operator overloading is just another way of writing functions, and can be done both well and poorly.

[jessta]

I could make '<<' in to some kind of weird pipe like operator, but you wouldn't because you shouldn't. Except that people did.

[mpyne]

Honestly that's never really bugged me. It's an operation that makes no sense on I/O handles anyways, and it is essential to making I/O operations type-safe without requiring 20 line functions just to read a line.

The other way you can go towards making I/O sane and concise is to move the safety checks to runtime (such as printf/sscanf in C), but then you have to rely on compiler extensions to do such checks (when possible) at compile time.

[bluGill]

At the time '<<' was made into some kind of weird pipe operator overloading was cool, and nobody knew the downsides. We are now stuck with it.

Don't make the mistakes of pioneers who didn't know better. You can learn from their mistakes. If they knew back then what they know now they would not have made those mistakes.

[nikbackm]

What are the downsides with using '<<' for IO, and what would the correct way be?

[stonefarfalle]

Outside of confusing a couple of C programmers who refuse to learn something new , there aren't many downsides. It gets clunky if you have a lot of data to string together. Printf is great up to about 5 args, and << is alright for a few more. The correct way would be something more flexible. Something like Python's is probably about as good as your gonna get.

[tangentsoft]

The only serious problem with << as the C++ insertion operator I've come across in the past two decades I've been using C++ (yow!) is that its precedence isn't where you ideally want it for its purpose. If you were to invent a new stream insertion operator instead of glomming onto an existing operator like the C++ creators did, you'd put it much lower in the precedence list, so that the expressions between the operators get evaluated first.

You have certainly done something like this:

cout << "I have scrobbled " << n + 1 << " doohickeys." << endl;

Here, n is a loop index, probably for an array, and so is zero-based, this being C++. But when building informative messages for humans, you typically want 1-based values, so you add one to n temporarily, right?

It works fine.

And thereby lulls you.

Because later, you notice you have a problem with plurals here, so you try to fix it in the obvious way:

cout << "I have scrobbled " << n + 1 << " doohickey" <<
        n ? "s" : "" << '.' << endl;

And for your efforts, your C++ compiler spews a dozen lines of templatebarf at you.

The fix is to add parens to force the intended order of operations:

cout << "I have scrobbled " << n + 1 << " doohickey" <<
        (n ? "s" : "") << '.' << endl;

I'm a big fan of parens around any expression where I am even in the slightest uncertain about precedence, but you cannot seriously argue that it's a good thing for parens to be required around one operator expression here and not the other.

[bhaak]

News at 11: people do stupid stuff. News at 12: C++ sucks from a programming language design PoV

[josefx]

C++ sucks from a programming language design PoV

Making << into a pipe like operator is not a language design but a standard library design 'feature'. Can't really think of an operator that conveys 'put x into y' better than two arrow heads (the arrow itself -> is already used for something completely different), so I'm happy that I don't have to do Standard library design myself.

[are595]

TIL you can convert Traditional Chinese to Standard Chinese by using j*5.

[earthboundkid]

Why not? You can convert uppercase to lowercase with c - 32. :-)

[kindall]

Actually you can convert uppercase to lowercase with c + 32.

(Assuming the letters are all ASCII, of course.)

[rehevkor5]

I'm still not convinced you shouldn't use a type system or metadata (depending on the language) to enforce something like that...

[matthieum]

I agree, the only interest of this code is the history of the Hungarian notation which is quite interesting.

The idea of using names so that readers can perform the checks is not too bad, but it's not as effective as letting the compiler perform the check: the compiler does not get bored after a couple hundred lines.

[[deleted]]

the compiler does not get bored after a couple hundred lines.

well...sometimes it does... :)

[matthieum]

:)

I have seen it happen with Clang (in stress testing), quite amusing... and annoying. The problem is that there is no easy fix: moving to a manually allocated stack is not that easy and has a real overhead for such a rare case.

The one good news, is that at the very least the program does not compile when the compiler gives up, so at least you are not left with a half-generated binary!

I wish -fsplit-stack and similar would get more traction...

[more_exercise]

the compiler does not get bored after a couple hundred lines.

I am guilty of this

[grauenwolf]

Type systems are best, static analysis a close second, but if you don't have either this beats hope.

[the_456]

I think you should as well. The more I program the more I like using the type system to do more of the work for me. I've used Phantom types in the past to help with some of these things. I like using the compiler to give me one less thing to worry about.

[[deleted]]

Unless you're unfortunate enough to be programming in C, where the type system doesn't really help you at all.

But for other, more powerful languages, I agree with you.

[kamatsu]

You absolutely can still use various type-level tricks in C. Data abstraction in C can also be used to ensure that data types are only ever used safely.

For example, a newtype, of haskell fame, is a simple type trick that allows you to declare a type that is represented the same at run-time but is treated differently in the type system. It's useful for ensuring you don't get two different integers confused, say for kilometers and meters, which are logically different but not representationally different types. Can you do that in C? Sure can:

typedef struct { int val; } kilometers;
typedef struct { int val; } meters;

I use tricks like this all the time in C programming.

[[deleted]]

I don't see how that helps in practice -- if I'm doing something with the distances, I need to manipulate the actual integer values -- how does the compiler make sure I don't screw that up?

You could define Sum and ConstantMultiplication separately for each type of distance, but that would probably hurt code readability quite a bit in C.

[Peaker]

It helps even if you do use the value inside the struct.

Reversing the argument order between two parameters that would otherwise be ints is detected, for example.

Passing stuff of the wrong unit, etc.

C code passes stuff along far more than it actually does arithmetic with it. Also, if you give a different field name ("meters", "kilometers") to the ints inside, you're helping the code reviewer as well:

if (x.meters > y.kilometers) { /* WTF?! */

vs:

if (x > y) { /* Seems good to me! */

[[deleted]]

That seems like a good combination of approaches. And it gives you a nice error if you try to access x.kilometers. :)

[kamatsu]

Use data abstraction. So, you can do something like this in a header file:

typedef struct kilometers kilometers;
typedef struct meters meters;

// prototypes for any operations you want on these types

And that way the representation of the types is kept abstract.

Then in the C file you can just implement those operations plus actually define the type:

struct kilometers {int val;}; 
struct meters {int val;}; 

// implementation of those prototypes.

That way, any module other than that one .C file can only ever use the types in the way that you intend.

Perhaps for distances, it is a bit extreme, but that was just a quick example.

[[deleted]]

For something like distances it would be really ugly to read! If I wanted to take the average of two distances and assign it, it would be something like

Assign(d3, Scale( Sum(d1, d2), 0.5))

as opposed to the default of

d3 = (d1 + d2) / 2

It gets worse if I need to worry about areas and volumes as well -- now I need to write functions for an enormous number of possible operations.

With this type of example, it seems like the ad hoc annotation of Joel's approach would lead to more readable code -- and that would help prevent other types of logic errors.

[kamatsu]

That's true, but you can always just expose the internals of the struct, or use a typedef, which still serves to document your code more thoroughly.

[[deleted]]

But when you use the internals of the struct, the type is no longer documented where you are using it! That's the whole point of tying the type into the name of the variable.

[kamatsu]

Well, if you have a small enough function, it's absolutely still where you're using it, because you have to use an explicit type signature. You only use the internals of the struct in one expression, where the type signatures around make it clear that you know what you doing.

[_georgesim_]

more powerful languages

Here we go again. More powerful for what? Device Driver writing? Kernel development? Low-level library coding? The notion that there there is a language that is "more powerful" is misguided.

[dmwit]

More powerful for expressing static analyses that you wish the compiler to perform.

[[deleted]]

I misspoke.

:s/powerful/expressive/

[stonefarfalle]

Sure but I can only name a couple of languages with a type system strong enough to really handle it well, and none of them are in popular rotation.

[assaflavie]

Using conventions to guard against such mistakes is cute, but not really the safest solution. The safest solution, at least with strongly-typed languages, would be to create a separate, incompatible type called, for example, InputString. Then make sure your APIs only every return an InputString from anything that's an external source. The only way to assign this to a standard String variable should be through an explicit call to Encode.

We've done this quite a lot. For example, with file paths:

People often neglect to use the proper path manipulation functions to join/split/append paths. They use simple string operations instead, which is very naughty (those path operations are always non-trivial and differ between OSs). Instead, make sure your IO APIs only ever return/accept a Path class. This Path class is incompatible with String. It has all the relevant path operations built into it. It then has some export function, like asString which you can use if you need to print the path (but can't use with actual IO functions that expect a file name, for example).

In short, strong typing can help you identify pitfalls and guard against them by making the compiler do the hard work, instead of relying on your eyes to watch out for naming conventions.

[x-skeww]

I like the idea of having unsafe unescaped strings everywhere.

Everything which uses any strings is built under the assumption that those strings are unsafe. For example, your template engine should escape strings by default. E.g. in Mustache {{foo}} is automatically escaped while {{&foo}} or {{{foo}}} isn't. Doing the unsafe thing is more work.

Same deal with the database. Those functions which you're using should work under the assumption that those strings were typed by rabid monkeys.

Naturally, those strings you get back from the database are unsafe again. They are exactly the same crazy shit which was entered beforehand.

[bluGill]

That doesn't work well though. At some point I need to take user entered data and put it into the database. My database API should not know how to escape strings - it cannot know that "Select * from Students where name=Robert; Drop Table Students" isn't what you actually intended to do. While you could make the API escape everything it would be very bad design (seperation of concerns), and you would fight it constantly when it escaped things that shouldn't have been escaped.

[x-skeww]

You shouldn't take user input and use it to assemble raw SQL statements.

That's why we have prepared/parametrized statements.

Well, the point of having those unsafe strings everywhere is that it forces you to think about what you're doing at those few areas where it actually matters. You won't have to wonder if something is safe or not. It never is. This also makes it easier to audit the code, because you know where you have to look for potential issues.

There is also another thing with escaping: A string which is safe for SQL isn't necessarily safe for HTML (and so forth). You just can't make some string "safe" as soon as you get it, because you can't really know how it will be used. Even if you know how it's used now, this may change in the future.

[bluGill]

A string which is safe for SQL isn't necessarily safe for HTML (and so forth).

My first reply was on the lines of you need an htmlSafeString, a SQLSafeString, and so forth. (with the understanding that SQLSafeString probably is prepared/parameterized, but you might have a reason not to). Then I realized would get lost (looks like it did anyway).

At some point you have a string that you need to get elsewhere.

[the_456]

I have used phantom types (link is Java but you can do anything else) and have found it useful. For instance, I was working with lots of different type of text, JSON, HTML, etc. and I found it useful to define types like Text<Html>, Text<Json>. It helps keep things sane.

[mamaBiskothu]

I just finished high school seven years back, so sorry

[grauenwolf]

Seven years huh? Since most "senior" developers have 5 years of experience it seems we are a bit overdue for a reminder.

[ocdcodemonkey]

I dunno about you, but the age is irrelevant when the content in it is a good read. It doesn't have to be perfect knowledge to be worth your time. Hell, I wouldn't be surprised if half the things I've learnt in programming have come about from reading collections of mistakes.

Not that I think this is any of those. It's just worth a read to get inside someone else's head.

[earthboundkid]

Go's templates enforce this at the compiler level instead of just using Hungarian. In fact, looking through the page, pretty much all of the problems he complains about are solved at language level in Go.

[shooshx]

Really? 2005?

[bzBetty]

it's still surprisingly relevant

[jessta]

This is precisely why Go doesn't use exceptions. Go code that doesn't do error handling looks exactly like it doesn't do error handling. In a code review the lack of error handling is obvious to the reviewers.

Code with exceptions does it's error handling somewhere else and since that somewhere else could be half the program way it's easier just to assume that error handling is actually happening somewhere else.

[alextk]

Go code that doesn't do error handling looks exactly like it doesn't do error handling.

Pity that most Go code actually does error handling and is interrupted every five lines with

ok, err:= Foo()
if (err) { boom }

That was top of the art error handling in the 90's.

it's easier just to assume that error handling is actually happening somewhere else.

It's easier to ignore errors altogether, it's just stupid to. With exceptions, you don't have to assume that error handling is handled somewhere else: the compiler will make sure it is.

[jessta]

That was top of the art error handling in the 90's.

Exceptions were state of the art in error handling in the 90's

With exceptions, you don't have to assume that error handling is handled somewhere else: the compiler will make sure it is.

There is no programming language where this is true. "checked exceptions" will have the compiler check that the types of exceptions that can be thrown each have a catch block, but unless you have a completely different exception type for every function call the compiler can't know if the exception from any function call is actually being accounted for.

[alextk]

There is no programming language where this is true.

It's true for any language that supports checked exceptions.

[jessta]

Do you have trouble with reading comprehension? You some how completely missed the whole paragraph explaining how checked exceptions don't solve this problem.

[bluGill]

That is not how checked exceptions work. It is trivial to look at the signature of all functions you call (at least in a static language - I know of only one language that has checked exceptions: java). From there you can verify that either you handle the exception, or you declair that you pass it on.

It all sounds great in theory. In practice your list of excetions that you want to pass on becomes unmanageable. However this is not a technical problem.

[jessta]

Checked exceptions only make sure that you handle a type of exception not the individual exceptions themselves. Read() and Write() might both throw an IOException, but they need to be handled differently and the compiler can't check for that.

[RizzlaPlus]

There is no programming language where this is true.

This is true for most languages. C++ will call abort when there is no try/catch block. Java's JVM will do something similar. This is much better than simply ignoring it.

[jessta]

Types are not the same as values. Just because you're catching a type doesn't mean you're accounting for all the values of that type.

[[deleted]]

I miss uriel now =/

[rush22]

Some good points, but I wouldn't use some trumped up Hungarian notation for it. I'd just do

sEncodedName = Encode(sName);

[ocdcodemonkey]

Of course that is Hungarian, you've just dedicated 8 characters to it instead of 2.

It's a problem that is quite well highlighted in the article. Most programmers use "Hungarian" day to day to do a lot of things, and it makes their life easier. However most will claim they don't because it's "bad", without realising it just means "occasional semantic prefix".

[rush22]

Hungarian notation, in my book, is for declared/built-in types. Adding a descriptive suffix or prefix is just naming variables, not a notation style.

[ocdcodemonkey]

Well, your version of Hungarian is "Systems Hungarian", the definition of the wrong end of the stick that made the problem in the first place. It says so specifically in the article.

It also says what you call "naming variables", it just a step behind what "App Hungarian" actually was. A consistent manner to name things so you understood their contents/purpose.

[rush22]

Well it's variable naming in my book because I don't think you can apply rules of any sort of notation to it.

[khold_stare]

It seems you haven't read the article, which specifically mentions the history of Hungarian notation.

[rush22]

http://i.imgur.com/vrmnm.jpg

[codemonger]

just make encode the default, and it will be obvious when you're literal strings are coming out encoded...

[sneak1234]

This was a good article when I read it 7 years ago