Gee, Hermann, we live in two different worlds! 😂
Please allow me to supply a review of your review, also for everyone else,
so that all of us can subsquently draw the right conclusion. Sorry about
the length of this email!
Your reply below demonstrates that you believe that the available features
of the specific programming language used shall determine the way in
which the program is designed and implemented: You impose the available
features on the way of thinking. This is what most programmers do.
Whereas I believe that only mathematical principles shall guide the way in
which a program is designed and implemented: I impose these principles on
the available features of the programming language used. -
As a result, you think in concrete terms, whereas I think in abstract terms.
Also see further below.
On 17/5/21 5:13 am, Ichthyostega wrote:
ok... let's play that reviewing game!
Let's assume that this is some pull request or code a colleague has
written,
and let's assume that I would be the reviewer.
Then this would be my response...
(1) the code contains lots of redundant comments. Please delete all of
them.
Please retain only comments which do say something substantial in
addition
to what the method signature already states.
The code is for the /compiler/, and I assume you agree that achieving
minimum
code redundancy is a worthwhile goal.
Comments in natural language prose, on the other hand, are for /humans/. In
this context, redundancy is encouraged, if only to prevent misunderstandings
by the human reader. I am confident that no psychologist will disagree
with me. 😁
In general, I like to present include files in a form which not only
contains all
necessary and sufficient information for the compiler, but also for the
human
user of the specified abstraction(s). Hopefully, the degree of
redundancy is
sufficient to avoid misunderstandings, but due to the nature of natural
languages,
this cannot be guaranteed.
This is a design decision based on my personal taste: It avoids having
to refer to
separate documentation.
Example:
// determine if item is valid
virtual bool valid () const = 0;
// determine if track is valid
bool valid () const override;
The comment fails to define what "valid" means.
Exactly, and quite deliberately so. The former expresses the fact that
the designer of any class derived from /Item/ must determine what "valid"
means, and the designer of the latter has obviously decided to express
the meaning purely via the implementation.
Moreover, the names of properties should start with "is*"
So the name should be "isValid" or "is_valid"
This is typical concrete thinking. All right, I will rename the
function "pipapo". 😁
(2) design question: is it really a good idea to offer an "bool"-like
operator
to check validity, as you did on class Item? Sometimes this can be
the case,
but an outside user could as well expect, that a bool check on an
item
checks for emptiness.
By all means - if you derive a class from /Item/, you are welcome to let
function
/valid/ check for emptiness. Maybe your derived class objects are
guaranteed
never to be invalid in the mathematical sense of existence. The latter
is my
preferred kind of a concrete implementation of function /valid/.
Even more? Why do we need to check an Item for being "valid"?
wouldn't it be better design that an invalid item can not
be constructed at all?
Oh Hermann, please.... So you think that after successful construction
everything
is hunky-dory? Think again: A class usually also contains non-constant
functions
and operators. Obviously, the execution of each one of those can change the
validity of an object (whatever that means 😁), ie the result of a
subsequent
application of function /valid/.
Furthermore: Yes, it can indeed make sense to construct an invalid
object (ie
an object for which /valid/ returns /false/). For instance, in order to
enforce the
generation of an object by other means (ie a different function or
operator).
If you really want to retain that operator, then please
- are you sure your safe bool idiom really prevents automatic
conversion?
void* is an rather generic and widely used type. A undisclosed
member
pointer is much safer in esoteric corner cases.
- better use an explicit conversion operator, then the compiler
knows we
do not want implicit conversion, but only usage in a bool{ }
context
There is an easy solution if you do not wish /operator const void */
being used
for an implicit conversion: Do not use base class /Item/! 😂
And BTW, if you think that way, you should also have a serious word with the
evil designers of standard C++ I/O streams!
The purpose of class /Item/ is simply as follows: If you decide to
derive a class,
say /X/, from /Item/, you must define function /valid/ and _also specify
what it means_.
Subsequently, if /x/ is an object of your class /X/, then you may use
the convenient
notation /if (x)/ to check if /x/ is valid, and /if (! x)/ to check if
/x/ is not valid. Moreover,
you cannot make use of a pointer to /Item/ in order to delete a derived
class
object either (which is why the destructor is protected), and if your
derived
class has a destructor, then the implicit call to the underlying
/Item/'s destructor
has no effect (because it does not need to have one).
And that is /all/. Class /Item/ has no other purpose.
(3) Naming
Names should communicate what the stuff is.
"Item" "Object" etc is way to generic and void of meaning.
I would propose
- either to go for a MidiItem and a TextItem, i.e. for specific items
- or make it a Mix-In: then rename the "Item" to "ValidityCheckable"
Just more examples of concrete thinking. Again, if I want to know what
a function actually does, I either refer to the comments describing its
purpose,
or to its implementation, or both. I /never/ rely purely on a
function's name
to understand its meaning. (There is not sufficient redundancy in just a
simple name so that I have the chance to understand its meaning like
everyone else.)
(4) Making the Constructor and the Destructor of an Interface inline
can be
dangerous. You will risk getting into linking problems, which are
sometimes
hard to diagnose. Also I would recommend to have at least one
non-virtual
function of a base class defined non-inline within some
translation unit.
Because this is the place where the compiler emits the VTable. And
this
should happen only once (otherwise: again hard to diagnose linking
and
consistency problems. Been there, seen that).
Now you are talking about compiler and linker deficiencies. Fair enough
- if you
cannot trust the compiler and/or linker to do the right thing, you need
to change
the code accordingly.
Moreover: why do you make the destructor protected but virtual?
This is a very unusual combination. Usually you make a destructor
virtual,
because you want people to handle elements of derived class
through that
interface. If you just want to express that this is only a Mix-In and
should always be deleted from the child class, it is sufficient to
make
it non-virtual and protected.
Indeed, that is unusual, but there is a reason. See further above.
Which brings me to the next point...
(5) your hierarchy is not clearly thought out.
I beg your pardon? 🙁 I prefer to ignore this statement.
What is meant to be the interface for using those entities?
This should be named clearly, and it should have the full contract.
The way the interfaces are laid out seem to indicate that a clear
decision
about that is avoided. Moreover, you assemble the inheritance
based on
conditional guards. This is a clear no-go.
You must not do that for interfaces.
What is the point of an interface that sometimes means this,
but with special includes means something different?
Sometimes this might be acceptable for some very technical and
ephemeral
stuff. But not for something fundamental. A Midi-Element is something
entirely different than an Text-Element
No, it is not: A /Midi::Element/ is either a /Text::Element/ plus more
functionality, or
it is just a /Support::Item/ plus more functionality, depending on whether
/TEXT_PROCESSING/ is defined or not.
This design decision is based on the fact that some applications making
use of
the supplied classes require the functions of /Text::Element/, and
others do not.
Instead of using compile time value /TEXT_PROCESSING/ in order to express
whether or not functionality to convert midi files to textual
representations and
vice versa shall be included or not, I could have
(1) defined each derived class twice, one with the functions enforced by
/Text::Element/, and the other one without. This approach would have almost
doubled the size of the source code.
(2) not bothered at all, ie always derive /Midi::Element/ from
/Text::Element/. This
approach would have led to larger executables (including unnecessary code)
for those applications that do not need the functionality enforced by
/Text::Element/.
Please try to make one abstract interface, which has the full
contract,
and try to make that interface clear and stable and hide those
details.
In this way, you would force any derived class which eg only wishes to
specify
validity, to also define the functions enforced by /Text::Element/ and
/Midi::Element/.
This does not only lead to code redundancy, but is simply a nuisance.
Having Midi::Element inherit from Text::Element just based on the
presence
of some compilation guard has the smell of "using inheritance for
sharing
implementation". Please avoid that, it makes code hard to maintain.
Prefer composition over inheritance in such cases.
Show me how to do that without having to define each derived class twice.
(6) Signatures: If you want a constraint and guide the users of your
interface,
please use dedicated types. In your case, you offer functions on
Midi::Track
to encode and decode some stream, but a vital constraint is only
stated
in the comment.
Better offer a function to decode(MidiStream &) etc.
And then offer a factory function to build such a MidiStream.
This way the functions can only be used the correct way.
No, that is /not/ a vital constraint - on the contrary. Again, it is up
to the user of this
class to define what a /Midi::istream//or Midi::ostream/, respectively,
is. They may
simply be /std::istream/ or /std::ostream/, or your own derived classes,
or somebody
else's special stream classes.
(7) Hierarchy of Interfaces
I am quite sceptical why Midi::Track inherits from Element
Can you please explain what intention you wanted to express..
The only purpose of the abstract base classes /Support::Item/,
/Text::Element/, and
/Midi::Element/ is to enforce the existence of the functions specified
as pure virtual
in the respective classes, and to provide convenient, uniform operators
for all
derived types and applications making use of those functions.
- did you mean that a MIDI track is a special kind-of Element?
That is, can a MIDI-track really stand-in at each and every place
where an Element is expected?
Yes, you can rightly say that a /Midi::Track/ object is also a
/Midi::Element/ object. But
nobody expects a /Midi::Element/ object anywhere - not even a pointer to
such an
object. This is an /abstract/ base class!
- or did you rather mean that a MIDI track has the capability
of being received/sent to a MidiStream?
In that case, Element is a capability Mix-in and should be named
appropriately, e.g. MidiStreamable
The answer to your question is "yes". Your suggested alternative design
method
is something which I find less convenient.
(8) some functions do confuse me, and I'd need some clarification
- uniqueChannel() : is this a function to check a condition,
or does it serve to extract "the" channel of the track?
The specified alternative return values in the comment should tell you
the whole story. If not, the comment is not sufficiently redundant -
sorry about that. 😂
Wouldn't it then be better to return a vector with all the
channels used? Or to prohibit having several channels mixed
in one MidiTrack?
Regarding the first question: There is also a function called /allChannels
/which you obviously missed.
Regarding the second question: If I would do that, then class /Midi::Track
/would not be able to manipulate any kind of track in a standard midi file.
For checking the condition, I'd propose to offer a dedicated
bool hasUniqueMidiChannel() const
Here you are:
inline bool hasUniqueMidiChannel () const
{ return !! uniqueChannel (); }
- clear() : what does this function really do?
the comment is confusing, does a MidiTrack need to be initialised?
That means, a MidiTrack is stateful and can be in uninitialised
state??
Which is something I'd recommend to avoid at all cost.
Maybe you need to brush up your knowledge of the Standard C++ Library?
It uses function /clear/ all over the place, and I use that name with
the same
semantics.
- timeCodes() : the name is not clear; since the implementation
is inline, I'd propose to call that function CntTimeCodeMessages()
or similar, so that its meaning is clear in usage context
Again: The name does not matter. Refer to the associated comment, please.
- scale() : this function likewise leaves me puzzled.
Is this function meant to change the timings of the track,
e.g. the bpn?
The events of a track do not contain bpn values, but rather delta times
relative
to a bpn defined elsewhere. Their scaling changes the /accuracy/ of the
delta
times, but /not/ the required time period for the track, provided that
the midi
file's division value is changed accordingly). Sorry, I did not attach
include files
/midi_event.h/, /midi_quantity.h/, and /midi_division.h/, where you
would have found
appropriate explanations. I wanted to keep the experiment as lean and mean
as possible.
(9) Mutability
Does MidiTrack really need to be mutable?
In the old times we used to do that liberally, but in fact
mutable objects and manipulation by side-effect lead to way more
complicated systems which are hard to reason about, and which are
hard to parallelise. Can we think of making Midi::Track immutable?
No, we cannot, because otherwise it would not be possible to extract
a track from a midi file, and function /merge/ would not work either.
Now, let /t/ be an object of type /Midi::Track/, and its contents have
already
been generated (eg via extraction /operator >>/ applied to a midi stream,
or function /merge/ supplied with a vector of existing tracks).
Furthermore,
we know that /t/ is /valid/ (because we have made use of /if (t)/ or /if
(! t)/,
respectively). Hence, /t/ is now a valid and useful track.
Let /E/ be an environment in which an immutable track is required. If we
want to pass /t/ to any function /E::f/, all we need to do is to declare the
applicable track argument to be of type /const Midi::Track &/ (which is
sufficient if /t/ is not modified somewhere else in the meantime), or of
type
/const Midi::Track/, which will generate an exclusive constant copy of
/t/ for
environment /E/.
That is, the decode() operation should be better located on some
decoder component, which returns a fresh new midi track, and
split and scale should likewise return a new mutated copy.
I know, sometimes there are performance problems, but typically
modern machines are optimised for copying moderate amounts of data
And the benefits in maintainability are worth going for
a design with immutable data most of the time.
See further above.
End of the reviewing game. ;-)
Good.
In my work job, I would reject the review and the ticket would go back
to the author of the code, and we'd sit together next day and discuss and
resolve those points, typically in pair programming. We do code reviews
on every major change and I am convinced this is the best way towards
software quality. I consider code reviews much more valuable than
any fancy linters or code metrics tools.
Code reviews can only be successful if all programmers involved are either
abstract thinkers or concrete thinkers. A mix always leads to conflict.
I know this from lots of personal experiences. 😁
I know, all Bosses of the world love code metrics. But what is what
they get?
Code optimised to look good in the statistics report.
Code optimised to make the linter happy.
Because bosses usually do not have sufficient mathematical knowledge.
Yes, I agree, a well designed software is much easier to maintain
and work with. Especially when it is built such that the user of an
interface doesn't have to "open the box", but can rely on the interface
and its contract alone.
I fully agree with you here. But unfortunately, design based on concrete
thinking tends to lead to unmotivated classes (which actually are /not
/classes, but concepts to be distributed over several classes),
typically with
an over bloated, and therefore unmanageable, amount of data members.
Whereas design based on abstract thinking and mathematical principles
can only lead to classes the existence of which can be formally justified,
and the number of data members is always easily manageable and
comprehensible, as mathematicians very much enjoy the /divide-and-conquer
/method when defining abstractions. Which means that rather than a
massive, incomprehensible class with countless data members, you get
a hierarchy of sub-classes, each one of which is easy to manage. On top
of that, the total amount of code is always significantly smaller than the
amount of code generated by concrete thinkers.
I am afraid, without abstract thinking and using mathematical principles
/before/ even writing a single line of code, you always end up with a mess.
The /Yoshimi/ code is a good example for that.
Conclusion: Hermann and I are presently incompatible, and it is likely that
I am also incompatible with everyone else.
There are two ways to solve this problem: Either Hermann (and most likely
everyone else 😁) is willing to learn how to think only in abstract
terms and
to change the existing /Yoshimi/ code accordingly, or I resign from the
/Yoshimi/
project.
I am confident that all of you will prefer the second option. That is fair
enough, and since there is always the possibility for me to grab a copy
of the
/Yoshimi/ code just for reference and to design my own synthesiser, I am not
too worried either. 😁
I wish all of you success in your future endeavours.
Cheers, guys
Rainer
_______________________________________________
Yoshimi-devel mailing list
Yoshimi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/yoshimi-devel
