Title: Code Review

Last issue I published some (non-working) novice code for you to review. I am delighted that there were several responses. When I came to format them for publication I found that several had no attached name. Please ensure that when you email material as an attachment that you include your name in the attached file.

The specification clearly states that the most important thing is to initially get the program working. So this will be phase 1. Initially I will not worry about program structure, program style. That will be Phase 2.

#include <ctype.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
void input(void);
void sort(void);
void display(void);
typedef struct perspost perspost;
struct perspost {
  int pnumber;
  char forname[10];
  char lastname[15];
};

typedef enum { false, true } bool;
int main(void) {
  bool quit = false;
  
  do {
    char val_buffer[32];
    int val;
    puts("  MENU ");
    puts("0......Quit");
    puts("1......Input people to register");
    puts("2......Sort the register");
    puts("3......Display the register");
    fputs("Val: ", stdout);
  
    fgets(val_buffer, sizeof(val_buffer), stdin);
    sscanf(val_buffer, "%d", &val);
    switch (val) {
    case 0: quit = true;   break;
    case 1: input();       break;
    case 2: sort();        break;
    case 3: display();   break;
    }
  } 
  while (!quit);
  return 0;
}
//==============================
const char datafile[] = "fil.txt";
void cwrite_person(const perspost * bod){
  printf("%d ", bod->pnumber);  
  fputs(bod->forname, stdout);  
  fputs(" ", stdout);
  fputs(bod->lastname, stdout);
  putchar('\n');
}

void cread_person(perspost * bod) {
  char buffer[64];
  fputs("personnumner(personalnumber): ", stdout);
  fgets(buffer, sizeof(buffer), stdin);
  sscanf(buffer, "%d", &bod->pnumber);
  
  fputs("Fornamn(forname): ", stdout);
  fgets(buffer, sizeof(buffer), stdin);
  sscanf(buffer, "%s", bod->forname);
  fputs("Efternamn(lastname): ", stdout);
  fgets(buffer, sizeof(buffer), stdin);
  sscanf(buffer, "%s", bod->lastname);
}

bool input_another(void){
  char buffer[32];
  fputs("Another person? (y/n) : ", stdout);
  fgets(buffer, sizeof(buffer), stdin);
  return tolower(buffer[0]) == 'y';
}

void input(void) {
  FILE * my_file = fopen(datafile, "ab");
  do {
    perspost bod;
    cread_person(&bod);
    fwrite(&bod, sizeof(bod), 1, my_file);
  }
  while (input_another());
  fclose(my_file);
}

int persons_in(FILE * src) {
  fseek(src, 0, SEEK_END);
  return ftell(src) / sizeof(perspost);
}

void swap_person(perspost * p1, perspost * p2) {
  perspost tmp = *p1;
  *p1 = *p2;
  *p2 = tmp;
}

void fwrite_person(const perspost * bod, int pos, FILE * dst) {
  fseek(dst, pos * sizeof(*bod), SEEK_SET);
  fwrite(bod, sizeof(*bod), 1, dst);
}

void fread_person(perspost * bod, int pos, FILE * src) {
  fseek(src, pos * sizeof(*bod), SEEK_SET);
  fread(bod, sizeof(*bod), 1, src);
}
//--------------------------------
void sort(void) {
  FILE * my_file = fopen(datafile, "r+b");
  const int storlek = persons_in(my_file);
  int k;
  for (k = 0; k < storlek; k++) {
    perspost pos1;
    int m;
    fread_person(&pos1, k, my_file);
    for (m = k+1; m < storlek; m++) {
      perspost pos2;
      fread_person(&pos2, m, my_file);
      if (pos1.pnumber > pos2.pnumber){
        swap_person(&pos1, &pos2);
        fwrite_person(&pos1, k, my_file);
        fwrite_person(&pos2, m, my_file);
      }
    }
  }
  fclose(my_file);
}

