Title: Student Code Critique Competition 26

#includes should follow the C++ standard and use the include files without the .h extension. The C header for math should be replaced by cmath, which includes the math definitions in the std namespace.

Some of the #include statements in the program are not necessary, and they should be removed from the code. Include statements which are not used add false dependencies.

For large programs with a lot of header files, the false dependencies can forces a recompilation of an otherwise unchanged compilation unit. On a heavily loaded machine the reading and parsing of the include files can be a large part of the total compilation time.

Forward declarations of functions and variables should be deferred to the latest possible time. This ensures that the declaration is made as close to point where it is used as possible. A consequence of this is the removal of declarations that are not used.

Following this advice, we remove the lines:

void clrscr();
int i, r, j, m, x;

One should try to only use arrays of the needed size. Allocating more memory than we need might be masking that we do not really know how memory is used. It could be hiding an error in indexing, which could be hard to spot, even when using memory bounds checkers.

A simple scan of the code that assigns to the array Sum shows that the variable j, which is used as the second index into the array, only indexes the range [1:11]. If we change the declaration of the array Sum to reflect this usage, we reduce the amount of memory required by a factor of 454.

The first index i runs in the interval [1:2048], which leads to a reduction of the needed memory by a factor of approximately 2.5, all in all we are able to reduce the memory need of the program by a factor of over 1100.

The indexing into the array is done from 1. The C and C++ arrays are normally indexed from 0, but adapting an existing algorithm that used indexing from 1 can lead to problems.

This is especially important if the code might be used by others, or if the code will be used again and again over a period of years. It will be a lot easier to see the connection between the algorithm and the code if we keep indexing from 1.

The declaration of the array should look like this:

long Sum[2049][12]

The keyword static that was previously used is not necessary [I think the writer has missed the significance of using static to both ensure default initialisation and to use 'static' memory provided at load time rather than, possibly precious, stack memory.] The size of the array has taken into account that the indexing starts at 1.

The for-loops should declare the type of the counting variable it uses inside the for-statement. This makes sure that the counting variable is only visible inside the for-loop [As this was for an MSDos platform, that might not be true].

The inner for-loop contains the construction:

for(int i = 1; i < pow(2,j); i++) {
// code //
  i = i + 1;
}

Because the code block spans several lines, it is not immediately obvious that the for-loop step size is two and not one. This should be written as:

for( int i = 1; i < pow( 2, j ); i =+ 2 ) {
// code //
}

The innermost loop has the following construction:

for(i ......) {
  if(i == 1) {
  // code //
  }
  if(i > 1) {
// Code using variable r
    r = r + 1
// code not using variable r
  }
  r = 2;
}

From the above it should be obvious that every time r is used, it has the value 2, and we might as well change the single usage of r to using a properly named variable, like

const long magic_constant = 2;

Making it obvious that the code depends on a magic constant.

To ease the reading of the for-loops writing the data, these loops should use the same variables as was used to generate the data:

for(int j = 1; j <= 11; ++j) {
  for(int i = 1; i <= pow(2.0, j ); i++) {
    output << i << " " << j
           << setw(10) << Sum[i][j]
           << '\n';
    output << i << " " << j
           << setw(10) << Sum[i][j]
           << '\n';
  }
}

The innermost loop for writing the data two times could be changed to two lines, writing the same output. This makes it more obvious that the data is written twice, at the cost of making all changes to the writing code in both places, but the eye is quite good at catching differences in lines that should look the same.

The post increment operator used in the forloops, should be changed to the pre increment operator, if not the code will generate a temporary to hold the value before incrementing, that will never be used.

The use of space characters in the code, can significantly increase readability, especially if used in a consistent way, like always enclose operators like +, -, *, /, =, ==, <= and so on with an equal amount of space before and after. [However placing spaces after any form of open bracket and before any form of close bracket often reduces readability. In addition there are good readability arguments for not placing spaces either side of a strongly binding operator such as * or /.]

The rule could read something like: Binary operators have an equal amount of one or more space characters on both sides.

The difference is clearly seen comparing two versions of the first for statement:

for(j=2;j<=11;j++)

and

for ( j = 2; j <= 11; j++ )

[Yet is not:

for(j = 2; j <= 11; j++)

slightly more readable? Too much space can be as bad as too little.]

All in all the code ends up looking like this:

[snipped]

This code generates the same output as the sample output, except that a space is inserted between the writing of the two array indexes.

It is a good practice to include all the relevant header files (meaning, provide the proper prototypes), even though it is not mandatory as far as the C programming language is concerned. (It is compulsory in C++). For using malloc, include the stdlib.h header file, so that the necessary prototypes are provided to the compiler.

In the absence of an appropriate prototype, the compiler would assume that the return type is an integer (while malloc function actually returns a void pointer). This is fine in a 32-bit system, but will be a problem on a 64-bit system. The reason is that in a 32-bit system, an integer and a pointer occupy the same amount of memory space (four bytes), whereas in a 64- bit system, an integer occupies 4 bytes, but the pointer occupies 8 bytes. So provide prototypes. It might save you a lot of trouble later.

