CodeSOD: Don't Do This

Let's say you were writing a type checker in TypeScript. At some point, you would find that you need to iterate across various lists of things, like for example, the list of arguments to a function.

Now, JavaScript (and thus TypeScript) gives you plenty of options for building the right loop for your specific problem. Or, if you look at the code our anonymous submitter sent, you could just choose the wrongest one.

var i = 0;var func1: string[] = func_e.get(expr.name);if (func1.length != 0) { do { var a = typeCheckExpr(expr.arguments[i], env); if (a != func1[i]) throw new Error("Type mismatch in parameters"); i += 1; } while (i != expr.arguments.length);}

Now, it's tricky to reconstruct what the intent of the code is with these wonderfully vague variable names, but I'm fairly certain that func1 contains information about the definition of the function, while expr is the expression that we're type checking. So, if the definition of func1 doesn't contain any parameters, there's nothing to type-check, we skip the loop. Then, we use a do loop, because that if tells us we have at least one argument.

In the loop, we check the passed in argument against the function definition, and chuck an error if they don't match. Increment the counter, and then keep looping while there are more passed in arguments.

Our submitter claims "There's nothing strictly wrong about this snippet, and it all runs correctly," which may be true- I don't know enough about how this code is used, but I suspect that it's going to have weird and unexpected behaviors depending on the inputs, especially if the idea of "optional parameters" exists in the language they're type-checking (presumably TypeScript?).

But bugs aside, the core logic is: if the function takes parameters, iterate across the list of arguments and confirm they match the type. The do loop just confuses that logic, when the whole thing could be a much simpler for loop. As our submitter says, it's not wrong, but boy is it annoying. Annoying to read, annoying to parse, annoying because it should be a pretty simple block of code, but someone went and made it hard.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!