void display(void) {
  FILE * my_file = fopen(datafile, "rb");
  const int storlek = persons_in(my_file);
  int n; 
  for (n = 0; n < storlek; n++) {
    perspost bod;
    fread_person(&bod, n, my_file);
    cwrite_person(&bod);
  }
  puts("END\n\n");
}

My approach to the code presented in the Code Critique Competition is intended to be as minimal as possible. I have tried to explain why the code does not work and what needs to be done to get it working with as few changes as possibly. I have specifically avoided questions relating to the design of the program and to the solution adopted by the author because I think it would be unhelpful to talk about the use of qsort() or the finer points of scanf() when the author is probably more interested in why his or her code doesn't work the way it is written.

My initial concern with the problem as presented is that the author does not seem to be clear about what he or she wants to do. A certain amount of forethought is required before writing any computer program and, if you can't express what you want to do clearly in whatever your written language happens to be, you are unlikely to be able to express it clearly in C. The first step is to specify the problem precisely:

Given the small size of the program, it is probably a good idea to put this into the code as a comment.

The next step is to reformat the code to make it more readable. This can be a very sensitive area involving endless debate about such fascinating issues as where to put braces, how much to indent by, and what naming convention to follow. The only rules that everyone I know agree on are 'Use comments and blank lines to divide your code into logical chunks' and 'Indent blocks of code consistently'. In keeping with my minimalist approach, I have attempted to follow the author's formatting and naming conventions while making the code more readable^[1].

After the code has been reformatted to make it more readable, it is a good idea to get the data structures right. I have specified the personal number as being 15 digits long but the structure as it is defined in the code only allows for 14 digits. When declaring character arrays in C, you must always reserve one space for the null character at the end of a string. So the structure declaration becomes:

#define S_PNUMBER 15
#define S_FORNAME 10
#define S_LASTNAME 15
struct t_perspost {
  char pnumber[S_PNUMBER+1];
  char forname[S_FORNAME+1];
  char lastname[S_LASTNAME+1];
  };
typedef struct t_perspost PERSPOST;
#define S_PERSPOST sizeof(PERSPOST)

Once this has been done, references to perspost' can be replaced with PERSPOST' which makes it clear that this is a user-defined structure. References to sizeof(persoft), sizeof(preg), and sf can be replaced with S_PERSPOST which is at least consistent throughout the code.

Next we move on to the functions that make up the program. main() calls the function meny() which displays a menu to the user. On return from meny(), however, there is a very disconcerting call to getch() which will hang the program until the user remembers to hit enter. Presumably the user has indicated his or her intention to exit the program within the function meny() so this call can be removed.

The function meny() displays a menu to the user prompting him or her for an action. At present, the function runs once and then drops through to return to main. It might be better to allow the user to select several operations before exiting and to provide an explicit option to exit the program rather than just dumping him or her back at the system prompt particularly if an invalid option is selected. This implies some kind of menu loop that will prompt the user for an option until a specified exit option is selected. I would also prefer to see the selected option being read as a character rather than a number because this allows the exit option to be given an obvious mnemonic such as 'q' instead of having to assign it to '0' and putting it first in the list or last in the list (which looks awkward) or having to assign it to '3' and then changing the number every time a new menu option is added. The modified meny() is as follows:

