Title: Student Code Critique Competition 27

Dear Student

I think I should remind you that the set text for this course is Accelerated C++ by Koenig and Moo, and you are supposed to read it. I looks to me as if you haven't.

First of all let's look at what you have. We can see more clearly what is happening if we also display the index, together with the data as characters and code, as

for(int i=0; i<=len; i++)
  cout << i << " " << str[i+1]
       << " " << (int)str[i+1] << endl;

which on a machine using ASCII yields -

0 e 101
1 e 101
2 d 100
3   32
4 h 104
{similar lines snipped by FG}
17 . 46
18   0
19 + -64

So your loop outputs 20 characters (0 to 19 inclusive) The 19th is the zero marking the end of the string, and the 20th is whatever is in memory after that. We can simply fix your loop as

for(int i=0; i<len; i++)
  cout << str[i] << endl;

However there are some issues with the way you are doing this. Firstly, how did you choose the value of arraySize? Did you count the letters in the string and add 1 for the 0 at the end? Pretty tedious, and it makes using strlen pretty pointless.

Secondly, the approach is not easy to adapt to other strings - in other words there is a close link between the algorithm and the actual data it processes. To cope with another string, would you have changed arraySize? To what? You could fix it at 1024 and limit this to strings of length less than that, but that would be pretty unsatisfactory.

Using an array of characters is a common way to represent a string in C. Alternatively you might have said

char * str = "Need help with C++.";

Then said something like

for(char * ptr = str; *ptr; ptr++)
  cout << *ptr << endl;

which relies on the fact that at the end of the string *ptr is 0 and therefore false, ending the loop. In other words it depends a lot on the way the string is stored - the details of the implementation. This is how it is done in C - but we're supposed to be using C++.

Why have you been given this as a task anyway? What is the point of outputting a sentence one letter at a time? Hopefully you realise that this is an exercise concerned with the two basic questions:

In this exercise the thing of interest is an English sentence. We need a way of representing that inside the machine. The second question in this case is how to 'go through' that representation - to traverse it. This is a very common real task. You traverse representations to search them, count them, add them up, change them and so on.

We want to do this with the minimum of effort, and the maximum of generality - so in future we can do it with even less effort. If possible we want to avoid the details of how it is done - we just want to think of it as a string, not as bytes or values in RAM. In other words we want to work at as abstract a level as possible.

But the Standard C++ Library has such a thing, not surprisingly called string (surely you got as far as Chapter 1 in Accelerated C++?). We just say:

string str = "Need help with C++.";
cout << "The sentence \" << str
     << "\" has " << s.length()
     << " characters." << endl << endl;

How is that string stored in memory? Who cares? Not our problem. We want to traverse that string from start to end, so we can just say

for(string::const_iterator i=s.begin();
    i!=s.end(); i++)
  cout << *i << endl;

If you've lost the handout on iterators, see chapter 5 in Accelerated C++.

[As a teacher I would have to say that a struggling student would almost certainly give up the course if faced with such a critique. Of course the work might be that of a bright but lazy student but it does not have the hallmarks of such work. To me it looks like the work of a very conscientious student who has completely failed to grasp a couple of important elements, such as how for-loops work. Francis]

Le us solve the student's immediate problem of how to turn a list of similarlooking statements into a for-loop to save repetition.

The existing code looks like:

cout << str << endl;
cout << str + 1 << endl;
cout << str + 2 << endl;
cout << str + 3 << endl;
etc.

Taking this very slowly, the first thing to do is to ensure that all the statements have a similar form, so I shall change to first line to:

cout << str + 0 << endl;

This gives them all an addition operator, but adding zero has no other effect. Now we can progress to producing a list of statements that are all textually identical by introducing a variable to replace the constants in each line:

int i = 0;
cout << str + i << endl;
i++;
cout << str + i << endl;
i++;
cout << str + i << endl;
i++;
..etc

Now that really can be replaced by a loop - provided that we know when to stop. The final statement in the original code is

cout << str + 18 << endl;

so we need to stop when i passes 18, i.e. continue while i is less than 19.

We can now wrap up all those identical statements into this:

int i = 0;
while(i < 19) {
  cout << str + i << endl;
  i++;
}

and we realise that the construct:

iterator = start;
while(condition) {
  body-statements
  iterator++;
}

can be replaced by a for-loop:

for(iterator = start; condition; iterator++)
  body-statements

which gives us our for-loop:

for (int i=0; i<19; i++)
  cout << str + i << endl;

"Magic numbers", i.e. constants used in their numeric form, should be avoided, except for zero in most cases. Sometimes these constants are part of the problem domain, e.g. the acceleration due to gravity, in which case they should be provided by a named constant (or a macro in older C implementations). Here, the 19 output statements are due, I assume, to the length of the test string. Since we have calculated the length, we can use it. This also means that if the test string is changed, the number of lines exactly equals the length:

