Coding Style used in Smalltalk/X Classes

Code Format in ST/X

However, eXept and the author already went throught the above discussion process, and if you donate your code, you are the newcomer to the team. Don't expect everyone else to adopt to your personal preferences, but adjust to theirs.

Initially, we did not ask for a strict coding guideline, but followed the "do not discuss" rule above. However, over time, a lot of code has found its way into ST/X which is hard to read simply for its different indentation and coding style. Also, some code is harder to read for beginners or needs mental analysis to understand. We do not want this situation to become worse in the future, and will reformat any old code as we encounter it and find time.

We want the ST/X code base to conform to a common indentation style for two reasons:

• newcomers should not be confused by various different styles

• to support graphical structure recognition
  It is a matter of fact, that the human brain is able to perform quite a bit of preprocessing on the unconcious level, based on the pure layout and geometric arrangement of code. For example, loop structures should be visible as such, without even a need to read the actual message names - simply by how the code is arranged. Newcomers and infrequent programmers may not experience this, but to a long-time hacker, used to his style of indentation, a lot of pattern matching happens on this level.

Also, eXept requests that any code which is to go into the main stream of the ST/X code base (i.e. is to be published as part of the ST/X deployed package) conforms to the following coding style, simply because it is usually us who will have to deal with any error reports and questions about the code later.

The rules are defined to make this "pictoral structure matching" easy. They are:

1. Kernigham - Ritchie (KR) style indentation
   Sorry to all Smalltalkers and Lispers: 98% of the world is indenting their code this way, and ST/X does so as well. It may well be true, that some of the reasons for Lisp and Smalltalk not being mainstream was due to their indentation style, which is hard to read for beginners (it may well be for a guru with 10 years of Smalltalk experience, but...). Very short loops (collection collect, select) or if-for-value can go into a single line.

2. Indentation level is 4
   2 is not enough to make alignment of closing bracket to opening keyword easy; 8 is too much and usually leads to a need to scroll the editor to the right. Be careful, when editing ST-files in an external editor (which is stupid, anyway): the meaning of a Tab character in the file is "move to the next multiple-of-8 column". This "8" is independent of any of your personal preferences for how much the tab-key should indent.
   No special indentations (such as a ^ statement being indented 2 colums to the left).

3. Blocks which are NOT loops or conditionals should NOT be indented as such
   Although Smalltalk makes no difference between a block argument which is defining a callback block (as used in GUI widgets) and blocks which are loop-blocks (as in the collection protocol) or conditionals, it definitely DOES make a difference to a programmer's mental model of it.

   Therefore, we demand that ONLY loops and conditionals are indent KR-style. Callbacks, async block arguments and "if-for-value" blocks are not. Blocks which are not executed (i.e. assigned to a variable for reuse) should also not be indented as if they were control blocks.

   Thus, a block assigned to a variable should be indented as:

   var := [ blocks code... ]

   or (for long blocks):

   var := [ ... blocks code ... ]

   instead of:

   var := [ ... blocks code ... ]

   Reason: this would require the programmer to look at and read the first line, to see that it is not a loop or conditional (and maybe even have to scroll). Of course this also means, that loops should not be indented like the above.

4. Local block variables are declared in an extra line.
   If block locals are written after the opening bracket or block args, that line looks like a boolean-or message sent to some variable. You would have to actually "read" the code, in order to understand, that it's a variable declaration. If it is on an extra line, this begins with a "|" which is immediately identifyable as a declaration. You would not write method locals after the method selector in the first line - would you?

   So block locals should be declared as:

   collection do:[:el | | local1 local2 | ... ].

   instead of:

   collection do:[:el | | local1 local2 | ... ].

   Reason: because the first looks much like an expression, and I have to read it (and cannot using my "graphical structure recognizer")

5. One empty line separates the first statement from the selector, comment and locals. Period.
   The first statement should not directly follow any of the above. In order to guide the reader where to start reading, it must be separated by an empty line. Separate local variable declarations from the comment also by an empty line. For very simple getter/setter or forwarder methods, which have no comment, you may ommit that empty line.

   There is no need for an additional empty line between the method's selector and argument declaration and the method comment or locals declaration. Especially, your code should NOT put an empty line BEFORE any local variable declarations AND at the same time OMIT the one before the first statement, as in:

   foo: arg1 bar: arg2 |local1 local2| statement1. statement2. ...

   instead, write:

   foo: arg1 bar: arg2 |local1 local2| statement1. statement2. ...

   the reason is that in the first version above, you have to look at and read the local variable declarations to find the first line of real code. Whereas in the later example, the layout already leads you there. Be reminded that the syntax highlighter usually emphasizes the first line, so your code would be presented as:

       foo: arg1 bar: arg2
   	|local1 local2|
   
   	statement1.
   	statement2.
   	...
   

