How to convert to MVC Design?

Curtwagner1984

Greetings,

I have quite a few indicators like this: alt text

They are currently not implement using the MVC Qt classes, the state for the indicator is saved directly in the indicator class.
I'd like to convert this into a MVC design where I'll use a ListView for the view where each row houses an indicator.

On the model side, I'll subclass QAbstractListModel which will have a QList of data structs corresponding to the data the indicator should use.

Where I'm having a problem is, like you see in the picture, some rows have only one indicator while others have three. I'm not sure exactly what's the best way to design a data struct to house them.

I thought about making a base class for the data structure that has a label and subclassing it into a class that has a single led value and another class that has multiple.

This is a somewhat messy class diagram I came up with while prototyping.

alt text

I also have similar structure indicators where the data is a numerical value instead of an LED so this is why it's also in a diagram.

(The diagram contains some errors as I was playing around with Mermaid.)

I feel like this is extremely convoluted and there must be a better way I'm not clearly seeing.

I'll be very happy to hear some advice on how to simplify the design. Maybe my approach is in a completely wrong direction.

Thank you for the help.

mpergand

@Curtwagner1984 said in How to convert to MVC Design?:

I thought about making a base class for the data structure that has a label and subclassing it into a class that has a single led value and another class that has multiple.

Classic way to do it.
The other way is to define a generic class, something like:

class Indicator
{
  IndicatorType type;  // enum of the different types
  QVariantList values;  // various  data values specific to the type
// or QMap(QString, QVariant) if you want to find values by name
}

I'm using this kind of design often.

Curtwagner1984

@mpergand said in How to convert to MVC Design?:

The other way is to define a generic class, something like:

I thought about it. But I do want to seperate the API. For example, single LED indicator need to have a setLedState(LED_STATE_ENUM ledState) method. Where the input for this method will be a state of this single LED. On the other hand, the indicator with 3 axis might have the same method but with other arguments setLEDState(AXIS_ENUM axis, LED_STATE_ENUM state).

With a single class design, I'll have to put both functions in there:

class Indicator
{
    IndicatorType type;  // enum of the different types
    QVariantList values;  // various  data values specific to the type
    // or QMap(QString, QVariant) if you want to find values by name

    void SetLedState(LED_STATE_ENUM ledState);
    void SetLedState(AXIS_ENUM axis, LED_STATE_ENUM ledState);

}

Which will clutter the class.

What do you think about this:

enum AXIS_ENUM { X, Y, Z, NO_AXIS };
enum LED_STATE { LED_OFF,LED_ON_GREEN,LED_ON_RED };
enum INDICATOR_TYPE {LED_INDICATOR_SINGLE, LED_INDICATOR_AXIS, NUMERICAL_INDICATOR_SINGLE, NUMERICAL_INDICATOR_AXIS}

Q_ENUM(AXIS_ENUM)
Q_ENUM(LED_STATE)

/*
Base Indicator class that houses all the data common to all types of indicators
in a QVariant Map. And a Indicator type enum
*/
class IndicatorBase{    

    INDICATOR_TYPE GetIndicatorType(){
        return this._indicatorType;
    }

    protected:
        QMap<AXIS_ENUM,QVariant> _indicatorStateMap;
        INDICATOR_TYPE _indicatorType;
}

/*
LED indicator base. The Led indicators have a boolean variable that determines 
if the indicator is critical or not. It's common to all LED indicators.
*/
class LedIndicatorBase: IndicatorBase{

    public:
        bool GetCriticalStatus(){
            return this._bIsCritical;
        }


    protected:
        bool _bIsCritical = false;
}

/*
Class for a single LED indicator that houses **only the API** for 
a single indicator. While the data is stored in the base class.
*/
class LedSingleIndicator: LedIndicatorBase{
    public:
        LedSingleIndicator(bool bIsCritical, LED_STATE indicatorInitialLedState){
            this._bIsCritical = bIsCritical;
            this._indicatorType = INDICATOR_TYPE::LED_INDICATOR_SINGLE
            this._indicatorStateMap[AXIS_ENUM::NO_AXIS] = QVariant::fromValue(indicatorInitialLedState);            
        }

        void SetIndicatorState(LED_STATE stateToSet){
            this._indicatorStateMap[AXIS_ENUM::NO_AXIS] = QVariant::fromValue(stateToSet);
        }

        QVariant GetIndicatorState(){
            return this._indicatorStateMap[AXIS_ENUM::NO_AXIS];
        }
}


/*
Same as above but for axis LED indicator
*/
class LedAxisIndicator: LedIndicatorBase{
    public:
        LedAxisIndicator(bool bIsCritical, LED_STATE initialLEDsState){
            this._bIsCritical = bIsCritical;
            this._indicatorType = INDICATOR_TYPE::LED_INDICATOR_AXIS;
            this._indicatorStateMap[AXIS_ENUM::X] = QVariant::fromValue(initialLEDsState);
            this._indicatorStateMap[AXIS_ENUM::Y] = QVariant::fromValue(initialLEDsState);
            this._indicatorStateMap[AXIS_ENUM::Z] = QVariant::fromValue(initialLEDsState);
        }

        void SetIndicatorState(AXIS_ENUM axis, LED_STATE stateToSet){
            this._indicatorStateMap[axis] = QVariant::fromValue(stateToSet);
        }

        QVariant GetIndicatorState(AXIS_ENUM axis){
            return this._indicatorStateMap[axis];
        }
}

