На сколько плохо такое решение?

136

30 ноября 2019, 19:00

Есть функция сравнивающая два объекта и фиксирующая что именно различается в объектах. В словаре хранятся типы изменений и указатели на метод свойства объекта. До этого была конструкция из if`ов. Но мне кажется что такой подход слишком сложен для понимания. Как еще можно сравнить два объекта на различия.

UserEventList MonAdmin::compareObjects(const MonObject &oldObject, 
                                       const MonObject &newObject)
   {
        UserEventList list;
        QMap<UserEvent::EventType, QString (MonObject::*)() const> map;
        map.insert(UserEvent::ChangeName, &MonObject::name);
        map.insert(UserEvent::ChangeParent, &MonObject::parentToString);
        map.insert(UserEvent::ChangeSim, &MonObject::simNumber);
        map.insert(UserEvent::ChangeUid, &MonObject::uid);
        map.insert(UserEvent::ChangeClient, &MonObject::clientName);
        map.insert(UserEvent::ChangeDeviceName, &MonObject::deviceName);
        map.insert(UserEvent::ChangeDeviceSerial, &MonObject::deviceSerial);
        map.insert(UserEvent::ChangeAutoProfile, &MonObject::autoProfileToString);
        map.insert(UserEvent::ChangeConfiguration, &MonObject::configToString);
        QMapIterator<UserEvent::EventType, QString (MonObject::*)() const> it(map);
        while (it.hasNext()) {
            it.next();
            QString oldValue = (oldObject.*(it.value()))();
            QString newValue = (newObject.*(it.value()))();
            if (oldValue != newValue) {
                UserEvent event;
                event._type = it.key();
                event._oldValue = oldValue;
                event._newValue = newValue;
                list.append(event);
            }
        }
        return list;
    }

Answer 1

Такое решение читается лучше чем цепочка if-ов, потому что компактнее.

Однако, есть и недостатки: (1) большое число аллокаций памяти на куче (в map.insert), которые делаются при каждом сравнении, и которых вообще небыло в реализации через if. (2) Обход map-а через итераторы, имеет оверхед, в сравнении даже с обходом массива. При этом ассоциативность контейнера map не используется никак. (3) Использование сложносочиненных шаблонных типов (например QMapIterator<UserEvent::EventType, QString (MonObject::*)() const>), не облегчает чтение. (4) В отличии от от варианта с if, добавить поле, типа int, окажется сложно. (Нужен переход на QVariant, или аналоги).

Эти недостатки можно исправить:

Введем отдельный тип для геттеров.

typedef QString (MonObject::*t_MonObject_getter)() const;

Вместо локального объекта map, ввел бы статический массив MonObject_meta_info

struct {
    UserEvent::EventType eventType;
    t_MonObject_getter   getter;
} static const MonObject_meta_info[] =
{
    { UserEvent::ChangeName  , &MonObject::name            },
    { UserEvent::ChangeParent, &MonObject::parentToString  },
    ...
};

(Вместо анонимной структуры можно использовать именованную структуру или std::pair, в зависимости от принятого code-style.) Этот массив инициализируется единожды, на этапе старта программы, и расположен в памяти непрерывно, что удешевляет итерирование по нему, и позволяет копилятору вообще развернуть цикл.

Цикл обхода массива MonObject_meta_info читается лучше всего, если записан без явных итераторов или индексов:

for( auto& it: MonObject_meta_info )
{
        QString oldValue = (oldObject.*(it.getter))();
        QString newValue = (newObject.*(it.getter))();
        if (oldValue != newValue) {
            UserEvent event;
            event._type = it.eventType;
            event._oldValue = oldValue;
            event._newValue = newValue;
            list.append(event);
        }            
}

Недостаток (4) не праоделим, без существенного усложнения кода, что оправдано только если имеется большое число объектов (или полей) для которых нужно производить подобное сравнение.