6. Method comment
   Every method, except for simple getters/setters MUST have a method comment at the top (between selector and local variable declaration or code). The comment is delimited at the bottom by an empty line from the real start of the code or the declaration.

7. Sample usage method comment
   If there is a comment which demonstrates the example use (which is a very welcome thing to have), place it at the END, not at the beginning.
   We understand, that it is convenient during development of new code to have it at the beginning (where it is easier to select and doIt for a hacker), it disturbs the flow of reading for others later. Usually, when trying to understand unknown code, it will be read top to bottom and the usage examples may force one to scroll down.

   Also, tooltip messages and code completion hints are extracted from a method's first comment, and thus may get quite ugly and long if the usage example is at the top.

8. When wrapping long lines, selectors are aligned to the left
   When splitting a long message send among multiple lines, the followup lines are aligned at the left.
   Thus write:

   ... Dialog request: 'some string' initialAnswer: 'blablabla' initialSelection: (4 to:6)

   NOT:

   ... Dialog request: 'some string' initialAnswer: 'blablabla' initialSelection: (4 to:6)

   The reason is that the right-aligning version makes it harder to see which keyword parts belong to the send and where it ends.

9. Wrapping long lines: followup and/or in conditionals
   When splitting a long conditional into multiple lines, the break should be done before an and/or keyword, and the remaining code be indented one level to the right, with the actual if-keyword indented at the original column, so it is nor confused with the code of the if-body (see below).

10. Avoid complicated boolean expressions
    Complicated combinations of and:/or:/not should be avoided. If possible, use nested ifs or guards. Sorry to mathematicans: most programmers are not, and it is often not obvious what the outcome of a complicated and-not-or-not combination will be.
    Avoid double negation ("notEmpty not" or "foo not ifFalse"); for each of them, there is a corresponding positive condition (i.e. use "isEmpty" instead or "foo ifTrue")

11. Class documentation
    Every class should have a documentation method consiting of a comment-only method. The class documentation should also contain hints as how the class interacts with other parts of the system, and how the class/framework is used. For frameworks, the project-definition class is a good place to put framework documentation.

12. Examples
    A convenient place to add examples (especially for complicated frameworks, which need configuration and/or special instantiated objects) is the "example" method on the class side. This may consist of comments only or contain real sample code. Unit tests alone are not a relacement for an example method which shows how a complex framework (like an application or server process) is started, because these unit tests are often badly commented and it is often difficult to decide which test is testing internal mechanisms as opposed to the outside api of a framework.

Examples for "dos" and "don'ts"

    foo := bla ifTrue:[
	5
    ] ifFalse:[
	6
    ].

    foo := bla ifTrue:[5] ifFalse:[6].

    foo := bla
	    ifTrue:[...]
	    ifFalse:[...].

    foo := bla
	    ifTrue:[
		...
	    ]
	    ifFalse:[
		...
	    ].

    fooAction := [
	...
	statements
	...
    ].

    fooAction :=
	[
	    ...
	    statements
	    ...
	].

    (condition1 and:
	[condition2 and:
	    [condition3]])
		ifTrue:[
		    statements
		]

    (condition1
	and:[ condition2
	and:[ condition3 ]]
    ) ifTrue:[
	statements
    ]

    expr do:[:arg| |a b c|
	statements]

    expr do:[:arg |
	|a b c|

	statements
    ].

Avoid Obfuscated Code

• ommitting a "^ self" to save some typing

  Often, some alternative must return from a method after some other method is invoked.
  A typical example are guards, as in:

  ... foo ifTrue:[ ^ self doSomethingForFoo. ]. bar ifTrue:[ ^ self doSomethingForBar. ]. ...etc... ...

  the problem with the above code is that from reading, you do not know whether the return value from "doSomethingForFoo" is really wanted here, or if it's simply a lazy typer, and the real return value is "self". So the reader has to navigate to implementors of "doSomethingForFoo" and look for the return code. Even worse, if he finds different return values in the different messages (such one returning self, another returning a boolean), he is really in trouble to determine the correct expectations when he wants to subclass or modify one of the doSomething methods.

  Except for the cases when the return value really counts, you should always write:

  ... foo ifTrue:[ self doSomethingForFoo. ^ self ]. bar ifTrue:[ self doSomethingForBar. ^ self ]. ...etc... ...

  It has absolutely no implications on the execution speed or code size, but makes it clear, that you are not interested in the answer from "doSomethingForFoo".