// meny - user menu
void meny() {
int val;

do  {
  clrscr();
  printf(" MENY" );
  puts("");
  puts("");
  printf("1......Add people to the register\n");
  printf("2......Print\n");
  printf(""q......Exit\n");
  printf("Val: ");
  val = getch();
  switch(val) {
    case '1':  lasin();   break;
    case '2':  sortera(); break;
  }
} while( (val != 'q') && (val != 'Q') );
return;
} // end meny

At this point I should note that the functions 'asin() and utskrift() both make calls to meny(). I think that the author's intention is to return to the main menu at these points whereas what will actually happen is that a new menu will be created recursively. These calls should be eliminated and the flow of control should be allowed to return to the original menu.

The function lasin() prompts the user to enter a filename but, since the name fil.txt has been hard-coded into both sortera() and utskrift(), I think it better to specify that the program will always work with a file called fil.txt for the moment. An extension to the specification might be to allow the user to work with different files and to decide whether to enter the file name on the command line or to prompt the user for a file name before entering meny() or to allow the user to change the file name via a menu option but, as the program stands, it can only work with a single file called fil.txt. The simplest solution is to use the pre-processor to define REGISTER-FILE as fil.txt and to get the program working before adding bells and whistles. (It also gets rid of the rather curious declaration of namn[] in lasin(). If this is a DOS program then the length of namn[] must be at least 13 to hold an 8.3 format filename.)

At present, lasin() opens the register file using the mode "wb" which means that, every time the file is opened, it will overwrite an existing file of the same name if one exists. If the author intends to add more people to an existing file, the correct mode is "ab+" which opens the file to append data to the end if it exists or creates a new file if it does not. Given that records can now be appended to the file using calls to fwrite(), we can eliminate the call to fseek() as the internal file positioning is automatically taken care of by the standard library. It is also a good idea to check return codes from standard library calls just to make sure that nothing has gone wrong.

In order to determine whether the user has finished entering personal details, the author converts the personal number into an integer using atoi() and compares this value with zero. Since a personal number can consist of up to fifteen digits and the behaviour of atoi() is undefined if it is given a number that is too big, this could lead to unexpected results. It would be better to compare the personal number as a string with "0".

The modified version of lasin() looks like this:

//. lasin - input and store personal information
void lasin(){
PERSPOST preg;
FILE *my_fil;
size_t n_written;

clrscr();
my_fil=fopen(REGISTER_FILE,"ab+");
if ( my_fil ){
  do {
    printf("personnummer(personalnumber): ");
    scanf("%s",preg.pnumber);
    if( strcmp( preg.pnumber, "0" ) != 0 ){
      printf("Fornamn(forname): ");
      scanf("%s",preg.forname);
      printf("Efternamn(lastname:");
      scanf("%s",preg.lastname);

      n_written = fwrite(&preg,S_PERSPOST,1,my_fil);
      if ( n_written != 1 ){
        printf( "error: n_written %d\n",  
                      n_written );
        }
      }
    } while( strcmp( preg.pnumber, "0" ) != 0 );
  fclose(my_fil);
  }
return;
} // end lasin

In order to check that the file has been written correctly, it would be useful to be able to print it out without trying to sort it. For testing purposes, I added an extra option to the menu which calls the function utskrift() to print the file directly to the screen. This function makes a lot of calls to fseek() that are not strictly necessary because, after a call to fread(), the file will be left ready to read the next record. So the main loop of utskrift() can be replaced by a loop which calls fread() until there is nothing more to read:

// utskrift - print personal information
void utskrift(){
FILE *my_fil;
PERSPOST preg;

clrscr();
my_fil=fopen(REGISTER_FILE,"rb");
if ( my_fil ){
  while(fread(&preg,S_PERSPOST,1,my_fil)){
    printf("%s\n",preg.pnumber);
    printf("%s\n",preg.forname);
    printf("%s\n",preg.lastname);
    }
  fclose(my_fil);
  }
getch();
return;
} // end utskrift

Last of all there is sortera() the function that sorts the file. I am not certain which sorting algorithm the author intended to use - a comment would have been helpful - but I have decided to implement a selection sort which scans through the file repeatedly, finds the smallest record, and moves it to its correct position in the file.

The first problem with sortera() is that it opens the file in read-only mode. Since the intention is to read and write the file, the correct mode in "rb+". The function then correctly determines the number of records in the file and stores the value in the variable storlek. The outer loop of the sort algorithm contains two off-by-one errors; the first record in the file is actually at offset 0 and, since the inner loop will move forward through the file from the position after the current offset of the outer loop, the outer loop does not need to iterate to the very end of the file. The inner loop of the sort algorithm contains a similar error and will attempt to read beyond the end of the file.

The basis of the selection sort algorithm is to find the smallest remaining record on each pass through the inner loop and to swap it with the record currently pointed to by the outer loop. In order to do this, the algorithm must store both the current smallest record and its position in the file. The inner loop scans from the position after the current outer loop position to the end of the file and, whenever it finds a record smaller than the current smallest record, it saves the new smallest record and its position. After the inner loop is complete, the algorithm checks whether the current outer loop record is the smallest and, if it is not, swaps the current record with the smallest record.

The final version of sortera() omits error checks on the library calls to keep the length of the code short enough to print:

// sortera - sort and print personal information
void sortera(){
FILE *my_fil;
int storlek,k,m;
long ft;
PERSPOST pos_current;
PERSPOST pos_smallest;
PERSPOST pos_new;
int smallest;

my_fil=fopen(REGISTER_FILE,"rb+");
if ( my_fil ){
  fseek(my_fil,0,SEEK_END);
  ft=ftell(my_fil);
  storlek=((int)ft)/S_PERSPOST;

  // Selection sort
  for(k=0;k<storlek-1;k++){
    // Read the first of the remaining records
    fseek(my_fil,k*S_PERSPOST,SEEK_SET);
    fread(&pos_current,S_PERSPOST,1,my_fil);

// Scan forward through the file looking for the
//  smallest record
    pos_smallest = pos_current;
    smallest = k;
    for(m=k+1;m<storlek;m++){
      fread(&pos_new,S_PERSPOST,1,my_fil);

      // If this record is the smallest, 
      // save it and remember where it is in the file
      if(strcmp(pos_smallest.pnumber,
                       pos_new.pnumber)>0){
        pos_smallest=pos_new;
        smallest=m;
        }
      }

    // If the first record isn't the smallest, 
    // swap it with the smallest
    if (smallest != k ){
      fseek(my_fil,k*S_PERSPOST,SEEK_SET);
      fwrite(&pos_smallest, S_PERSPOST, 1,
                                       my_fil);
      fseek(my_fil, smallest*S_PERSPOST,
                              SEEK_SET);
      fwrite(&pos_current, S_PERSPOST, 1, 
                                       my_fil);
      }
    }
  fclose( my_fil );
  }

utskrift();
} // end sortera

Dear Francis,

I thought I would have a go at the Code Critique Competition you set in C Vu Volume 11 No 4. I have done the best I can in the time available but please do not hesitate to show me where I have gone wrong.

As you suggest it is helpful to rewrite the code to correctly show the program structure. Such things as indentation and the position of braces are a matter of personal taste. I prefer to have the opening and closing braces vertically aligned at the same indentation as the controlling statement. The code within the braces is then indented by two spaces. I notice that the student puts the opening brace at the end of the controlling statement, as do many experts, but I have never been very happy with this approach.

It also makes the code easier to read if there are appropriately placed spaces within the statements. It gets a bit heavy going when the code is constructed without spaces to separate the various terms and clauses^[2].

I think one reason the student has got in a bit of a muddle is the way he or she is trying to return to the menu function. Instead of returning from a called function the student is indirectly recursively calling menu(). Recursive calls are confusing at the best of times and, in this case, it is not what is required.

Two things have to be done to correct the problem. The first is to remove the recursive calls to menu(). The correct way to return to the menu() function is to place a return instruction in the called function. We now have the following situation. The menu() function calls, say, lasin(). When lasin() has finished doing its work the program will return to menu() in an orderly fashion. The menu() function will finish its work and return to main() where the program will terminate. The program is retracing its steps, it returns the way it came.

The only problem now is that only one menu() function is executed before the program ends. We now have to make the second amendment to the program. The menu() function needs to execute again and again. I have surrounded the menu() code with a while(1) loop. I have also added an 'exit' option to the menu. The user now has to choose the additional option to exit the program. An if-statement returns the program to main() only if the 'exit' option is selected. This prevents the program from exiting after the selection of only one menu option.

I have provided a way of detecting a value of '0' for the personnel number. I have placed a while-loop around the data entry code. The loop will only execute when the variable proceed is true. The variable proceed is first set true outside the loop so the loop will always execute the first time. If, within the loop, the user enters a '0' for the personnel number the proceed variable will be set to false. There is also no need for the user to enter any more personnel information so the rest of the data entry code is skipped. When the program next executes the while-statement the proceed variable will be false and so the loop will not execute. The register file is closed and the program returns to main(). In this way the loop will execute repeatedly allowing the user to add many records until a '0' for the personnel number is entered.

When rewriting the code I noticed that the function prototypes have empty prototype lists. This causes the (C) compiler not to recognise them as prototypes. The compiler issues a warning stating that there is a call to a function with no prototype. The parameter list (in a C program) must contain the parameter types, even if they are of type void.

There seems to be some very strange spelling used for variable names^[3]. There were quite a few I could not fathom. To aid my understanding of what is going on I decided to rename most of the variables. I gave them names that would be more informative to me. I think it is important to give clear and concise names to variables. This not only helps the program writer but other people when they come to review the code.

It is not necessary to use printf() to print simple text strings, use puts() instead. puts() automatically inserts a carriage return after the string so we can now remove the \n from some of the text strings. Successive carriage returns can be achieved by the use of successive \n in the text string of puts().

I think the text of the menu prompt is a little enigmatic. It would be best to state clearly what the user is expected to do. This time printf() is used because we do not want a carriage return at the end of the prompt^[4].

The menu is now in a reasonable state. There is still one problem. If the user types a non numerical value scanf() will not remove the user input from its buffer. The program will loop around to scanf() which will again see a non numerical value. The program will loop forever. To prevent this behaviour all we have to do is remove any characters from the input buffer. This is carried out by the fflush(stdin) statement. (I wonder if the writer checked this. fflush is specifically for output streams or for bi-directional streams where output is the most recent event. Francis)

We can now turn our attentions to the lasin() function. I have renamed this function add_record.

When adding a record, the program asks for a file name. The prompt states that the name of the file must be fil.txt. If the file name must be a fixed value there is no need for the user to enter it. The name of the file can be embedded within the code. This removes the need for the namn[10] array. I have changed the name of the file to register.dat as I think it more appropriate. The file is not a text file, it is a binary file.

When adding a record to the register file I assume the idea is to append a record to the end of the file if the file already exists or, if the file does not exist, create a new register file and then add the record. To do this we need to open the file in "a" mode and not "w" (write) mode as in the student's program.

When adding records to the register file it does not matter whether the record is inserted at the start of the file or at the end. The file will be sorted before it is printed in any case. There is, therefore, no need to use the fseek() function to place the new record at the beginning of the file. A simple fwrite() will suffice.

We are now in a position to look at the functions that display the register file. Sorting routines are always a bit difficult to get working, so I will get the printing function working first. I have changed the name of function that displays the contents of the register file from utskrift to display.

The utskrift() function is far too complicated. There is no need to work out the length of the file, calculate the number of records in the file and then seek each record. Simply open the file and write out successive records until the end of file is reached. The method I have chosen is to create a loop with a while(1)-statement. This would loop forever if it were not for the if-statement within the loop. The program breaks out of the loop when the end-of-file marker is true. This arrangement caters for the condition when the file is empty.

It is now time to tackle the sort routine. I thought this routine would need a lot of reworking to get it into shape. As it turns out there is not much wrong with it. It would be helpful if a comment is added describing the type of sort used. In this case a bubble sort is used.

A bubble sort is not very efficient for a large number of records but will be acceptable in this case. The header file stdlib.h enables a sorting routine called qsort to be used^[5]. It would have probably been better to use qsort from the beginning but developing your own sort routine will be instructive.

The first problem is that the sorting routine opens the file for reading only. We will need to open it for update (reading and writing) and so the mode string should be "r+b".

When sorting we need to consider the first record so k should be initialised to 0 in the for-loop. The inner loop should iterate between m+k, and the last record in the file. The student tries to go one beyond the last record. The limiting condition should be m < storlek. The student also omitted to close the file at the end of the sort routine.

With these corrections in place the program should work as intended. There will always be various improvements and alterations that could be made to a program. In the student's program, for example, there is a lot of opening and closing of the register file. The program could be designed to open the file once at the beginning of the program and then close it at the end of the program. The method adopted by the student is perfectly acceptable, though.

It is worth noting that the sort routing does not sort in numerical order. It sorts in lexicographical, or dictionary, order. If this is what the student intended all well and good. The personnel number is stored as a character string and so there is nothing to prevent any character from being entered. The same applies for the forename and last name.

I have made one important modification to the student's program. I have attempted to prevent excessive user input from overflowing the character arrays. I have limit the scanf() function to accepting up to one character less than the length of the array. The last character of the arrays is needed for the null terminator character. Any additional characters are purged from the input buffer by the fflush(stdin) statements (did you test that assertion?). It is always important to guard against the abuses of the user.

There is no need to include the header file io.h. I have also removed the getch() from just before the return statement in main^[6]. This makes the program exit when expected.

It is always a good idea to test for the successful opening of a file. This can save a lot of time when debugging the program. It will also prevent the program from continuing when there are problems such as a full disc.

To check for the successful file opening simply compare the value of the file pointer after the attempt to open the file. This can be achieved by an if-statement wrapped around the ffopen statement as shown in the listing. ffopen() will assign to the file pointer a NULL value if the attempt to open the file failed. This value is tested in the if-statement. If the file pointer has the value NULL a short warning message is displayed and the program exits.

I provide my version of the program on the disc enclosed. Please keep the disc.

I have enjoyed tackling this problem and hope my efforts are of some interest^[7].

As you rightly point out, it is time for someone other than the few regular contributors to do all the work. Attached is my critique of the code from C Vu 11 No.4. It has been an interesting exercise and I feel that I have learned a few extra things on the way. Thanks for the opportunity.

Fred Johnson. <fred@fredshome.freeserve.co.uk>

lasin();
my_fil = fopen(namn,"wb");

sortera()

if(strcmp(pos1.pnumber, pos2.pnumber) > 0)

#ifndef mycode    /* suppress multiple includes */
int main(void);
int lasin(void);
int meny(void);
void paintscreen(void);
int sortera(void);
int avsluta();
int utskrift(void);

#define pnumberlength 15
#define fornamelength 10
#define lastnamelength 15

const int esckey = 27; 
/* ascii escape char. change for other conventions */

#define mycode

#endif

#include <stdio.h>
#include <string.h>
#include <conio.h>
#include <stdlib.h>
#include <io.h>
#include "code.h"
#ifndef ATARI
  #include <dos.h>
#else
  #include "d:\csource\tcheader\turbo.h"
  #include "d:\csource\tcheader\vtscreen.h"
#endif

typedef struct{
  char pnumber[pnumberlength];
  char forname[fornamelength];
  char lastname[lastnamelength];
} perspost;

int main(void){
  meny();
  return 0;
}

int meny(void){
  int val =0;
  paintscreen();
  do {
    val = getch();
    if(val==esckey) break
/* Trap esckey here since defined values*/ 
/* can't be used in case statements */

    switch(val) {
    case '1': lasin(); break;
    case '2':   sortera(); break;
    default:
      printf("Invalid Input ** %c **  \n",val);
      sleep(1);
      paintscreen();
       break;
    }
  } while (val!=esckey); 
    return 0;
}

int lasin(void){
perspost preg;
FILE *my_fil;
char namn[11];/* 10 char filename + extra for '\0' at end of name */
long i=0,ptr=1;
  clrscr();
  printf(
    "Filename(must be named 'fil.txt :-)?:");
  scanf("%10s", namn); 
/* allow 10 chars only */
  if((my_fil=fopen(namn,"wb")) == NULL){
      puts("Failed to open file");
      sleep(1);
      paintscreen();
      return 0;
    }
  fseek(my_fil,0,SEEK_SET);
  clrscr();

/*  while(ptr !=0) */
  do  {
    puts("Personnummer(personalnumber): ");
     puts("Enter -1 to end ");
    scanf("%14s",preg.pnumber);
    ptr = atoi(preg.pnumber);
    if(ptr == -1)
      break;
    printf("Frnamn(forname): ");
    scanf("%9s", preg.forname);
    printf("Efternamn(lastname: ");
    scanf("%14s",preg.lastname);
/* fseek(my_fil,i*sizeof(perspost),
                   SEEK_SET);  not needed */
        fwrite(&preg,sizeof(perspost),1,my_fil);
    i++;
  }
    while(ptr!=-1);
  printf("Last entry\n");
  fclose(my_fil);
  sleep(1);
  paintscreen();
  return 0;
}

int sortera(void){
  FILE *my_fil;
  char myinput;
  size_t data_length, file_length, storlek,   outercount,innercount;
  perspost pos1, pos2, tmp;
  /* open file for read and write without truncating file */
  my_fil = fopen("fil.txt","rb+"); 
  puts("File opened");
  fseek(my_fil,0,SEEK_END);  
/* go to end of file */
  file_length = ftell(my_fil);  /* find filelength */
  data_length = sizeof(perspost);
  storlek = file_length/data_length;
  puts("Starting sort "); 
/* counter should start from 0 for first entry */ 
  for(outercount = 0; outercount<storlek; 
                  outercount++){
    fseek(my_fil, outercount*data_length, 
                    SEEK_SET);
    fread(&pos1,data_length,1,my_fil);
    for(innercount = outercount+1;
             innercount<storlek; innercount++){
      fseek(my_fil, innercount*data_length, 
                      SEEK_SET);
      fread(&pos2,data_length,1,my_fil);
      if(strcmp(pos1.pnumber, 
                    pos2.pnumber)>0){
        tmp=pos1;pos1=pos2;pos2=tmp;
        fseek(my_fil, outercount*data_length, 
                      SEEK_SET);
        fwrite(&pos1,data_length,1,my_fil);
        fseek(my_fil, innercount*data_length, 
                      SEEK_SET);
        fwrite(&pos2,data_length,1,my_fil);
      }
    }
  }
  puts("End of sortera"); 
  fclose(my_fil);
  sleep(1);
  utskrift();
  paintscreen();
  return 0;
}

int utskrift(){
  FILE *my_fil;
  long counter=0
  long data_length, file_length, storlek;
  perspost preg;
  clrscr();
  my_fil = fopen("fil.txt","rb");
  fseek(my_fil,0,SEEK_END); 
  file_length = ftell(my_fil);
  data_length=sizeof(perspost);
  storlek=file_length/data_length;
  while(counter<storlek) {
/* note, you do not need to recalculate size of preg, 
   use data_length which has already been calculated */ 
  
    fseek(my_fil,counter*data_length,
                       SEEK_SET);
    fread(&preg,data_length,1,my_fil);
    puts(preg.pnumber);
    puts(preg.forname);
    puts(preg.lastname);
    counter++;
  }
  fclose(my_fil);
  puts("END of print, press any key");
  getch(); /* stop here to read screen */
  return 0;
}

void paintscreen(void) {
  clrscr();
  printf("MENY");
  puts("");
  puts("");
  puts("Select required function");
   puts("1......Add People to the register");
  puts("2......Print");
  puts("ESC....Exit");
}

Four reviews, four different approaches and each one highlights different aspects. Each also has some flaws (in some places I have added footnotes or inserted comments, but not always, and only to steer novice readers away from some miss-understandings). I wonder which one you would award the prize to. Well, if the writer of the first item will contact me we can discuss a prize.

My thanks to the other three contributors for their efforts. If anyone would like to review the reviews please do so.

Elsewhere you will find an article on writing a program to validate ISBNs. The writer prepared this program as a student project. Please review his code and prepare an alternative solution. Extra credit will be given for providing solutions in more than one language and commenting on the relevant merits of each language to the problem.