/*
Numerical indicator have common data in the form of an int that 
determines the decimal percision of the number.
*/
class NumericalIndicatorBase: IndicatorBase{

    public:
        int GetDecimalPercision(){
            return this._decimalPercision;
        };

    protected:
        int _decimalPercision = 4;
}

//Very simular to the LED indicator
class NumericalSingleIndicator: NumericalIndicatorBase{
    public:
        NumericalSingleIndicator(int decimalPercision, double initialValue){
            this._decimalPercision = decimalPercision;
            this._indicatorType = INDICATOR_TYPE::NUMERICAL_INDICATOR_SINGLE;
            this._indicatorStateMap[AXIS_ENUM::NO_AXIS] = QVariant::fromValue(initialValue);
        }

        void SetIndicatorState(double valueToSet){
            this._indicatorStateMap[AXIS_ENUM::NO_AXIS] = QVariant::fromValue(valueToSet);
        }

        QVariant GetIndicatorState(){
            return this._indicatorStateMap[AXIS_ENUM::NO_AXIS];
        }
}
//Very simular to the LED indicator
class NumericalAxisIndicator: NumericalIndicatorBase{
    public:
        NumericalAxisIndicator(int decimalPercision, double initialValue){
            this._decimalPercision = decimalPercision;
            this._indicatorType = INDICATOR_TYPE::NUMERICAL_INDICATOR_AXIS;
            this._indicatorStateMap[AXIS_ENUM::X] = QVariant::fromValue(initialValue);
            this._indicatorStateMap[AXIS_ENUM::Y] = QVariant::fromValue(initialValue);
            this._indicatorStateMap[AXIS_ENUM::Z] = QVariant::fromValue(initialValue);
        }

        void SetIndicatorState(AXIS_ENUM axis,double valueToSet){
            this._indicatorStateMap[axis] = QVariant::fromValue(valueToSet);
        }

        QVariant GetIndicatorState(AXIS_ENUM axis){
            return this._indicatorStateMap[AXIS_ENUM::NO_AXIS];
        }
}


/*
In Subclass of QAbstractListModel. 
It will have a QList<IndicatorBase*> as data.
*/

enum Roles{
    isCriticalRole = Qt::UserRole,
    singleStatusRole = Qt::UserRole + 1,
    xAxisStatusRole = Qt::UserRole + 2,
    yAxisStatusRole = Qt::UserRole + 3,
    zAxisStatusRole = Qt::UserRole + 4,
    decimalPercisonRole = Qt::UserRole + 5,
}

QVariant MyListModel::data(const QModelIndex &index, int role) const{
     if ( !index.isValid() ){
         return QVariant();
     }
        

    IndicatorBase *indicator = static_cast<IndicatorBase*>( index.internalPointer());
    //get the indicator type from the base class. 
    INDICATOR_TYPE indicatorType = indicator->GetIndicatorType();
    switch(indicatorType){
        case INDICATOR_TYPE::LED_INDICATOR_SINGLE{
            LedSingleIndicator* currentIndicator = dynamic_cast<LedSingleIndicator*>(indicator);
            if (role == Roles::isCriticalRole){
                return QVariant::fromValue(currentIndicator->GetCriticalStatus());
            }else if (role == Roles::singleStatusRole){
                return currentIndicator->GetIndicatorState();
            }
            break;
        }
        case INDICATOR_TYPE::LED_INDICATOR_AXIS{
            LedAxisIndicator* currentIndicator = dynamic_cast<LedAxisIndicator*>(indicator);
            if (role == Roles::isCriticalRole){
                return QVariant::fromValue(currentIndicator->GetCriticalStatus());
            }else if (role == Roles::xAxisStatusRole){
                return currentIndicator->GetIndicatorState(AXIS_ENUM::X);
            }else if (role == Roles::yAxisStatusRole){
                return currentIndicator->GetIndicatorState(AXIS_ENUM::Y);
            }else if (role == Roles::zAxisStatusRole){
                return currentIndicator->GetIndicatorState(AXIS_ENUM::Z);
            }            
            break;
        }
        case INDICATOR_TYPE::NUMERICAL_INDICATOR_SINGLE{
            NumericalSingleIndicator* currentIndicator = dynamic_cast<NumericalSingleIndicator*>(indicator);
            if (role == Roles::singleStatusRole){
                return QVariant::fromValue(currentIndicator->GetCriticalStatus());
            }else if (role == Roles::decimalPercisonRole){
                return QVariant::fromValue(currentIndicator->GetDecimalPercision());
            }
            break;
        }
        case INDICATOR_TYPE::NUMERICAL_INDICATOR_AXIS{
            NumericalSingleIndicator* currentIndicator = dynamic_cast<NumericalSingleIndicator*>(indicator);
            if (role == Roles::xAxisStatusRole){
                return currentIndicator->GetIndicatorState(AXIS_ENUM::X);
            }else if (role == Roles::yAxisStatusRole){
                return currentIndicator->GetIndicatorState(AXIS_ENUM::Y);
            }else if (role == Roles::zAxisStatusRole){
                return currentIndicator->GetIndicatorState(AXIS_ENUM::Z);
            }else if (role == Roles::decimalPercisonRole){
                return QVariant::fromValue(currentIndicator->GetDecimalPercision());
            }   
            break;
        }
        
    }

        
}

mpergand

After a quick look at your code, seems ok to me.

Curtwagner1984

Thank you, I hope I'm not overcomplicating this. I just can't seem to see a better solution.