• reusing variables

  Do not reuse a local variable - a good style is to assign only once (functional style). Give your variables useful names. The code is written only once, but usually read many times by yourself and others later.

• use "contains:", "includes:" etc. instead of "do:"-loops whenever possible

  These methods not only save typing - they are also documenting what is done.
  Do not write code like:

  myMethod someCollection do:[:eachElement | someCondition on eachElement ifTrue:[^ true] ]. ^ false.

  instead, write it as:

  myMethod ^ someCollection contains:[:anElement | anElement someCondition].

  Just read it aloud and you know why.

• use "contains:", "includes:" etc. instead of "detect:ifNone:" when checking for the presence of an element

  The same argument as above applies here; often, "detect:ifNone:" is used to check for the presence of an element - not to retrieve that element.
  This should be avoided; instead of:

  (someCollection detect:[:el | someCondition on el] ifNone:nil) notNil ifTrue:[ ... ].

  write it as:

  (someCollection contains:[:el | someCondition on el]) ifTrue:[ ... ]

  Again, if in doubt, read the code loud for yourself.

  As an extreme example, look at:

  (coll detect: [:b| b == fooBlock] ifNone: [nil]) isNil ifFalse:[ ... ]

  which is a combination of "double-negation" confusion AND "detect:-to-test-for-inclusion" confusion.
  Every reader of that code (even an experienced smalltalker) has to read and think about this carefully, only to figure out, that it's nothing more than:

  (coll includesIdentical:fooBlock) ifTrue:[ ... ]

  By the way: you can search for such code fragments using the SystemBrowser's "code-search" facility. The above is found with the pattern:

  (`@e1 detect: `@b1 ifNone: [nil]) isNil ifFalse: `@b2

  Here is another example, which uses two nested loops:

  | list rslt | list := someList asOrderedCollection. list do: [:p| (p blocks detect: [:b| b == someObjectToSearchFor] ifNone: [nil]) isNil ifFalse: [foundElement := p] ]. foundElement isNil ifTrue:[ ^ false ]. ^ true

  let us rewrite this in smaller steps to a more readable version; the inner loop's statement is the one we saw in the previous example, so we can rewrite it to:

  (p blocks contains: [:b| b == someObjectToSearchFor]) ifTrue: [foundElement := p]

  which is the same as:

  (p blocks includesIdentical:someObjectToSearchFor) ifTrue: [foundElement := p]

  the outer is simply another search, and the "do" can be rewritten to:

  foundElement := list detect:[:p | (p blocks includesIdentical:someObjectToSearchFor) ] ifNone:[nil].

  as the foundElement is not needed, the overall code can also be:

  | list | list := someList asOrderedCollection. ^ list contains:[:p | (p blocks includesIdentical:someObjectToSearchFor) ]

  the last question we should ask is "why do we need a copy of the original collection as OrderedCollection for the search ?". So we can remove this as well, get rid of the temporary variable, and get the final code:

  ^ someList contains:[:p | (p blocks includesIdentical:someObjectToSearchFor) ]

  which is both faster and requires less memory (due to the removed list-copy). If the original collection is one of the hash-based collections (i.e. a Set), the includes-operation is probably even *much* faster ( O(1) instead of O(n) ).
  And it certainly is much more readable!

• avoid using symbols as enum-values
  Very often, symbols are used as enumerated values, as in:

  ... state == #foo ifTrue:[ ...doSomethingForFoo... ]. ... state == #bar ifTrue:[ ...doSomethingForBar... ]. ...

  Beside the fact that this code is not very object oriented and should probably be done somehow using a message send, this code is very sensible to typing errors, as neither the compiler, nor the runtime system has a chance to check any such mistakes.
  For example, if you mistype #bar as #Bar at some place, the corresponding if-statements will never be executed and that kind of bug is hard to find.

  There are multiple ways to make the above more secure:

  1. Use class variables instead of symbols
     Some of the danger can be taken out of the code, by removing the possibility for typing errors. Add a few class variables, for example named "FooState" and "BarState", and initialize them in the classes' initialize method as:

     initialize FooState := #FooState. BarState := #BarState.

     then, use those variables in the case code:

     ... state == FooState ifTrue:[ ...doSomethingForFoo... ]. ... state == BarState ifTrue:[ ...doSomethingForBar... ]. ...

     Codewise, this does not make much of a difference. Execution speed is also not affected. However, you will be notified when you mistype one of the state names (by the compiler), and get an "unknown identifier" error.

  2. Use pool variables
     This is pretty much the same solution as above, and should be used if the state values are needed across multiple non-inheriting classes.
     An alternative is to use access methods (on the class side) to fetch the states from the other class. Old smalltalkers seem to prefer that over class/pool variables - mostly because the early browsers only provided a convenient "senders" search. With modern browsers, you can easily find references to class- and pool variables, so that argument is no longer valid.

  3. Use state-objects
     The above being first a step towards maintainability, a much better solution, which also makes later extensions much simpler is to provide separate state objects (state classes) and move functionality into them. This will eventually remove all of these switches and is generally called "object oriented".
     So as a first step, create some StateClass, and two subclasses named FooState and BarState (these can and often should be private classes).
     If there is any need to check for the state explicitely, use testing protocol:

     ... state isFooState ifTrue:[ ...doSomethingForFoo... ]. ... state isBarState ifTrue:[ ...doSomethingForBar... ]. ...

     However, once this is done, the obvious next step is to move the above actions into each state object class, by definining doX there, and get rid of the if-condtional, as in:

     ... state doX ...

     if required, pass the receiver, as in:

     ... state doXFor:self. ...

     the BIG advantage of this becomes apparent, when a new state is added.

     BTW: this is a well-known pattern in OO-programming.

     Notice, that the Smalltalk language offers the added convenience that the class itself can be used as a state object (which means that you don't have to care for any singleton, instance creation, identity or other problems).
     In most other so called "OO"-languages, this is not possible; either because there is no such thing as a class available at runtime, or because static (class-) methods are not inherited the way that instance methods are.

     Also notice, that in Smalltalk/X you can define the states as private classes, to keep the number of visible public classes smaller.

Things you should not do

• Message chains which follow the object structure

  Do not hardwire knowledge about object relations into you program; a typical example (which is very bad) is the following extract from a piece of code found in the manchester archive:

  ... aReadStream ioConnection input readWait. ^ aReadStream atEnd not ...

  obviously, the readStream object is some kind of Socket-accessing thingy, and the programmer wants to wait for something to arrive.
  The bad thing with the above is that it hardwires the knowledge of the (internal) layout of that socket-accessing thingy into the program. (I.e. if the underlying stream implementation is ever changed to not use an ioConnection-slot, or that ioConnection-object no longer has an input-slot, or you want to port to another Smalltalk, your code is doomed.

  Instead of the above, place the forwarding code into the message receiver, so that the readStream instance gets a readWait method, which knows internally that there is an ioConnection (if there is). The same goes for the ioConnection object, which should forward a readWait message to its input.

  Thus the abve should be written as:

  ... aReadStream readWait. ^ aReadStream atEnd not ...

  with a forearder in the readStream:

  ReadStream >> readWait ioConnection readWait.

  and the ioConnection:

  IOConnection >> readWait input readWait.

  If required, add those forwarders as extensions.

  Another example is:

  ... widget wrapper wrapper wrapper controller enable. ...

  here, the widget's internal hierarchy is reflected in the access-path, and the code will have a very hard time if the wrapper hierarchy ever changes - either in the framework classes or by the application programmer.

  These code fragments were specially written for a ParcPlace Smalltalk system, so they sure make problems when ported to ST/X, Squeak or another Smalltalk dialect. However, even ParcPlace itself will not be able to change any internals (optimize or get rid of the wrappers), without affecting this program.
  Thus, there is a chance that the above code will not work without change in the next ParcPlace version. And don't even think of porting it without change to another Smalltalk dialect.

  For the socket wait example above, a better solution was to provide a #readWait method in the class of the socket-accessing object, and delegate things there.

  For the wrapper example, to provide an enable-method in the widget, which delegates it to some controller.

  This hides the object structure and allows both ParcPlace to change things, and allows ST/X's Socket class to behave like other SocketAccessors. It also makes it trivial to introduce a facade class, which forwards messages as required.

  Of course, the above examples do not only apply to system classes - the same is very often found in user code - especially in GUI code.

Copyright © 1995 Claus Gittinger, all rights reserved