Thursday 16 April 2009

Four mistakes in one line of code

Here is a first and good example of the very bad coding style in Seaside. There is one line of code with four severe mistakes in it:

WARenderContinuation -> shouldRedirect: aRequest
^ aRequest isGet not or: [(aRequest fields includesKey: '_n') not]

1) There is no sense in castrating Smalltalk and forcing the developer to use a text editor for finding the occurrences of the key '_n' by placing this value into a string. Smalltalk has this great feature of showing the references to symbols! So why does one use a string for something, which is really a symbol (a key)? This is very bad style! Amateurish! Just stupid!

2) There are several places where the key '_n' is used. Therefore, the actual key should not only be a symbol but it should also be stored at one central place, in something like a config class, which acts as the holder of all such symbols. From there on, the references can be tracked easily.

3) The knowledge about where the value for this key is to be found is spread over many other classes (in this case WARenderContinuation). That is very bad, too. Only the class WARequest should know where the value for this key is implemented. Therefore, WARenderContinuation should not know anything about somebody else's instVar 'fields'.

4) The knowledge about how the value of this key is stored (in this case in a dictionary) should also not be spread over other classes. Therefore, the usage of "includesKey:" in another class is another bad mistake in this case. Solely the class WARequest should know how this key is stored in its instances.

There is a very simple solution for these bad style codings:

^ aRequest isGet not or: [
(aRequest isTypeOfRequestForRedirect) not]

All we need is one additional little method in class WARequest:

isTypeOfRequestForRedirect
^self fields includesKey: self class requestKeyForRedirect

This method hides the place and the form of storing the key inside itself and that is where it should (only) be. In this case, the key itself is taken from a central definition method on the class side, because this key is used by other methods within the class WARequest and possibly also from by classes.

Unfortunately, similar occurrences of very bad coding style are spread all over the Seaside classes.

Obviously, the authors of Seaside have not a good clue of encapsulating class behaviour and class data into the particular classes. Instead, they spread this knowledge freely all over other classes and then even in the form of strings, which cannot be tracked by the brilliant Smalltalk development environments.

This is just one example of many for the very bad coding style in Seaside. It's more "structured programming with objects" rather than object-oriented design. From my developers I would not tolerate such basic mistakes.

No comments:

Post a Comment