Software Quality: cleaning and simplifying code

[pgerlier]

Continuing my careful study of GRAMPS code, I just finished analysing the gramps/gen/plug/menu/ subdirectory. It provides tools to describe and manipulate “options” (configuration settings) for plugins. Options are represented by an instance of class Option or its “specialised” subclasses.

However, many of the “specialised” subclasses are mere aliases to Option without obvious reason but to change the name and suggest an intended usage.

The most obvious are StringOption and TextOption. They are exact replacements for Option. There is no difference between them.

StringOption has subclasses whose only difference is the absence of value argument in the __init__(…) method⁽⁺⁾: FamilyOption, MediaOption, NoteOption, PersonOption. In this case, wouldn’t it have been simpler to declare

def __init__(self, label, value=""):

in StringOption? Or even in Option?

Anyway, the current approach looks like a violation of the Liskov substitution principle (or of the design-by-contract methodology).

Only DestinationOption, as a subclass of StringOption, does a different initialisation and provides additional methods.

The same can be said about ColorOption, PersonListOption and SurnameColorOption where there is no value argument in __init__(…), which could have been handled by the above suggested patch in Option.

What is also confusing is the inconsistent (personal opinion, you can legitimately disagree) naming convention. There is an “intermediate” subclass EnumeratedListOption to handle lists but its subclasses are FilterOption and StyleOption without the “List” reminder. And there are subclasses of Option with this “List” reminder, PersonListOption and PlaceListOption which do not inherit from EnumeratedListOption. This means they don’t allow for detailed manipulation of the list. In fact they store or retrieve a full list with the parent method, but you can stuff any value without guarantee it is a list.

One big problem in GRAMPS is the ability to grasp the big picture. When creating software, there are two pitfalls: not enough classes and too many classes. The former does not abstract enough the semantics; the latter obscures meaning and regularity.

I grep’ed Gramps code for class. This returned more than 3000 occurrences. Since my regexp is naive, I estimate the real number of classes to be slightly over 1000. Such a number does not help.

Doing the same on methods and functions, my estimation lies between 5000 and 10000 which is really excessive.

Are there projects or efforts about streamlining the code?

I fear that new additions are made without taking advantage of existing code, writing anew support functions, contributing to application inflation. I already found classes bearing the same name without being related to each other. The main problem here is that reading the code, you don’t know which one is active until you painfully issue a global analysis of the import statements.

EDIT:
⁽⁺⁾ And what is worse is the fact that here-doc for the class and method has been copy-pasted from Option without reviewing it. As a consequence, value argument is still mentioned and described while it is not present in the __init__(…) signature.

As a side effect, it bleeds into the “Developer coding documentation” giving a faulty API for these classes. This questions the reliability and usefulness of the “documentation”.

[comeng]

I think what you are trying to describe could be loosely termed “bloat”

But in the real world everyone who can likes to design and build the
next generation of vacuum cleaner, everyone who cannot likes to use the
next generation of vacuum cleaner, and that leaves the very few who have
the gall to use the dust pan and brush to clean up the mess.
phil

[emyoulation]

Let’s try to be constructive in the responses. Please?

@pgerlier is identifying something that he wants to improve. He has the skills and experience to do so.

I greatly appreciate the contributions of @dsblank to optimize the backend and workflows of Gramps. It may be among the most significant initiatives undertaken in years. This target identified by @pgerlier is of a similar nature.

It is thankless work that tends to draw nothing but grumbles about the parts that HAVEN’T been addressed yet. Even the excitement of seeing a favorite feature greatly improved tends to generate further demands.

I had a direct supervisor whose favorite phrase was “no good deed goes unpunished” and was almost gleeful trying to demonstrate by making follow-on work as unpleasant as possible. Publicly shaming staff as having caused unauthorized extra burden for coworkers.

This demotivator could not believe that staff was relieved to be allowed to attack things that had been making their jobs difficult. Or when that follow-on work piqued their interest and inspired their creativity rather than irritated them. Or when intense burden in the short run caused huge long-term gains.

Let’s try to do better. And try to do good deeds too.

[comeng]

I was being neither constructive or destructive will my comment I was
merely pointing out a fundamental feature of human endeavour and progress.
GRAMPS is where it is because of a team of dedicated and talented
developers who each have their own fields and bias. For someone with the
expertise and the will to say I want to try and clean up/simplify is
only to be applauded.
phil

[pgerlier]

UPDATE

I went through my modifications and experimented.

My first impulse was to replace aliases with lines like:

StringOption = Option

It is nice and works well in the report description/configuration code. However it completely broke the UI when I requested to create a report.

It comes from the “dynamic” nature of the report dialogs where the option class is used to map to a widget.

My changes above only give a new name to Option but does not create a new class. I had to revert to a (simple) class definition:

class StringOption(Option):
    """
    An option which is a single-line string
    """

That’s all. No __init__, no pass, etc.

The morale of the story is: static code analysis does not reveal all the hidden interactions between different Gramps parts due to the highly dynamic properties of Python. One has to be very careful.