Al's Code Review Guide
Last updated at 5:28 am UTC on 24 October 2005
Hello, In my wanderings through Squeaks method's I have noticed a number of things that I find annoying if not actual performance issues. To help you write code to my taste ( ;) ), I have prepared a code review checklist. If you decide to add stuff to this list, please initial your changes so we know who to blame. If you don't like something, add a paragraph to the description explaining in horrible detail why you think I'm stupid. Simply deleting the item of stupidity isn't as helpful as people might be prone to come up with the same dumb idea again.
Checklist:
1. Problem: making minor adjustments with an atomic bomb. – I've seen code for a game that destroyed and redrew the entire UI after each move.
Solution: Often in squeak it is possible to give objects new maching orders in the middle of the battle. When possible, save the nuclear option for when you load the map and then keep a separate list of things that might be updated over the course of the game and then send update messages to those. Doing this will make the interface much more responsive and reduce or eliminate unwanted visual flicker.
2. Misusing global variables: Variables are for things that varry, if your object has constants, make a method category of "constants" and then populate it with methods which return your constant. – If you want an argument in favor of this practice, look at the class declarations for the VM code... (You will need to change refferances to this former class variable too.)
3. Look at how your methods are being used.
In the interpreter code, following #2 above, you'd have a method called "typeMask" and it's text would be ^3. OK.. As you change all refferances to "typeMask" to self "typeMask", you will notice that it is _ALWAYS_ used as a parameter to bitAnd:. A better solution to this is to go ahead and change your method to "typeMask: param" and then the text to "^param bitAnd: 3".
This will allow you to strip a ton of code out of the rest of your class. Any time you see code repeated again and again and again, it's a good idea to make a new method out of it.
4. Accessor methods used from within the same object suck. Just reference your instance or class variable. I've seen quite a bit of code that accesses a local variable through an accessor method which merely returns the value of the variable. These create tons of extra "self" commands and are generally annoying. While you can't always get rid of the methods themselves, they should never be prefixed with "self". If you have a good reason for using accessor methods, perhaps for some debugging purpose, explain why that is the case in the accessor method itself.
5. Maximally exploit the methods you have: In Croquet's TFrame class, a base class for most of Croquet, there was an accessor method called "frameChildren". Someone followed some version of advice #3 and made a method "frameChildrenDo:". It checked for nility then iterated over the list. However, when you look at the callers of "frameChildren", you find dozens of instances of the same method written inline or, worse, the same method without the safety check!
6. Worship the semicolon: I don't know if this is a speed boost, but when you want to send a lot of messages to a single object, it looks alot better to concatinate all of these message sends with semicolons than to invoke the object separately for each call. There are a couple of places you can't do this but mostly it's good advice.
7. Remove all unnecessary variables: 60-70% of all squeak methods don't need variables at all. I don't know if this is a performance boost, but it certainly can't hurt. If you see an object receiving alot of messages, and then immediately being returned... For example:
| foo |
foo := something.
foo bar baz bat boof.
^foo
Can usually be written as:
^something bar baz bat boof
(there might be some semantics that I don't fully appreciate there..)
8. Demote variables to the smallest possible scope: eg:
| foo bar |
something aMethod: [ bar := x. bar aMethod: [ foo := y. foo aMethod]]
would become:
something aMethod: [| bar | bar := x. bar aMethod: [ | foo | foo := y. foo aMethod]]
Here, variable foo or even bar may not even need to be allocated if the block is never needed, definitely a speed boost. Also, by minimizing the scope of variables, you reduce the chance you may inadvertantly misuse something.
