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.