for (int i=0; i<len; i++)
  cout << str + i << endl;

Some people would argue that the pre-increment ++i should be used in place of the post-increment here. For integers it makes no difference. For more complex iterators it might do, so it is possibly better to stick to a consistent style to save surprises later.

If we go back to the beginning of the student's code, we spot that the main routine has been declared to return void. This is incorrect - it always returns int. My compiler, with warnings enabled, tells me this and kindly(?) changes it to an integer return.

Then comes another "magic" constant. Why choose 20? Ah, it must be that string being used to test again. This is problematic because if the string were changed to "I really need a lot of help with C++." then that array of 20 characters is not going to be big enough and all sorts of nasty things might happen. C++ has inherited from C the ability to dynamically size arrays from their initialisation lists, and also to initialise an array of chars by a string, so we can use this to create enough space automatically:

char str[] = "Need help with C++.";

We then wonder why we should rely on the function strlen to find the length. The compiler has already does this for us to allocate the size of the array "str", so we can use the sizeof operator to get the value at compile time, remembering that the sizeof operator will include the trailing null, whereas strlen does not. If we put this into a constant then the compiler should use the literal value rather than a variable:

int const len = sizeof(str) - 1;

The only proviso here is that if the literal string contained embedded nulls then strlen would only count to the first such null, whereas sizeof uses the real number of characters in the string.

The whole program now looks like this:

#include <iostream>
using namespace std;

int main() {
  char str[] = "Need help with C++.";
  int const len = sizeof(str) - 1;
  // Ignore trailing null
  cout << "The sentence \"" << str
       << "\" has " << len
       << " characters" << endl << endl;
  for(int i=0; i<len; ++i)
    cout << str + i << endl;
  return 0;
}

Note that I have also removed the duplication of the string embedded in the introductory output statement. That ensures that if we change the test string then the output is consistent. Also the header cstring is no longer required to declare strlen.

Although I would prefer not to include the whole of the std namespace and instead specify specifically which classes or functions I want, my compiler includes the namespace as standard and so I cannot check for errors if I omit some declarations. Therefore I have left this statement in place.

OK, so that has fixed the initial problem, of removing the duplicate cout statements. The student said that he wanted to write the body of the loop as:

cout << str[i+1];

for some value of i. This statement only puts one character from the string onto the output and there is no endl either. This indicates that the requirement was to print out individual characters. There is rarely a need to do this on a string, in real life, but as an exercise it is useful to know how to access parts of the string, so I assume that this is what is required. The student also mentions nested for-loops, which also suggests that individual character writing is wanted.

I wonder whether the requirement was stated as "replace the cout statements with a loop?", meaning "replace all cout statements with one inside a loop" while the student understood it to be "replace each cout statement with a loop of its own". Such a misunderstanding would not be unusual.

Let us assume that the student's interpretation is correct, how do we go about this? Each iteration in the loop, in its current form, prints the string starting from the i'th element. The way that char* strings are handled (also an inheritance from C) means that all characters from the pointed-to one are printed, until a null character is found, which is the same as saying until the string length as defined by strlen is reached.

The inner loop plus the end-of-line should produce exactly what the

cout << str+i << endl

line produced earlier, i.e. in pseudo-code:

for all characters from the i'th to the len'th
  print the character
print end-of-line

Using the conventional counter name j (when the outer counter is i), we get:

for(int j=i; j<len; ++j)
  cout << str[j];
cout << endl;

Putting this inside the existing loop, I get:

for(int i=0; i<len; ++i) {
  for(int j=i; j < len; ++j)
    cout << str[j];
  cout << endl;
}

The char* (and char array) style strings are very much a legacy from C and their use should really be discouraged when teaching C++ to new students, who should be introduced to the std::string class instead. This student appears to be trying to learn about loops rather than strings and so an array of integers might have been more appropriate. Again, a std::vector might be even better, since this provides more safety than a plain C array.

On the other hand, the student may be in an environment where they are using C++ as "a better C" and steer clear of the more complex aspects of C++. The sample code is very much along these lines, since it only uses cout and the stream insertion operator from C++, all the rest is plain C. From a C-programmer's point of view, using C++ in this way is a natural progression - I would even own up to doing it myself - but it does mean that some C ways of doing things need to be re-learnt to use C++ to its fullest power. If the student is in this sort of environment, then learning better C++ techniques will do no harm, and may well lead the student to become the local expert on C++. Helping others with their C++ will enhance the knowledge of the teacher too, as Francis keeps trying to tell us.