Some Seaside Refactoring

Written on July 2, 2014.

During a code review, I stumbled upon this. It is the callback of a submit button in a Seaside app:

RegisterComponent>> saveAction
| errorMessage |
errorMessage := self getError.
errorMessage ifNil: [
    (password = confirmPassword)
        ifTrue: [
            user password:  (ASHasher defaultHasher new hashString: password).
            (user email isEmail) ifTrue: [
                (CCUser issetUser: user email)
                    ifTrue: [
                        self session addErrorFlash: 'Email already used' ]
                    ifFalse: [
                        self user save.
                        Log message: 'Register new user: #', user id asString, ' - ', user email.
                        self createOwnCompany.
                        self session user: user.
                        self session moveToWithBC: CCRepositoriesComponent new.
                        CCMailSender sendRegisterEmailToUser: user ]]
                ifFalse: [ self session addErrorFlash: 'Email invalid' ]
        ] ifFalse: [
            self session addErrorFlash: 'Password and confirm password should be the same.' ]]
ifNotNil: [ self session addErrorFlash: errorMessage ]

Don’t panic, we’ll gladly clean this.

At first look, this method is typically way to long for a smalltalk method. I tolerate long methods in very specific cases like WAComponent>renderContentOn:; HTML is quite verbose once you add classes and nested divs. It’s easy to get fat rendering methods.

Another thing that looks bad is the identation level. When you see such a level of indentation in the middle of your code, you probably have a problem somewhere.

The two previous points were more something that comes more from gut feeling than rational thinking but I trust my gut when it’s telling me something.

Let’s go deeper. The method seems to do way too much: flash messages handling, kind of user validation, aftersave callback (createOwnCompany), logging, UI redirection and email sending. Pfoui!

Let’s start by the beginning:

RegisterComponent>> saveAction
    | errorMessage |
    errorMessage := self getError.
    errorMessage ifNil: [ ... ]
    ifNotNil: [ self session addErrorFlash: errorMessage ]

Mmmm. Does it mean that errors could have popped ud elsewhere? To me, it’s not the callback responsiblity to check this. Or it should do it explicitly by calling a well named method. In this case, the reader is left wondering why the hell the behavior is dependent on the previous errors. It means that the state of the component has been modified elsewhere. Anyway, we can always put some makeup in top of this but there’s a sign that we might need a solid refactoring of the whole class.

The line errorMessage := self getError. is quite confusing. The variable name errorMessage makes us think that we have a string while the self getError seems to return a kind of Error object.

Without knowing more, I would refactor that bit of code to:

RegisterComponent>> saveAction
    self getError ifNotNil: [:errorMessage |
        ^self session addErrorFlash: errorMessage]
    ...

It saves us a temporary variable and one indentation level. We can know proceed and go the inner part of the method. This is what it looked like:

(password = confirmPassword)
        ifTrue: [
            user password:  (ASHasher defaultHasher new hashString: password).
            (user email isEmail) ifTrue: [
                (CCUser issetUser: user email)
                    ifTrue: [
                        self session addErrorFlash: 'Email already used' ]
                    ifFalse: [
                        self user save.
                        Log message: 'Register new user: #', user id asString, ' - ', user email.
                        self createOwnCompany.
                        self session user: user.
                        self session moveToWithBC: CCRepositoriesComponent new.
                        CCMailSender sendRegisterEmailToUser: user ]]
                ifFalse: [ self session addErrorFlash: 'Email invalid' ]
        ] ifFalse: [
            self session addErrorFlash: 'Password and confirm password should be the same.' ]

We could do the same as the previous point and replace the first condition with the following, saving a big nested block:

(password = confirmPassword) ifFalse: [
    ^self session addErrorFlash: 'Password and confirm password should be the same.'
]
"the remaining code".

As we return directly from the block, we don’t need an else clause. But refactoring this would be using lipstick on a pig. The code needs more profound changes (e.g., why the hell the component should know how to hash a password). The whole logic should not be handled by the component but by the User class itself or, even better, something like a UserRegistrationService. One of the benefit of doing this is to have an isolated entity that you can test without involving the UI. It will also be useful if you want to develop an API.

This is how I would refactor the code. Note that this is still not perfect but it’s already a lot better. The snippet exhibits some architectural choices that we could discuss but it’s a topic for another post (e.g., sending an error when validation fails):

registration := (UserRegistration on: user)
    password: password
    passwordConfirmation: passwordConfirmation.

[user := registration register] on: ValidationError do: [:error |
        ^self session addErrorFlash: error printString]
self session user: user.
self session moveToWithBC: CCRepositoriesComponent new.

So now, the UserRegistration class is responsible for:

• Validating that the password and password confirmation match.
• The user email is not already taken by another user (by the way the method issetUser: is terrible; it’s not camelCased and does not convey what it is testing)
• Hashing the user password. By the way, the current method is missing salt. This need to be changed.
• Saving the user in database
• Logging the user creation
• Creating a default company for this user
• Send the registration email

Overall, we have the following:

    RegisterComponent>> saveAction
        self getError ifNotNil: [:errorMessage |
            ^self session addErrorFlash: errorMessage].

        registration := (UserRegistration on: user)
            password: password
            passwordConfirmation: passwordConfirmation.

        [user := registration register] on: ValidationError do: [:error |
            ^self session addErrorFlash: error printString].

        self session user: user.
        self session moveToWithBC: CCRepositoriesComponent new.

As we said, there’s more to do but it would involve a bigger refactoring of the class. The important thing is to start, this small cleaning is the first step of the process.

In real life, when seeing a piece of source code like this, I don’t necessarily refactor from top to bottom. I have a more organic workflow where my gut feeling tells me what to do.

I don’t know if code style is something that you learn in school, if it’s innate or if it comes with (a lot of) practice but as a programmer, you need to care about the code you’re producing.

Feedback is welcome, ping me on Twitter if the subject interests you.