next up previous
Next: About this document Up: My Home Page

Doing the Shuffle
Lecture 8

Steven S. Skiena

Programming Style

Although programming style (like writing style) is a somewhat subjective thing, there is a big difference between good and bad.

The good programmer doesn't just strive for something that works, but something that works elegantly and efficiently; something that can be maintained and understood by others.

Just like a good writer rereads and rewrites their prose, a good programmer rewrites their program to make it cleaner and better.

To get a better sense of programming style, let's critique some representative solutions to the card-shuffling assignment to see what's good and what can be done better.

Ugly looking main

MODULE card EXPORTS Main;
IMPORT SIO;
TYPE
 index=[1..200];
 Start=ARRAY index OF INTEGER;
 Left=ARRAY index OF INTEGER;
 Right=ARRAY index OF INTEGER;
 Final=ARRAY index OF INTEGER;
VAR
    i,j,times,mid,k,x: INTEGER;
    start: Start; 
    left: Left;
    right: Right;
    final: Final;
BEGIN
     SIO.PutText("deck size   shuffles\n");
     SIO.PutText("---------   --------\n");
     SIO.PutText("  200   ");
     SIO.PutInt(           times    );

REPEAT               (*Repeat the following until perfect shuffle*)

     i:=1;            (*original deck*)   
     WHILE i<=200 DO 
     start[i]:=i;
     i:=i+1;
     END;
  
     j:=1;           (*splits into two decks*)
     mid:=100;
     WHILE (j<=100) DO
     left[j]:=start[j];
     right[j]:=start[j+mid];
     j:=j+1;
     END;  
     x:=1;
     k:=1;           (*shuffle them into one deck*)       
     WHILE k<=200 DO     
      final[k]:=left[x];
      final[k+1]:=right[x];
      k:=k+2;
      END;

UNTIL  start[2]=final[2]; (*check if complete shuffle*)

times:=times+1;
END card.

There are no variable or block comments. This program would be hard to understand.

This is an ugly looking program - the structure of the program is not reflected by the white space.

Indentation and blank lines appear to be added randomly.

There are no subroutines used, so everything is one big mess.

See how the dependence on the number of cards is used several times within the body of the program, instead of just in one CONST.

What Does shufs Do?

PROCEDURE shufs( nn : INTEGER )=  		(* shuffling procedure *)

	VAR
	i : INTEGER; 				(* index variable *) 
	count : INTEGER; 			(* COUNT variable *)

	BEGIN

	FOR i := 1 TO 200 DO  			(* reset this array *)
		shuffled[i] := i;
	END;

	count := 0; 				(* start counter from 0 *)

	REPEAT
		count := count + 1;
		FOR i := 1 TO 200 DO 		(* copy shuffled -> tempshuf *)
			tempshuf[i] := shuffled[i];
		END;

		FOR i := 1 TO nn DO 		(* shuffle 1st half *)
			shuffled[2*i-1] := tempshuf[i];
		END;

		FOR i := nn+1 TO 2*nn DO 	(* shuffle 2nd half *)
			shuffled[2*(i-nn)] := tempshuf[i];
		END;

	UNTIL shuffled = unshuffled ; 		(* did it return to original? *)

						(* print out the data *)
	Wr.PutText(Stdio.stdout , "2*n= " & Fmt.Int(2*nn) & "   \t" );
	Wr.PutText(Stdio.stdout , Fmt.Int(count) & "\n" );

END shufs;

Every subroutine should ``do'' something that is easily described. What does shufs do?

The solution to such problems is to write the block comments for the subroutine does before writing the subroutine.

If you can't easily explain what it does, you don't understand it.

How many comments are enough?

MODULE Shuffles EXPORTS Main;
IMPORT SIO;

TYPE
  Array= ARRAY [1..200] OF INTEGER;  (*Create an integer array from *)
                                     (*1 to 200 and called Array    *) 
VAR
   original, temp1, temp2: Array;    (*Declare original,temp1 and   *)
                                     (*temp2 to be Array            *)
   counter: INTEGER;                 (*Declare counter to be integer*)

(********************************************************************)
(*  This is a procedure called shuffle used to return a number of   *)
(*  perfect shuffle.  It input a number from the main and run the   *)
(* program with it and then return the final number of perfect shuffle *)
(********************************************************************)

PROCEDURE shuffle(total: INTEGER) :INTEGER =
VAR
   half, j, p: INTEGER;               (*Declare half, j, p to be integer *)
