Code review comment for lp:~mixxxdevelopers/mixxx/features_controllerAbstraction

On Mon, Mar 19, 2012 at 4:14 PM, Sean M. Pappalardo
<email address hidden>wrote:

> > * Please make sure to remove commented code referring to non-existing
> stuff
> > * Make sure to delete "res/controllers/Stanton SCS.1m VUMETERS ONLY.xml"
> > * Please delete src/controllers/OLDmidi soon because it makes reviewing
> your
>
> These are among the reasons why I didn't propose a merge yet, silly. :)
>
> > * In defs_controllers.h, make helper functions instead of these macros.
>
> What benefit would that give? The macros are straightforward and fast.
>

Macros should only be used for constants. They're no faster than an inline
function. These particular macros refer to member variables of a class and
have logic in them. This obscures the logic within the class they are used
in because it is hidden behind what looks like a constant. If they have
member variables then they're specific to a class so why not make them a
private inline helper function of that class?

>
> > * Please add "virtual" to every destructor declaration.
>
> As a matter of course? What will that do? I only do it to destructors that
> will be reimplemented.
>
>
It's just something that every class should have. It should be as automatic
as making your member variables private. Some day in the future someone
will subclass your class -- guaranteed. Be nice to them -- it might even be
your future self. :)

> > shared-state-logic-monster. That said, storing the mappings in the
> controller
> > is almost as bad.
>
> I do this because I view a Controller as a device, with all properties
> particular to that device (e.g. a mapping) contained in the one object.
> Then due to the time-sensitive nature of MIDI processing, I imagine local
> object data is faster to access.
>
>
It's fine for the controller to have a mapping. That mapping should be
encapsulated into one class so you can pass them around and vary the
implementation of the mapping independent of the class that consumes the
mapping (the Controller).

Accessing a member variable versus the member variable of another class is
possibly the difference in 1 or two assembly instructions -- if any
difference. If you care about this in the name of speed then I'm surprised
you use so many virtual functions (they are about 2x slower than a regular
function call). This sort of difference is totally meaningless from a Mixxx
performance perspective. We're talking picoseconds here!

Don't let tens or hundreds of assembly instructions get in the way of
writing good code. For the long-term health of the project it's worth it to
structure the project in small, bite-sized and testable classes. This is
extra work, I know.

> > * Take all serialization/deserialization code and put it into a
> > MappingParser/MidiMappingParser helper class which creates a
> > Mapping/MidiMapping class from a given path.
>
> That sounds like overkill. If we do in fact make a MidiMapping class,
> shouldn't the same class that holds the data be responsible for all
> operations on that data?
>

The mapping parsing / serializing code is long enough to warrant living in
its own utility class. It is long and obscures the purpose of whatever
class it lives in (originally MidiMapping, now *Controller).

Ideally every class does one thing and one thing alone. This is generally
called separation of concerns but is also related to encapsulation. Putting
all logic related to a given thing in one class leads to gargantuan classes
that are difficult to read and impossible to unit test (only blackbox
testing is possible).

>
> > * Need to remove all methods that deal with a char*/length data buffer
> e.g.
> > Controller::send, Controller::receivePointer. Instead we should be using
> > QByteArray for everything.
>
> Not all of them can be removed without duplicating code or complicating
> the classes, since the device APIs send and expect data this way. If we
> try, know that interfacing raw bytes between JS and C++ is a very delicate
> issue. I've had numerous problems with bytes getting mangled in the past.
> Plus I imagine primitive data types to be slightly faster. Here again,
> what would be the benefit of switching to QByteArrays?
>

At the point where the rubber hits the road (the device APIs) we will
access the char*/length of the QByteArray. Inside of Mixxx we should not be
fiddling around with pointers to data buffers and trying to allocate them
in one part of Mixxx and free them in another. This is just asking for
segfaults. QByteArray is safer and just as fast as the code as it is
currently written due to Qt implicit-sharing.

>
> > * I'd prefer we get rid of the Controller::send(QList<int>) method and
> instead
> > require people to use QByteArrays
>
> We can't. This is how QtScript passes data byte arrays to C++.
>

Okey, I figured.

> > * midi-mappings-scripts.js You're changing the names of a lot of common
> script
> > stuff. Won't this break MIDI-scripts in the wild?
>
> Yes, but the only things that will be incompatible are MidiButtonState and
> MidiLedState. A README telling to search-n-replace will fix them right up.
> I left backwards-compatible calls for everything else.
>

Hm, I agree with Owen. Breaking scripts in the wild is not an option.
People will install 1.11, say "it breaks my MIDI controller" and install
1.10. Can we keep the ButtonState and LedState as they are for MIDI and
mark them deprecated?

> --
>
> https://code.launchpad.net/~mixxxdevelopers/mixxx/features_controllerAbstraction/+merge/98121
> You proposed lp:~mixxxdevelopers/mixxx/features_controllerAbstraction for
> merging.
>