Software design principles and MuseScore

• Mar 7, 2020 - 23:20

I want to share my impressions of getting acquainted with the MuseScore code base.
I touched the code directly when implementing the new feature in the MuseScore, before that, I was only acquainted with libmsсore (MuseScore mobile app) a year and a half.
At the moment, I was suggested to development MuseScore Editor.
And I am sorry for my English.

If it’s simple, there are problems in the MuseScore Editor code base!

This is not criticism, this is expert opinion and superficial review.
My experience is 10 years of professional development and before that 5 years as a hobby

Preamble

First, the brain's ability to process information and operate on entities is very limited. Therefore, one of the important tasks of the developer is the struggle to reduce complexity. By complexity is meant the number of paths and combinations of code execution and the number of entities that you need to keep in mind at one time.

Second, in order not to be left behind (to satisfy needs at the same speed, to keep up with all the others, to remain relevant, etc.), you need to unceasingly change. Those decisions that were once made for certain conditions may be bad for changing conditions, so when conditions change - adding new features, new functionality, new platforms, new formats, lots of new code added, increasing the number of relationships, etc., you need to unceasingly review decisions. And the developer’s task is to sensitively feel this line when the old solution is no longer suitable and a new one needs to be accepted, for example, two classes need to be made from one class, instead of implementation an abstraction should be introduced, modules should be made from a monolith, etc.

There are many sources of information telling how to develop programs. In fact, they all tell how to cope with complexity and stay flexible.

To describe the problems of the MuseScore Editor code, I will use the following sources:

Looking ahead, I’ll say that in the MuseScore Editor code almost all the principles of development are substantially violated.

Single responsibility principle

One of the most important principles of software development, the letter S from SOLID.
That states: every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class, module or function. All its services should be narrowly aligned with that responsibility.

> A class should have only one reason to change.
> Robert C. Martin

Antipatterns: “God object”, “Big lump of mud”, “Magic button”, “Fear of creating new classes” and so on.

MuseScore has many God Objects, for example:

  • MuseScore - musescore.cpp file - 8000 lines, but moreover, most of the methods of the MuseScore class are implemented in other files (excerptsdialog.cpp, exportaudio.cpp, file.cpp, harmonyedit.cpp, etc., a lot of them). The class does a lot, it performs the function of both the controller and the view, and implements a lot of very different logic.
  • ScoreView - scoreview.cpp file - 5165 lines, and also most of the ScoreView methods implemented in other files (dragdrop.cpp, editlyrics.cpp, keyb.cpp, etc., many of them). The class does a lot.
  • Score - the score.cpp file - 4850 lines, and also part of the Score methods implemented in other files (edit.cpp, joinMeasure.cpp, layout.cpp, etc., many of them). The class does a lot

Huge objects with vague responsibility - this is one of the main problems of MuseScore.
This also includes problems with the structure of files and folders, for example, in the main project of the mscore application there are fundamentally implementation files of various functions - the user interface, import of other formats, the midi, the audio driver, the mixer, and various services and all this in one heap (although there are some files that are grouped).

There should be a structure of files and folders, grouped by function. Classes must fulfill one responsibility, for example, if MuseScore is considered a controller, then it should not have the implementation of functions itself, it should only delegate the execution to other classes (and even with such a condition it will be difficult)

Open–closed principle

The letter O from SOLID

That states: "software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification"; that is, such an entity can allow its behaviour to be extended without modifying its source code.

MuseScore is designed in such a way that in order to add a new function, it is necessary to change the implementation of current functions.
For example, to add import of a new file format, you can’t just implement some kind of interface, for example IScoreReader, somewhere to register it and that's it. There is no such interface, you need to change the existing code in different places, and it is even possible to change the behavior of the existing code.

Another example. If you need to add a new representation of notes, the current notes layuoting functions are changed in which a large number of conditions and preferences, these functions turn out to be incredibly complex, and changing them can lead to unpredictable consequences (there are 4550 lines of strongly branching code in the layout.cpp file). Instead, there should be an interface, for example, ILayouter implementations which implement the arrangement of notes for different representations, and the general code should be allocated into small utilitarian functions that can be used or not used (but cannot be changed).

The inability to expand functionality without changing the implementation of the current functional, and therefore again and again, changing the implementation of current functions, for adding new ones (exactly new, not the correction of old), is a universal approach in the MuseScore code. There are no (or very few) abstractions and interfaces, everywhere specific implementations and dependence on them.

Liskov substitution principle

The letter L from SOLID

That states: functions that use the base type should be able to use subtypes of the base type without knowing it.

I will skip the analysis of the implementation of this principle; a deeper immersion in the editor's code base is required.

Interface segregation principle

The letter I from SOLID

That states: too large interfaces must be divided into smaller and more specific ones, so that the software entities of small interfaces know only about the methods that they need to work with. As a result, when changing the interface method, program entities that do not use this method should not change.