[The above paragraph contains a number of misconceptions. There is no requirement on any system that any integer type have the same amount of storage as any pointer type. Note that not only does both C & C++ allow pointers to different fundamental types to be different in both size and layout, but some implementations actually use this license. The requirements are that void* and char* be identical in size and layout; void* must be able to store the value of any data pointer without loss of information and that all pointers to struct (and, in the case of C++, class) only need a declaration of the type name. Different types of pointer can, and sometimes are, different sizes. Pointers can, and sometimes are laid out differently to the layout of an int.

Lastly, as of the 1999 release of C, implicit int and implicit function declarations are no longer supported. Francis]

An array name is not exactly a pointer [1], even though in many circumstances it degenerates into one. An array is a single entity, in the sense that its name and its memory location are inseparable. When we declare a variable like int a[10]; we have an object that can hold 10 integers and it is named a. There is no other memory associated with that object. When we just use an array name where a pointer is expected (type being appropriate), it is interpreted as a pointer to the first element of the array. In our case, if we use a, where an int* is expected, then it is evaluated as &a[0]. This dirty work is done by the compiler. Also note that this association cannot be changed. For example, we cannot do something like

int a[10];
a = malloc (10);
// *ERROR* cannot reassign another object to a
// a is not a pointer variable.

This is why sizeof(a) gives the size of the array object named a. The size of a in our case is 10*sizeof(int) which is often equal to 40.

The confusion mainly comes because of the applicability of the subscript operator to an array and a pointer pointing to a malloc'ed memory location. For example,

int a[10];
int *p = malloc(10*sizeof(int));
// initialize a[0]...a[9] and p[0]...p[9]
for(int i=0; i < 10; ++i) {
  printf("p[%d]=%d\n", i, p[i]);
  printf("a[%d]=%d\n", i, a[i]);
}

The similarity between these two usages is the main reason for the confusion that an array name is a pointer. But one should remember that the "array" p is not a single entity. There are two entities involved - one is the pointer variable p, and another is the allocated memory that can hold 10 integers. Also the association between the allocated memory and the pointer variable is only temporary, in the sense that the pointer variable can be made to point to another memory location. For example:

int a[10];
int *p = malloc(10*sizeof(int));
// initialize appropriately
p = a; // correct: evaluated as &a[0]
a = p; // error: a cannot be re-assigned.

I leave it as an exercise to the reader to convince himself/herself that an array name is not a const pointer.

The sizeof operator is evaluated at compile time. And it gives the size of the object that is given as an operand. So:

int a[10];
sizeof(a);
// evaluates to the size of 10 integers
char *p = malloc(n);
sizeof(p);
// evaluates to the size of the pointer p
// which is independent of n.

Note that the type of the result of sizeof operator is size_t (which is unsigned int or unsigned long [Or some other unsigned integer type]). So check for the appropriate format specifiers in your system and use it in the printf statements.

In the provided program, the argument to malloc is 10 (which is in unsigned char). The size of an integer [on this system] is 4 bytes. So the value provided to malloc is not in multiples of the size of an int. This most probably will result in hard-to-find bugs (because of memory alignment problems. [Not likely, malloc is required to return suitably aligned memory for any type]) Also the behaviour might depend on the implementation of malloc. Whatever the after effects, the provided code doesn't seem to reflect what the user wants. If the user wants to store 10 integers, then (s)he has to calculate the required size manually, by doing 10*sizeof(int), and passing that to malloc.

If it was C++, things are more clear. For an array of 10 integers, we can just write

int *p = new int[10];

Since the function malloc returns a pointer of type void (a void*), there is no need to cast it explicitly. A void* can be assigned to other pointers without any explicit cast [In C but not in C++]. In C++, it is recommended to use the new for allocating memory, which returns a pointer to the type asked for. operator [Not really, the memory allocation tool in C++ is operator new, the creation of dynamic objects is done with a new expression] Since no generic pointer types are involved, there is no question of casting.

The use of hypotenuse instead of hypo in the last printf statement will result in compilation error. Also if it was C++, the compilation would fail because the return type of main() is omitted. [Also true in C99]

Comments are normally used to explain what a particular piece of code does (if that cannot be easily understood, just by reading it) and why. Comments like // Start of Main Program are really superfluous, and do not add value. Also wrong comments can be very harmful. For example, the comment // Define Libraries is not correct. The header files just provide declarations. Any declared functions are defined elsewhere.

When a prompt needs to be displayed to the user, it is always prudent to flush the standard output (because a prompt normally doesn't terminate with a new line). If this is not done, then it is not guaranteed to be displayed on the terminal, because most of the terminal drivers are line buffered. [But most, if not all, flush stdout before collecting input from stdin.]

There is no validation done on the provided input. The following checks can be performed on the input. Check if they are legal numbers. Check if they are greater than zero.

Check if there has been any error while calling the various functions (at least for scanf, pow and sqrt.) For example, while calling the pow function, it is possible that an overflow occurred. This can be determined by checking the errno variable. Such validations would help to determine the precise cause of a failure.

Also note that scanf doesn't provide much error checking. For example, if a value overflows, scanf doesn't report it. For this reason, a combination of fgets and strtod is preferred. The strtod function returns the error status through the errno variable.

For a double, scanf requires that %lf be specified as the format. [2]

Including a header file doesn't "define" the functions of a library. They just declare them. The function definitions of a library are probably available elsewhere in the system as shared/static libraries. This information should be provided to the compiler (or more precisely, to the linker). Normally the option is '-l' (l as in Link). For more details on this, look at the compiler/linker documentation and man pages.