[Freecol-developers] Merge request 41 -- removing Blob listeners

[Arnaud Blouin]

Hi all,

We are researchers in software engineering that currently work on user
interface (UI) bad coding practices. We are studying UI listeners, in
particular listeners that manage several widgets, like the following
listener extracted from Freecol:

public void actionPerformed(ActionEvent ae) {
final String command = ae.getActionCommand();
EuropeAction act = EuropeAction.valueOf(command);
switch (act) {
case EXIT:
exitAction();
break;
case PURCHASE:
getGUI().showPurchasePanel();
break;
case RECRUIT:
getGUI().showRecruitPanel();
break;
case SAIL:
sailAction();
break;
case TRAIN:
getGUI().showTrainPanel();
break;
case UNLOAD:
unloadAction();
break;
default:
super.actionPerformed(ae);
break;
}
}

In such listeners, the source widget is identified using if/switch
statements. Thanks to empirical studies we have indications that such a
practice has a negative impact on the code quality. We call UI listeners
that control three or more widgets, Blob Listeners (see this PDF
document for more details: https://hal.inria.fr/hal-01308625v2/document).

We identified in Freecol 15 instances of the Blob listener and for seven
of them we have a refactoring solution. The merge request we just
submitted (https://sourceforge.net/p/freecol/git/merge-requests/41/)
removes these seven Blob listeners by splitting them into atomic UI
listeners (i.e. they manage a single widget). Each commit of this
request refers to one Blob listener. The new code is less complex in
terms of cyclomatic complexity (CC) and lines of code (LoC): the new
code has -122 LoCs and -36 of CC on the refactored files (according to
Sonar analyses).
Several details about the refactoring are described below.


Complementary to this merge request, we have several questions for the
Freecol's developers:

1/ Do you think that coding UI listeners that manage several widgets is
a bad coding practice?


2/ Do you think that the threshold of three or more widgets per listener
relevant for characterising a Blob listener?


3/ Do you consider the refactored code suitable for removing Blob listeners?


4/ Do you have any idea of the reason of the presence of Blob listeners
in the code?
For example, lack of anonymous functions (aka lambdas) in Java 7 (and
previous JDKs)? Lack of programming experience at the early stage of the
development? etc.

5/ Do you have suggestions about other bad user interface coding
practices? This can concern controllers, commands, GUIs, widgets, data
bindings, GUI testing, etc.



Regarding the refactoring, FreeColPanel defines a button OK and an
actionPerformed to manage it. Most of the FreeColPanel's children
override the behaviour of this button OK (e.g. TradeRoutePanel). This
explains why our refactoring did not always remove the actionPerformed
methods to let the management of the button OK as it.

Possible issue:
src/net/sf/freecol/client/gui/panel/report/ReportPanel.java
(actionPerformed, l140): we cannot find the associated widgets that will
trigger the execution of the code lines 145-150.

We identified 8 Blob listener for which we do not have a refactoring
solution, for example:
src/net/sf/freecol/client/gui/label/UnitLabel.java (actionPerformed,
line 394): the listener registrations are made in QuickActionMenu, so we
prefer not to refactor this Blob.


Do not hesitate to discuss about the refactoring and provide me feedback.

-- Arno

[Michael T. Pope]

On Fri, 23 Dec 2016 12:42:11 +0100

Strange, I cannot comment my merge request.
I replaced the tabs by spaces as required.

Cool. Will look at it again in a few days time.

Mike, thanks for your quick feedback. When you say that the refactoring
is an improvement, what does it improve for you?

I like it when the code for defining a UI element and the code for
interacting with it are close together. So hauling code out of the action
listener routine and into a lambda next to the point a button is defined
is an obvious win for me. OTOH, I like lambdas (old Lisp hand here).
Others are less convinced.

I also like shorter code. Mostly.

Also, when you say that "Interface complexity is the #1 menace.", I
guess you are talking about the number of widgets that can compose a
user interface,

Partly. Also number of different classes involved, and especially the
interactions between the classes... however that is a slippery concept ---
I have seen cases where blindly reducing the cyclomatic complexity made
a codebase harder to read/understand.

the difficulty to use user interfaces?

I am thinking mainly from the code development side. I try to keep out of
UI design / usability.

Cheers,
Mike Pope