Bad code

Topic/tags: rants, technical, the C programming language, program design

Recently, the Registrar’s office asked the CS department to consider whether we’d accept a standardized CS examination. We might count it as a course equivalent, as general CS credit, as generic science credit, or not at all. The syllabus they sent us some was 250 pages long, including a description of the curriculum, some sample marked solutions, and commentary from graders.

No, I did not read it all. Instead, I jumped quickly to the sample solutions. Here’s one of the first ones I found. I have done my best to maintain the formatting.

int popStack(int stack[], int number)
        {
                int value;
/* Check for error situation first */
                if (number < 1)
                        {
printf("The stack is already emty");
                                return -1;
                        }
                else if (number > 99)
                        {
                printf("Elements size is beyond the stack size);
                return -1;
        }
        else /* Stack can be popped */
                {
                        value = stack[number-1];
                        stack[number-1] = -99;
                        number = number -1;
                        return value;
            }
}

How would you grade this? Think for a minute. If you know me, how do you think I’d grade this? Let’s see what they said.

int popStack(int stack[], int number)                   // 1 mark for data type of return value
        {                                               // 1 mark for passing array parameters
                int value;                              // 1 mark for int parameter number
/* Check for error situation first */
                if (number < 1)                         // 1 mark for testing stacks empty
                        {
printf("The stack is already emty");
                                return -1;              // 1 mark for return message
                        }
                else if (number > 99)
                        {
                printf("Elements size is beyond the stack size);        // 1 mark for testing
                return -1;                                              // 1 mark for message
        }
        else /* Stack can be popped */                  // 1 mark for assignment
                {
                        value = stack[number-1];        // 1 mark for replacing value with -99
                        stack[number-1] = -99;
                        number = number -1;             // 1 mark for returning value
                        return value;
                }
}

No, I don’t know why the comments on the marks don’t quite correspond to the line they are supposedly marking. The problem is worth ten marks, so the student would get full credit.

How would I respond to this piece of code? With comments like the following.

• No documentation for the overall purpose or the meanings of the parameters.
• Formatting so bad that it makes the code unreadable. [2]
• Use of magic numbers like 99 and -99. [3]
• Violates the abstraction barrier by printing error messages directly to the user, rather than to the calling procedure. [4]
• Bad design: Reveal the index of the top of the stack (and thereby the underlying implementation) to the client. [6]
• Bad design: 1-based indexing for the top even though C is a language with zero-based indexing. [7]
• Bad naming: The parameter that represents the top of the stack is called number.
• Spelling error: empty. [8]
• Signals errors with -1, which means that we can’t store that number in the array. This restriction should be documented. [9]

Then there’s that whole number = number -1; line. I don’t know what to do with it. It suggests to me that the programmer really doesn’t understand C. I think they’re trying to change the top of the stack. But C is a call-by-value language, changing number only has an effect in this procedure; it does not change whatever variable caller had. Since they don’t use number afterwards, the line is pointless. We could give it a point by making number (well, top, as it should be called), a pointer. Then the programmer would write *top = *top - 1; or, better yet, *top -= 1;

If a student gave me this code on a final exam, I would have reservations about passing them in the class, no matter how they had done the rest of the semester. I would not want to impose someone who writes code like this on another faculty member, let alone a team of fellow programmers.

I bet you can tell how I’m going to vote on how (or whether) we should count this exam.

Postscript: I wasn’t able to find the problem prompt the first time I looked through the exam. But I found it the second time. Here goes.

Write a function called popStack in the programming language C, which performs the popping operation of a stack. The stack is represented by a one dimensional integer array of 100 elements called stack, which must be passed to the function as a parameter. An integer value called number, which identifies (the top number of elements) in the stack must also be passed to the function as a parameter. In popping the stack, the function should replace the emptied array location with the value of -99- and return the value which was popped. 10 marks [10].

No, I don’t know why the top number of elements is parenthesized. I also don’t know what -99- means. I think it’s just -99.

I’m still convinced that the code is wrong, too. If the integer hold 100 elements, then they should either be 0 through 99 or 1 through 100. But the code disallows a top (what they call number) of 0 and a top of 100.

The problem also does not address the issue that an array-based representation of a stack has two things we need to keep track of, both the array and the top. If you’re going to keep track of the top, you need to pass that to the procedure by reference.

The next problem bothers me just as much.

Write a function called dequeue in the programming language C, which performs the dequeue operation of a queue. The queue is represented by a one dimensional integer array of 100 elements called queue, which must be passed to the function as a parameter. The function should replace the emptied array location with the value of -99 and return the value which was dequeued. The empty location of the array should be set to -99.

As far as a I know, any sensible array-based implementation of a queue maintains two additional values, one for the front of the queue and one for the number of values in the queue [11]. Oh, never mind, I see that their implementation of an array-based queue shifts all of the values whenever you dequeue a value. I guess that saves a bit of storage [12], but at a huge performance cost. I would not want to hire a student who implemented queues this way. I also see that the code will fail when the array is full.

[1] I realize that they may have been given the documentation in a prompt. As far as I can tell, that prompt was not made available ot me. Even if I gave students summary documentation in a prompt, I’d expect them to restate it in their answer. Code needs documentation.

[2] Beginning students screw up formatting. By the end a course, they should not. This formatting is really atrocious. It took me a minute or more to figure out that the right brace before the final else did not match the left brace that started the procedure.

[3] Magic numbers are one of the most basic of code smells. They make code harder to read and harder to maintain. It’s really easy to define constants.

[4] Not everyone understands this principle; perhaps I should be generous. But I really do believe that utility code should only communicate with its client and not with the user. How would you feel if every time a library routine failed, it printed something on the screen? [5]

[5] Oh, wait, Windows used to do that.

[6] Ideally, we’d set up a struct to store both the array and the index of the top (which is also the number of elements). But we’d also have to pass that struct by reference.

[7] Either you assume a client who understands the details or you don’t. This code mixes the two assumptions poorly.

[8] I wouldn’t really take off for that issue.

[9] This is one of those cases in which I really wish that C had exceptions. I don’t like -1 as a return value unless we are careful to specify that it’s an array of non-negative integers. I might suggest returning INT_MIN. But whatever we do would have to be documented.

[10] Source left anonymous to protect, um, someone. Maybe me.

[11] It’s possible to store the front and back of the queue, but I find that approach harder. Full queues and empty queues often have the same values in that approach.

[12] More precisely, it saves about 64 bytes or 512 bits of storage.

Version 1.0 of 2018-04-14.