### Denali UI Code Structure Review #### Peter Lucia plucia@diality.com Apr 3, 2020 #### Model View Controller While it may require more components, it will be more easily maintainable, so I think it's a good choice. - https://en.wikibooks.org/wiki/C%2B%2B_Programming/Code/Design_Patterns#Model-View-Controller_(MVC) "Newcomers will probably see this "MVC" model as wasteful, mainly because you are working with many extra objects at runtime, when it seems like one giant object will do. ***But the secret to the MVC pattern is not writing the code, but in maintaining it, and allowing people to modify the code without changing much else."*** It seems we have many models and many views, but only one main controller class, applicationcontroller.cpp, and two other helper controller classes: messagedispatcher.cpp and guicontroller.cpp. As we add views and models, we may want to think about ways of carefully splitting this up into multiple controllers that have their own domain of responsibility. #### Use of Singletons From main.h: ``` // TODO : A singleton parent class needs to be created // to taking care of the Threading, init, quit, and so // TODO : // - We still need to work on threading on other classes // - We need to have a singleton parent class // - Some code has been added to debug can interface (We still have swap frames) #define SINGLETON(vCLASS) \ private: \ explicit vCLASS(QObject *parent = nullptr); \ virtual ~vCLASS() { } \ vCLASS(vCLASS const &) = delete; \ vCLASS & operator = (vCLASS const &) = delete; \ public: \ static vCLASS &I() { \ static vCLASS _instance; \ return _instance; \ } \ ``` It appears that we are using a form of the Meyers Singleton pattern, which it seems the online community endorses as being thread safe. - https://stackoverflow.com/questions/12248747/singleton-with-multithreads "In C++11, the following is guaranteed to perform thread-safe initialization:" ``` static Singleton* getSingletonInstance() { static Singleton instance; return &instance; } ``` - https://www.modernescpp.com/index.php/thread-safe-initialization-of-a-singleton “The beauty of the ***Meyers Singleton*** in C++11 is that it's automatically thread safe." - From Modern C++ Design: Generic Programming and Design Patterns Applied, the Meyers Singleton is as follows: ``` Singleton& Singleton::Instance() { static Singleton obj; return obj; } ``` Could there be any flexibility lost by using a macro in main.h for the singleton constructor? What if we eventually need to pass class-specific parameters to the singleton constructor? If we're going to use macros a lot, it's probably a good idea to add a doxygen style code documentation above them in the so it's clearer what their intended use and function is. It seems like the PROPERTY macro as it seems like it will simplify creating properties and lines of code. I like it. Do you think it could limit or restrict us in any way? ``` #define PROPERTY(vTYPE , vVARIABLE , vDEFVALUE, vALWAYSEMIT ) \ Q_PROPERTY(vTYPE vVARIABLE \ READ vVARIABLE \ WRITE vVARIABLE \ NOTIFY vVARIABLE##Changed) \ Q_SIGNALS: \ void vVARIABLE##Changed \ ( const vTYPE & v##vVARIABLE ); \ private: \ vTYPE _##vVARIABLE = vDEFVALUE; \ vTYPE vVARIABLE () const { \ return _##vVARIABLE ; \ } \ void vVARIABLE ( const vTYPE & v##vVARIABLE ) { \ static bool init = false; \ if ( vALWAYSEMIT \ || ! init \ || _##vVARIABLE != v##vVARIABLE ) { \ DEBUG_PROPERTY_CHANGED(vVARIABLE) \ init = true; \ _##vVARIABLE = v##vVARIABLE; \ emit vVARIABLE##Changed( _##vVARIABLE ); \ } \ } ``` #### Coding Style and Documentation Coding style could be more consistent. I'm a big fan of automating code styling with tools like cpplint. This avoids spending valuable time in code reviews fretting over styling issues that can easily be automated away. Here is one example of the code styling inconsistencies I noticed: The closed parentheses () are spaced far away from the method names here in guiview.h: ``` private slots: void onActionReceive (GuiActionType vAction, const QVariantList &vData); // UI <= HD/DG void doUSBDriveMount (); void doUSBDriveRemove(); void doExport (); public slots: // is public since will be used in the UI and is in the same thread. void doActionTransmit(GuiActionType vAction, const QVariantList &vData); // UI => HD/DG void doActionTransmit(GuiActionType vAction, const QVariant &vData); // UI => HD/DG void doUSBDriveUmount(); void doExportLog (); ``` But then if we go to messagedispatcher.h, for example, the spacing is different; ``` public slots: bool init(); bool init(QThread &vThread); private slots: void quit(); public: void enableConsoleOut(bool vEnable) { _builder.enableConsoleOut(vEnable); } private: void initConnections(); void initThread(QThread &vThread); void quitThread(); ``` Could use more intention-revealing names. For example, the Data struct used in many model classes isn't particularly revealing. Could it be renamed to something else that would give more of a hint about what is included in the structure? The prefix choices of "m", and "v" don't signify a clear meaning to me. Typically, the "m" prefix is used to indicate a member variable of a class. The "v" prefix seems to indicate a local variable. It seems the underscore prefix is used to indicate that a function or variable should be private. PEP8 in python follows this convention as well. Since our development environments easily provide this information with a hover or Ctrl+Click easy navigation to the definition of variables and function names, I'm wondering if we really need these single character prefixes. I think the code could be more readable without them, but if you prefer to keep them that is absolutely fine with me too. In many model classes, I see the Data struct is defined like the following example in mpoweroff.h: ``` public: QString stringPrefix = "PowerOff"; struct Data { quint8 mStatus = 0; bool mShutDown = false; }; ``` As the return type of the data() function, it's not quite clear at first what it does. Since the data() function is in a bunch of model classes, maybe adding the prefix, "get" to it would make its use clearer. ``` Data data() const; ``` The naming for signals could be made clearer. From our coding standard "4. Functions and variables should have descriptive and meaningful names or well-commented declarations; variables with a small scope can have short names." For example, how about `receivedAction` instead of `didActionReceive` or `mountedUSB` instead of `didUSBDriveMount` in guicontroller.h. ``` signals: void didActionReceive (GuiActionType vAction, const QVariantList &vData); // UI <= HD/DG void didActionTransmit(GuiActionType vAction, const QVariantList &vData); // UI => HD/DG void didUSBDriveMount (); void didUSBDriveUmount(); void didUSBDriveRemove(); void didExportLog(); void didExport (); ``` In messageglobals.h, the Can_Id uses a mix of snake and mixedCase. Couldn't it just be mixedCase? Is the "e" character prefix really needed if the IDE can tell us it's an enum? According to SOP-0026, Software Coding Standards, C++.docx, "Function/Method" and variable names must be Camel Case" ``` enum Can_Id : quint16 { eChlid_LOWEST = 0x7FF, eChlid_NONE = 0x7FF, // Broadcasts //// Alarm eChlid_HD_Alarm = 0x001, ///< HD alarm broadcast eChlid_DG_Alarm = 0x002, ///< DG alarm broadcast eChlid_UI_Alarm = 0x004, ///< UI alarm broadcast [Out] //// Sync eChlid_HD_Sync = 0x040, ///< HD sync broadcast eChlid_DG_Sync = 0x080, ///< DG sync broadcast eChlid_UI_Sync = 0x200, ///< UI sync broadcast [Out] // UI not listening eChlid_HD_DG = 0x008, ///< HD => DG eChlid_DG_HD = 0x010, ///< DG => HD // UI is listening eChlid_HD_UI = 0x020, ///< HD => UI eChlid_UI_HD = 0x100, ///< UI => HD [Out] // UI lessens occasionally eChlid_DG_UI = eChlid_DG_Alarm, ///< No direct channel has been defined between DG&UI, May be required for logging eChlid_UI_DG = eChlid_UI_Sync , ///< No direct channel has been defined between DG&UI, May be required for logging }; ``` The styling in the cpp views is a mix of snake case and mixed case as well: For example, in vtreatmentbloodflow.cpp: ``` bloodFlow_FlowSetPoint (vData.mFlowSetPoint ); bloodFlow_MeasuredFlow (vData.mMeasuredFlow ); bloodFlow_RotorSpeed (vData.mRotorSpeed ); ``` A minor point, but I did come across a few self describing functions that don't have doxygen comments (format.cpp and mtreatmenttime.cpp) If we have code coverage requirements for documentation purposes, I suppose this will eventually need to be addressed.