Return bool from setter function?

stan.m

Is there a problem with the following pattern for a Q_PROPERTY setter function?

bool setValue(const int value)
{
    if (value != mValue) {
        mValue = value;
        emit valueChanged(value);
        return true;
    }
    return false;
}

This allows simplifies emitting another signal if the setter assigns the new value. The specific case is the QAbstractItemModel::setData() function with a case statement as follows:

bool PersonModel::setData(const QModelIndex &index, const QVariant &value, int role)
{
    const auto person = mPersons[index.row()];
    bool somethingChanged = false;
    switch (role) {
      ...
    case AgeRole:
        if (person->age() != value.toInt()) {
            person->setAge(value.toInt());
            somethingChanged = true;
        }
        break;
       ...
    }
    if (somethingChanged) {
        emit dataChanged(index, index, QVector<int<() << role);
        return true;
    }
    return false;
}

If the "return bool from setter" pattern were used, the case statement becomes much simpler:

case AgeRole:
    somethingChanged = person->setAge(value.toInt();
    break;

I am considering making this setter pattern a code generation snippet for our company's development team. It works for me, but am I missing something? AFAII, no one does this and I am suspicious that there is a good reason why.

Kent-Dorfman

and what's wrong with the more common pattern of

if (x->GetValue() != choice) {
    x->SetValue(choice);
    // put your emit here!
}

It's about readability. Me thinks you tryin to be too sneaky, so your intent is less obvious to a reader who is not intimately familiar with your methodology.

Also, the setter should simply set the class value, not have hidden side effects. What you propose would fail a lot of the safety critical coding standards because the implicti emit would be seen as a side-effect of the function/method.

stan.m

@Kent-Dorfman Thanks for the response!

BTW, the "common pattern" you show is what we currently have in our property code generator.

The setData() code duplicates the "isChanged" test inside the setValue() function. Performing the same test twice is wasteful and increases the complexity of the code. The sample I show is a reduced version -- the full code has 8 case blocks with 4 different data types. I am mostly concerned about the two QVariant conversion calls; the original setData() code is prime for a copy & paste error.

What you propose would fail a lot of the safety critical coding standards because the implicti emit would be seen as a side-effect of the function/method.

I don't follow your statement about the "implicit emit". The emit in the setter is a standard pattern and the final emit "if somethingChanged" looks explicit to me. Can you elaborate?

Kent-Dorfman

@stan-m I think I was misunderstanding part of the OP. My point was that a method/function should do one thing only and you should avoid code that creates side effects by letting its functionality have hidden or non-explicit behaviour. For the setter that emits a "changed" signal that is probably a valid consequence, based on the needs of the framework.