Changing some code up, fixing some bugs, and should we criticise when people are doing their best?

by Oli Howson

So Kasim, whom I believe to be an OCR GCSE CS teacher recently posted the code below on Facebook, reporting an error and seeking help.

I, like others, managed to run the code fine and assume the error was due to some funky Idle caching problem or some such. But the code did make me twitch a bit and I thought it might be a good idea to try and improve upon it to help the OP and others.

This brings me to the musings part of this post; should we criticise people that post what can only be described as bad practice? There is a dearth of Computer Science teachers in this country to which I gladly added a few months ago by leaving teaching and we certainly shouldn't be driving them away. This chap is doing his best, trying to help students, and utilising the resources at hand. My cap comes off to him for this - but yes we should criticise. Constructively. If I saw a PE teacher showing kids how to play football by picking up the ball and running through the goal I'd have to say "oi! even I know that's called rugby". But seriously, students don't know better. No wait - most students don't know better - there's always one isn't there? So if this is what they're taught, this is what they'll think is right. Then things won't work, coding becomes harder/boring/not done and the student falls out of love with CS. Think about it, they may go on to become a politician and nobody wants that! So we have to criticise - and do so in a way that helps. It's called constructive criticism.

So let's look at the problems:

  1. Globals. Globals bad! Why? At this level, they're just bad practice. This isn't lecture time so check out this stackoverflow link for a starting point.
  2. Recursion. Recursion not bad, recursion very good. When. Used. Correctly. Personally, I hate it because it makes my face hurt but I understand, like the Politicians, it's something we need when we need them. But it can go wrong, leading to messy/unreadable code, confusion, and in the worst cases can crash a program.
  3. Verification. Always verify code entered. It's boring, but needs to become a given else it gets forgotten when needed.
  4. Decomposition. My favourite word in the whole entire galaxy. I generally decompose to death (far beyond what's needed) because it makes things easier in the long run and matches my way of thinking.
  5. Unneccesary else - or in this case elif - statements. Learning [LPC](https://en.wikipedia.org/wiki/LPC_(programming_language) for Discworld I've learnt to use return values a lot more - particularly in IF constructs. IF something is true return 1. So you don't need to do an else, because if it gets to the next line of code it hasn't returned and hence is automatically an else! And doing an elif instead - that's potentially doing a comparison that isn't needed.
  6. Poor variable names. This bit is pretty personal, but skilllevel got to me a bit;
    camel/dromedary case (skillLevel) would be my choice, Pascal case (SkillLevel) or snake case (skill_level) are all clearer.
  7. Comments - there are none. Like everything, get it right from day one and it gets easier when it's really needed.
    I'm sure more things will occur to me as and when I get to them! (they did - points 6 and 7 are added!)

Another member of the group has kindly let me include his improved version:

I'm sure Sean (the author) would be the first to admit this isn't perfect but it's a lot better.

  1. Uses iteration rather than recursion
  2. Has (some) comments
  3. Camel cased variables
  4. Saves the use of the else in the second decision statement
  5. Got rid of the globals (thank the gods!)

This got me thinking about how I would do it, and I came up with the below. Unfortunatly the code's starting to get a bit bigger so I've linked to code/pdf rather than print-screening.

Let's see where we are with this:

  1. Globals - gone
  2. Recursion - gone
  3. Verification - verified
  4. Decomposition - decomposed
  5. Unnecessary else/elifs - removed
  6. Variable names - improved for clarity
  7. Comments - added

But - there are a couple of bits I don't like.
8. I have firm beliefs on when to use definite, post-test definite and pre-test definite iteration, and the final loop here isn't following my beliefs. See below.
9. I really don't like returning long lists of variables. I know it's fine, I know it works, but I don't like it. So That lot can be encapsulated into a class (in Pascal we'd do it in a record).

Choosing your loop

So, my final (until someone suggests some improvements) version, code and pdf. I'm sure I've upset someone, and the code is certainly a lot longer than it was, but this is they way I code, and the way I've always taught others to code. Feel free to contribute ideas via e-mail or twitter :)