Arbtirary thoughts on nearly everything from a modernist poet, structural mathematician and functional programmer.

Wednesday, December 12, 2007

Bad Coding, Bad teaching. OR: Why I want to be a professor

Edit: It seems this site hates my whitespace.... Sorry

I've been helping a friend with CS homework the last few weeks. He's taking "Object Oriented Programming I", which is the intro to programming class for CS and related majors, taught in Java.

I've been helping him because his professor is a hack, an absolute hack. Were he a brilliant man, but a bad teacher (e.g. Calinscu), or an idiot who knew how to teach, it would be acceptable, but he cannot code well and he cannot think well.

Firstly, my friend said "I do not know what object programming is. I don't know how object oriented programming languages work." Investigation shows that the problem is not on my friend's end, because he's understood everything I've taught him. It's that the subject has not come up in his lectures. He has mentioned "template classes", which is a bit redundant....

But I digress. Code samples and critique! These are the professor's, not my friend's... (Some variable names have been changed, but the logic is precisely the same.) The numbers in parenthesis mean I'm going to comment on the line after the segment.

First, a BubbleSort (Copied from a handout):

public int [] bubbleSort (int [] array){
(1) boolean done=false;
(2) int [] arraySorted=array;
(1) int p=0;
(1) int temp=0;
(3) while(!done&&p<=arraysorted.length-1){ (4) done="true"; (5) for(int j="0";j<=arraysorted.length-p-2;j++){ if (arraysorted[j]>arraysorted[j+1]){
temp=arraysorted[j+1];
arraysorted[j+1]=arraysorted[j];
arraysorted[j]=temp;
done=false;
}
}
p++;
}
return arraysorted;
}

1: booleans initialize to "false" and ints initialize to '0'. explicitly setting these values is unnecessary. Perhaps a good habit, but unnecessary. p is also in no way descriptive. It isn't following the lisp '-p' convention; why can't he use 'i' or 'count'? 2: Also unnecessary. the parameter "array" does not point to the same location as the array that was sent to it, so the line "int [] newArray = bubbleSort(oldArray);" will create a copy of oldArray, and pass that to bubbleSort().
3:This 'done' flag is absolutely unnecessary... It does nothing. To make matters worse, he handles the flag how drunken frat boys handle glassware: throws it around till it hurts something:
4:Every iteration of the outer loop sets "done" to "false". This means the operation is made n times in the outer loop, where n is the number of loops. It is also set to "false" in every inner loop (as long as flipping happens), this adds ~nlog(n) operations to the procedure. The operations that are done to this boolean flag (which I remind the reader, does not need to be there) have a complexity of O(nlogn). He has effectively turned a flag, which, if properly used (assuming it did need to be there) will take exactly n+1 [O(n)] operations into a flag which will take between 2n and (2n)^2 operations.
5:This one is fun. On the first page of the "how to write readable code" handbook (which doesn't-- but should-- exist) is the following statement:
"Do not use magic numbers!" In CS there are 3 numbers: 0,1,n. Any number besides 0,1 number should be specifically labeled because it's being used for something specific. Better, he has "<=/*some number*/-2" rather than "