BEGIN                                     
   FOR j:= 1 TO total DO           
      original[j] := j;
      temp1[j] := j;
   END; (*for*)
   half := total DIV 2;
   REPEAT
      j := 0;
      p := 1;
      REPEAT
         j := j + 1;
         temp2[p] := temp1[j];  (* Save the number from the first half  *) 
                                (* of the original array into temp2     *)
         p := p + 1;
         temp2[p] := temp1[half+j]; (* Save the number from the last half*)
                                    (* of the original array into temp2  *)
         p := p + 1; 
      UNTIL p = total + 1; (*REPEAT_UNTIL used to make a new array of temp1*)
      INC (counter);       (* increament counter when they shuffle once *)
      FOR i := 1 TO total DO
         temp1[i] := temp2[i];
      END;   (* FOR loop used to save all the elements from temp2 to temp1 *)
   UNTIL temp1 = original;   (* REPEAT_UNTIL, when two array match exactly *)
                             (* same then quick *)
   RETURN counter;  (* return the counter *)
END shuffle; (* end procedure shuffle *)

(********************************************************************)
(* This is the Main for shuffle program that prints out the numbers *)
(*      of perfect shuffles necessary for a deck of 2n cards        *)
(********************************************************************)

BEGIN
   ...
END Shuffles.  (* end the main program called Shuffles *)

This program has many comments which should be obvious to anyone who can read Modula-3.

More useful would be enhanced block comments telling you what the program is done and how it works.

The ``is it completely reshuffled yet?'' test is done cleanly, although all of the 200 cards are tested regardless of deck size.

The shuffle algorithm is too complicated. Algorithms must be pretty, too

MODULE prj1 EXPORTS Main;
IMPORT SIO;
CONST
   n : INTEGER = 100;        (*size of split deck*)

TYPE
   nArray = ARRAY[1..n] OF INTEGER;          (*n sized deck type*)
   twonArray = ARRAY[1..2*n] OF INTEGER;     (*2n sized deck type*)

VAR
   merged : twonArray;                       (*merged deck*)
   count : INTEGER;


PROCEDURE shuffle(size:INTEGER; VAR merged:twonArray)=
VAR
   topdeck, botdeck : nArray;                (*arrayed split decks*)
BEGIN
   FOR i := 1 TO size DO
      topdeck[i] := merged[i];               (*split entire deck*)
      botdeck[i] := merged[i+size];          (*into top, bottom decks*)
   END;
   FOR j := 1 TO size DO
      merged[2*j-1] := topdeck[j];        (*If odd then 2*n-1 position.*)
      merged[2*j] := botdeck[j];          (*If even then 2*n position*)
   END;
END shuffle;


PROCEDURE printout(count:INTEGER; size:INTEGER)=
BEGIN
   SIO.PutInt(size);
   SIO.PutText("    ");
   SIO.PutInt(count);
   SIO.PutText(" \n");
END printout;


PROCEDURE checkperfect(merged:twonArray; i:INTEGER) : BOOLEAN=
VAR 
   size : INTEGER;
   check : BOOLEAN;
BEGIN
   check := FALSE;
   size := 0;
   REPEAT
      INC(size, 1);                               (*check to see if*) 
      IF merged[size+1] - merged[size] = 1 THEN   (*deck is perfectly*)
         check := TRUE;                           (*shuffled, if so *)
      END;                                        (*card progresses by 1*)
   UNTIL (check = FALSE OR size - 1 = i);
   RETURN check;
END checkperfect;

Checkperfect is much more complicated than it need be; just check whether merged[i] = i. You can return without the BOOLEAN variable.

A good thing is that the deck size is all a function of a CONST.

The shuffle is slightly wasteful of space - two extra full arrays instead of two extra half arrays.

Why does this work correctly?

BEGIN
   SIO.PutLine("Welcome to Paul's card shuffling program!");
   SIO.PutLine("           DECK SIZE      NUMBER OF SHUFFLES  ");
   SIO.PutLine("           _________________________________  ");
   num_cards := 2;
   REPEAT
	counter := 0;
	FOR i := 1 TO (num_cards) DO
		deck[i] :=i;
	END; (*initializes deck*)
        REPEAT
           deck := Shuffle(deck,num_cards);	
           INC(counter);
        UNTIL   deck[2] = 2;
        SIO.PutInt(num_cards,16);  SIO.PutInt(counter,19); 
        SIO.PutText("\n");
        INC(num_cards,2);
          (*increments the number of cards in deck by 2.*)  
   UNTIL  ( num_cards = ((2*n)+2));    
END ShuffleCards.

Why we know that this stopping condition suffices to get us all the cards in the right position. This should be proven prior to use.

Why use a Repeat loop when For will do?

Program Defensively

I am starting to see the wreckage of several programs because students are not building their programs to be debugged.




next up previous
Next: About this document Up: My Home Page

Steve Skiena
Thu Sep 25 19:19:43 EDT 1997