Am 16.05.21 um 20:28 schrieb Rainer Hans Liffers:
But please allow me to make a little experiment:
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.
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.
Moreover, the names of properties should start with "is*"
So the name should be "isValid" or "is_valid"
(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.
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?
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
(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"
(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).
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.
Which brings me to the next point...
(5) your hierarchy is not clearly thought out.
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
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.
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.
(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.
(7) Hierarchy of Interfaces
I am quite sceptical why Midi::Track inherits from Element
Can you please explain what intention you wanted to express..
- 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?
- 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
(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?
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?
For checking the condition, I'd propose to offer a dedicated
bool hasUniqueMidiChannel() const
- 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.
- 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
- scale() : this function likewise leaves me puzzled.
Is this function meant to change the timings of the track,
e.g. the bpn?
(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?
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.
End of the reviewing game. ;-)
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.
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.
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.
_______________________________________________
Yoshimi-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/yoshimi-devel
