These are examples of ugly code fragments taken from student's
solutions.   What can be done to make the solutions simpler or cleaner?

These programs were written in a world where style and readability are
not graded, which explains certain glitches.  But it is good to "code
review" to see how things should be done right.

PS: For all complete programs, I have changed a few symbols so these
should not produce the correct answers..


Problem 170: Clock-Patience

#include <stdio.h>

enum face {two, three, four, five, six, seven, eight, nine, T, J, Q, K, A};

enum design {C, D, H, S};

struct card
{
  enum face value;
  enum design suit;
  int hidden;
}; 

...

struct card createcard(char curvalue, char cursuit)
{
  struct card newcard;
  
  newcard.hidden = 1;
  switch(cursuit)
  {
    case 'C':
      newcard.suit = C;
      break;
    case 'D':
      newcard.suit = D;
      break;
    case 'H':
      newcard.suit = H;
      break;
    case 'S':
      newcard.suit = S;

...


  switch(curvalue)
  {
    case '2':
      newcard.value = two;
      break;
    case '3':
      newcard.value = three;
      break;
    case '4':
      newcard.value = four;
      break;
    case '5':
      newcard.value = five;
      break;
    case '6':
      newcard.value = six;
      break;


==> WHAT WAS GAINED BY USING ENUMERATED TYPES???



Problem 151: Power Crisis (For what mod the Josephus permutation leave 13 last?)



//---------------------------------------------------------------------------

#pragma hdrstop
#include <iostream.h>
//---------------------------------------------------------------------------

#pragma argsused
int oneleft(bool* b,int n)
{
	int cnt=1;
	int index=0;
	for (int i=0;i<n;i++)
		if (b[i])
		{
			++cnt;
			index=i;
		}
	return (cnt==1)?index:(!cnt)?-2:-1;
}
void add(bool* b,int size,int& cp,int moves)
{
	while(moves&&oneleft(b,size)!=-2)
	{
		while (!b[++cp%=size]);
		--moves;
	}
	b[cp]=false;
}
void main(int argc, char* argv[])
{
	while(true)
	{
		int in;
		cin>>in;

		bool cit[10];
		cit[0]=0;
		for (int i=1;i<in;++i)
			cit[i]=1;

		if (!in)
			return;
		int m=1;
		int cp=0;

		while(oneleft(cit,in)!=12)
		{
			if (oneleft(cit,in)==-2)
			{
				++m;
				cp=0;
				cit[0]=0;
				for (int i=1;i<in;i++)
					cit[i]=1;
			}
			add(cit,in,cp,m);
		}
		cout<<m<<endl;
	}
}
//---------------------------------------------------------------------------

==> CAN THIS BE READ?  WHAT ABOUT SYMBOLIC CONSTANTS?  COMMENTS?  CUTENESS?
    WHAT ARE THE LOGICAL MEANINGS OF THE TESTS?




Problem 256: Quirksome Squares (which numbers abcd have the property that 
		ab^2 + cd^2 = abcd)


#include <iostream.h>
#include <stdlib.h>


int a2[] = {0, 1, 81, -1};

int a4[] = {0, 1, 2025, 3025, 9802, -1};

..

int main(void)
{
        char str[10000];

        cout.fill('0');

        while(true)
        {
                if (!cin.getline(str, 10000))
                {
                        break;
                }
                        
                int numDigs = atoi(str);
                int * array;

                switch (numDigs)
                {
                case 2:
                        array = a2;
                        break;
                case 4:
                        array = a4;
                        break;
                case 6:
                        array = a6;
                        break;
                case 8:
                        array = a8;
                        break;
                default:
                        cout << "ERROR";
                        return -1;
                }

                while (*array != -1)
                {
                        cout.width(numDigs);
                        cout << *array << endl;
                        array++;
                }
        }

        return 0;
}


      /* Function for creating the quirksome arrays:

                int numCount = 0;
                for (int i = 0; i <= 9999; i++)
                {
                        for (int j = 0; j <= 9999; j++)
                        {
                                if ( (i * j) + (i + j) == (i * 10000) + j)
                                {
                                        out << i * 10000 + j << ", ";
                                        if (numCount > 5)
                                        {
                                                out << "\n";
                                                numCount = 0;
                                        }
                                }

                        }*/


==> SLEAZY PREPROCESSING, USE OF CASE INSTEAD OF ARRAYS, COMPLICATED PARSING


Problem 227: Puzzle (simulate 5*5 tile shift puzzle)


#include <stdio.h>
#define BOARDSIZE	5

main()
{
 int i, j, row, col, puzzle=1, error=0;
 char c, temp;
 char garbage[80];
 char b[BOARDSIZE][BOARDSIZE];
 
...

     for (j=1; j<=BOARDSIZE-1; j++) {
	scanf("%c", &b[0][j]);
	if (b[0][j] == ' ') {	
	    row = 0;
	    col = j;
	}
	else if (b[0][4] == '\n') {
	    b[0][j] = ' ';
	    row = 0;
	    col = j;
	}
     } 

...

     while ((c != '0') && (error != 1)) {
        scanf("%c", &c);
	if (c == 'A') {
	    if (row-1 >= 0) {
	        temp = b[row-1][col];
	    	b[row-1][col] = ' ';
	    	b[row][col] = temp;
	    	row = row-1;
	    }
	    else {
		error = 1;
	    }
	}
        else if (c == 'B') {
	    if (row+1 <= BOARDSIZE-1) {
            	temp = b[row+1][col];
            	b[row+1][col] = ' ';
            	b[row][col] = temp;
            	row = row+1;
	    }
	    else {
		error = 1;
	    }
        }             
	else if (c == 'L') {
	    if (col-1 >= 0) {
            	temp = b[row][col-1];
            	b[row][col-1] = ' ';
            	b[row][col] = temp;
            	col = col-1;
	    }
	    else {
		error = 1;
	    }
        }             
	else if (c == 'R') {
	    if (col+1 <= BOARDSIZE-1) {
            	temp = b[row][col+1];
            	b[row][col+1] = ' ';
            	b[row][col] = temp;
            	col = col+1;
	    }
	    else {
		error =1;
	    }
        }
     }


==> INCONSISENT USE OF CONSTANTS, REDUNDANT CODE IN FOUR CASES



Problem 100:  3n+1 problem


....

public static int[] readTwoNumber(String str)
{
      int x=0;
      int[] answer = new int[2];
      while (str.charAt(x) != ' ')
            x++;
                
      answer[0] = Integer.valueOf(str.substring(0,x)).intValue();
      answer[1] = Integer.valueOf(str.substring(x+1)).intValue();
      return answer;
}
public static void main (String[] args) throws IOException
{
     BufferedReader inputStream = new BufferedReader(new FileReader("prob1.in"));
     PrintWriter outputStream = new PrintWriter(new FileOutputStream("prob1.out"));
                
     String str = inputStream.readLine();
     int[] TwoNumber;
     int maxCycle;
     while (str != null)
     {
           TwoNumber = ThreeN.readTwoNumber(str);
           maxCycle = ThreeN.Calculate(TwoNumber[0],TwoNumber[1]);
           outputStream.println(str + " " + maxCycle);
           str = inputStream.readLine();
     }
     inputStream.close();
     outputStream.close();
   }
}


==> WRITTEN IN JAVA, COMPLICATED I/O, SUCH A SIMPLE PROGRAM SHOULDN'T BE SO LONG




Problem 272: TEX-Quotes

main()
{
   int count = 0, index = 0, i;
   char c, words[80000];

   while ((c = getchar()) != EOF)
      words[index++] = c;
   words[index] = '\0';

   for (i = 1; i < index; i++) {
      if (words[i] != '"')
         printf("%c", words[i]);
      else {
         ++count;
         if (count % 2 != 0)            /* open quote */
            printf("%c%c", '`', '`');
         else
            printf("%c%c", '\'', '\'');
      }
   }
}

==> DON'T USE NO DOUBLE NEGATIVES, DO USE CHARACTER CONSTANTS, NULL TERMINATE?


Problem 127: Accordian Patience (card solitiare with collapsing piles)

#define S 52

struct card{
  char num;
  char suit;
  int pile;
} A[S][S];

void play(struct card A[S][S]);
void add(struct card A[S][S], int i, int j);
void shiftup(struct card A[S][S], int i);
void shiftleft(struct card A[S][S], int i);
void print();

main()
{

...

    while(i < S){

      if(i % (S/2) == (S/2-1)){
	scanf("%c%c\n", &A[0][i].num, &A[0][i].suit);
	A[0][i].pile = 1;
	i++;
      }
      else{
	scanf("%c%c ", &A[0][i].num, &A[0][i].suit);
	A[0][i].pile = 1;
	i++;
      }

    }

...    

}

void play(struct card A[S][S])
{
  int i = 0;
  int x, y;

  while((i < S) && (A[0][i].pile > 0)){

    x = i-3;
    y = i-1;

    if((x >= 0) && ((A[0][i].num == A[0][x].num) || (A[0][i].suit == A[0][x].suit))){

      add(A, i, x);

      if(A[1][i].pile == 0)
	shiftleft(A, i);
      else
	shiftup(A, i);

      i = x;

    }

    else if((y >= 0) && ((A[0][i].num == A[0][y].num) || (A[0][i].suit == A[0][y].suit))){

      add(A, i, y);

      if(A[1][i].pile == 0)
        shiftleft(A, i);
      else
        shiftup(A, i);

      i = y;

    }

...

/* shifts all cards up in column i */

void shiftup(struct card A[S][S], int i)
{
  int temp = 1;

  while(A[temp][i].pile > 0){
    A[(temp-1)][i].num = A[temp][i].num;
    A[(temp-1)][i].suit = A[temp][i].suit;
    A[(temp-1)][i].pile = A[temp][i].pile;
    temp++;
  }

    A[(temp-1)][i].num = A[temp][i].num;
    A[(temp-1)][i].suit = A[temp][i].suit;
    A[(temp-1)][i].pile = A[temp][i].pile;

}


==> CODE DUPLICATION IS A BAD THING.  WHY COPY RECORD FIELDS SEPARATELY?



Problem 175: Keywords (which book titles satisfy which profiles)


#include<stdio.h>
#include<string.h>
void main() {
	char profile[51], title[251][256], ch;
	long numP = 0, numT = 0, distance[51], word, seq, i, x;
	while ((ch = getchar()) != EOF && ch != '#') {
		getchar();
		getchar();
		printf("*");
		if (ch == 'P') {
			numP ++;
			scanf("%d", &distance[numP]);
			getchar();
		}
		else if (ch == 'T') {
			numT ++;
			word = 0;
			while (title[numT][word] != '|') {
				word ++;
				scanf("%s", &title[numT][word]);
				printf("%s\n", title[numT][word]);
			}
			printf("FDFF\n");
		}
		else {}
	}
	for (x = 1; x <= 5; x ++) {
		printf("%s", title[1][x]);
	}
	printf("4\n");
}

==> VERY BRITTLE IO, MEANINGLESS DEBUGGING STATEMENTS