In the MuseScore code there are practically no interfaces and abstractions, and there are God Objects, such as the MuseScore, ScoreView, Score that are passed to many methods and are available in very many places, we cannot see where it is called, what it depends on, what it requires, and we cannot limit in dependences to reduce a mess and improve complexity control.

Dependency inversion principle

The letter D from SOLID

That states:

  • High-level modules should not depend on low-level modules. Both should depend on abstractions (e.g. interfaces).
  • Abstractions should not depend on details. Details (concrete implementations) should depend on abstractions.

As was said, in the MuseScore there are practically no abstractions and interfaces.
But there is a dependence of the lower levels on the implementation of the high-level.

For example, a ScoreView is passed to the sequencer, from which it calls some methods. This means, on the one hand, you cannot use this sequencer in a different configuration, with a different view, and on the other hand, the sequencer “knows” about the implementation of the view and what specific methods you need to call, that is, you cannot change the implementation of the view without changing the sequencer.

Another example, in the classes of the domain model OttavaSegment, Spanner, TempoText call method MuseScore::updateInspector, that is, the domain model class “knows" about the presence of an inspector, knows when to call which method. This means that these classes cannot be used in a different configuration, with different behavior of the inspector, or in which the inspector may not be at all (for example, in the mobile application of MuseScore that uses domain model libmscore)

One can also note the widespread use of singleton and global variables (extern).

There should be interfaces, abstractions, and there should be no dependency on a particular implementation. There should be weak binding.


Comments

More in dev chat, but most of this applies to systems structured differently from MuseScore-the-program and is therefore not a problem here.

@igor.korsukov, nice write-up! A few of us have been pushing for changes along these lines for a while, but it's not always easy to achieve coordination in an open source project like this. I'm glad that there is somebody pushing for it on the inside!

Of course it's easy to talk about change in theory, quite another to make it happen in practice, so I look forward to seeing a concrete implementation of your abstract classes. ;-)

First off; I do not disagree that many structural improvements are possible (and some required) within the codebase.

However, I am less inclined to see much of the current situation as "wrong". For example you mention not being easily able to change layouting engines, partially because of the absence of a clean interface there. So far, the target of MuseScore has been western notation only, so there is no need to provide an interface for multiple implementations is your goal was to only support a single implementation anyhow.
What I do agree on is that as soon as the need for support of a different system does come through (such as Jianpu for example) that that is the right time to abstract the first implementation away.

To that end I agree with the presented case of file format support. (Even though most formats already are in separate classes/modules here)

Lines of code imho has never been a good metric for correctness of form. While it may serve indeed as an indicator that the "single purpose" principle may be violated, it in no way guarantees that.
Yes some files and classes are huge or complex; but so are some real-world scenarios (especially layout interaction between different elements).
Score has the properties of being a God-like object, because it is being handled down into many different modules; but it is also the main object of interest and editing of this software so it makes sense that it would interact with nearly all other modules.

In conclusion; yes, there are many areas where structure can be improved and some where it should/must be. But I also think that it makes sense to do that refactoring as part of the functional change that requires it; and not out of pure possible future proofing of the codebase.
After all, nobody knows what the future brings, and in hindsight you might just waste time in restructuring something that doesn't really require it.

In reply to by igor.korsukov

i don't understand the point of this thread. I speak from long career as a professional software engineer, by the way. I don't read here anything more than noble, abstract design principles that are surely worthwhile to bear in mind, and emphasize, and which, if followed wisely and skillfully in a new project, will indeed foster a more understandable and maintainable codebase than the overnight job of a bunch of high-school hackers hired to produce it in mumblescript. Yes, professionalism involves knowledge and skill, whether it's a codebase or a cantata being constructed, and done properly, programming is an art.

But to approach a large, functioning, in-use codebase and say, "Why, what a bunch of nonmodular kludgey junk! Toss it! Demolish it, and let arise a new tower built with modern construction principles and materials!" is pointless. Destabilization of a functional, complex, in-use product is a higher NEGATIVE design principle than any of the laudable desiderata you promote. Even if you proposed some speciific (but global) change to the architecture of a given object or object family, destabilization for the sake of code beauty is the polar opposite of professionalism and responsible product management, and strongly to be discouraged.

In reply to by BSG

I do not propose a revolution, I am for evolutionary development. But I believe that everything can change and improve, and not just for the sake of beautiful code. If there is a choice to complicate the current classes and their relationships, or vice versa, make the system easier, make some kind of responsibility from a separate class, then you need to make a new class.
The problem is, adding new functionality will significantly change the old one - this greatly inhibits development. We recently looked at Pull Requests - and it’s just scary to merge them, because they change a lot, a lot of relationships, one change breaks another ... - this is a problem.

Do you still have an unanswered question? Please log in first to post your question.