Big is ugly and evil - at least in Smalltalk code

This is an old wisdom that every experienced Smalltalker knows and should be aware of. Unfortunately, there are still experienced Smalltalkers who don't practice this wisdom, mainly because they are not capable of thoroughly abstracting and delegating.

Today I came across such an example of code developed by one of our code contributors with about 6 years of Smalltalk. I had to rework this code completely to make it clean and to delegate responsibilities to those classes where they really belong.

There was a method in an UI controller class that is part of our Seaside application classes, which is supposed to create a new user login instance for certain types of requests. This involves the incoming WARequest, which contains most of the needed data.

The original implementation did all the relevant logical checking in one big method of about 20 lines of code in the UI controller instance. This method was messaging to three different model classes plus the WARequest instance.

I did the following:

1) All request related checking was moved to WARequest, which now answers to one new method "isRequestWithLoginObj". Only the request class should know about its internals and about its type of request. Alternatively, one could also delegate this to the login model, but for various reasons I preferred the first choice.

Unfortunately, all of this had to be added by us to WARequest, because the standard Seaside class does not provide any such services. In the Seaside library the responsibilities and knowledge about request internals is spread over many other classes, which directly interrogate the "fields" instVar of WARequest. Therefore knowledge of many request internals, which should really be hidden inside WARequest, is spread widely.

2) All other logical checking, whether or not a new UserLogin model is to be created, was moved to the login model class together with passing the request instance. Now these two model classes make this up entirely among themselves.

3) Two other model classes, which are related to this procedure, are no longer touched by the UI controller. Instead, only the UserLogin factory code talks directly to its related model classes.

The result:

From 20 down to just 3 lines of code left in the UI controller method, two of which are related to internals of the UI controller.

Most important is the clear delegation of responsibilities to the respective classes and the much greater modularisation of the code through potentially re-usable small methods.

I made the experience that it is much wiser to put work into re-factoring such code, because modular code is much better to maintain, much simpler to modify and therefore also cheaper over the life cycle of a project and typically also less error prone.

I wish the authors of Seaside would regularly undergo similar re-factoring, not because the Seaside methods are too big, but primarily because the responsibilities in the Seaside code are not sufficiently encapsulated in the respective classes.

See also my post: "Proposal 1: Class WARequest!