Monday, January 25, 2010

Making it obvious is hard to do

It is always interesting to join a project which has some code already. Our code is supposed to be self documenting and easy to read. However some of the code i have seen so far is not. Let me give you one example.
The following code is part of a search feature. It contains two different forms and a result screen. The two different forms allow the user to either search by the details of a person, eg. name and address or by some ids, which are assigned by the system. The following method is part of a presentation model for the forms. It is called, whenever one of the forms dispatches a change event (user tabs out of a input field.)


public function handleSearchFormChange() : void
{
if ( ( detailedSearchFormPM.criteria.isValidForSearch() || uidSearchFormPM.criteria.isValidForSearch() ) )
{
if ( detailedSearchFormPM.validatorGroup.validate( true ) )
{
searchEnabled = true;
}
else
{
searchEnabled = false;
}
}
else
{
searchEnabled = false;
}

if ( detailedSearchFormPM.criteria.isValidForReset() || uidSearchFormPM.criteria.isValidForSearch() )
resetEnabled = true;
else
resetEnabled = false;

}


What does this method do? The name doesn't tell us. Instead is is named after the event it is supposed to handle. So instead of abstracting away some concept, this method makes the code even less abstract by introducing additional information, which is not needed here. Is there a bug in the first part? Should the search button only be enabled if the detailed search contains valid data?

In the mxml code you can see the method beeing used like so


<... change="pm.handleSearchFormChange()" .../>


So at the level of the mxml code the information is pretty much duplicated. By looking at the mxml code alone, we are not able to guess, what this method is doing.

Now lets look at the code of the method. It updates two properties searchEnabled and resetEnabled. These properties are bindable and are connected to enabled property of the two buttons search und reset. However, what rules are used to calculate these properties, is not obvious from the code. The code accesses inner objects of the FormPMs, not good, because it ties us to the implementation of these PMs. (Law of Demeter)

Let's see, if we can refactor the code a bit.


public function handleSearchFormChange() : void
{
enableButtonsIfFormsContainInput();
}

private function enableButtonsIfFormsContainInput() : void
{
enableSearchButtonIfFormContainsValidInput();
enableResetButtonIfFormContainsAnyInput();
}

private function enableSearchButtonIfFormContainsValidInput() : void
{
searchEnabled = detailedFormContainsValidInput() || uidSearchFormContainsValidInput();
}

private function detailedFormContainsValidInput() : Boolean
{
return detailedSearchFormPM.criteria.isValidForSearch() && detailedSearchFormPM.validatorGroup.validate( true );
}

private function uidSearchFormContainsValidInput() : Boolean
{
return uidSearchFormPM.criteria.isValidForSearch();
}

private function enableResetButtonIfFormContainsAnyInput() : void
{
resetEnabled = detailedSearchFormContainsInput() || uidSearchFormContainsInput();
}

private function detailedSearchFormContainsInput() : Boolean
{
return detailedSearchFormPM.criteria.isValidForReset();
}

private function uidSearchFormContainsInput() : Boolean
{
return uidSearchFormPM.criteria.isValidForSearch();
}


This would be start to make it a bit more readable. Now a next step would be to move some of the methods to other objects. All the methods which are concerned with the inner details of one of the FormPMs should be moved to the FormPMs. The resulting code looks like this.


public function handleSearchFormChange() : void
{
enableButtonsIfFormsContainInput();
}

private function enableButtonsIfFormsContainInput() : void
{
enableSearchButtonIfFormContainsValidInput();
enableResetButtonIfFormContainsAnyInput();
}

private function enableSearchButtonIfFormContainsValidInput() : void
{
searchEnabled = detailedSearchFormPM.containsValidInput() || uidSearchFormPM.containsValidInput();
}

private function enableResetButtonIfFormContainsAnyInput() : void
{
resetEnabled = detailedSearchFormPM.containsInput() || uidSearchFormPM.containsInput();
}


Now we have to decide if we really want to know, when the entry method is called. Instead of the method name we use the event parameter to tell us when the method is called.


public function enableButtonsIfFormsContainInput( event : FormsInputChangeEvent ) : void
{
...
}


and in mxml it looks like so



<... formsInputChange="enableButtonsIfFormsContainInput(event)" .../>



Now the code reveals much more about its intention. If somebody needs to fix this code or to add another feature, it is much easier to do so, because it is obvious, what the code is supposed to do.


Best,
Ralf.

Saturday, January 9, 2010

What i'm up to: C++, Qt, Release It

I have a few days of, which allows me to spend some time on learning a new language. I choosed C++ this time. For the GUI i use QT, which has some nice abstractions like signals and slots.

On my book shelf i have "Release it" by Michael T Nygard. It is about architecture for production versus architecture for the ivory tower.