[C++] наследование vs агрегация - Философия программирования

есть какая-то задача, например слежение за воронами
надо
1) вести учет ворон,
2) считать суммарное число перьев у ворон одного типа

для этой задачи написан класс который делегирует работу двум другим классам, решающем соответствующие подзадачи

class  PlumesCounter
{
public:
    uint getCount(uint corvusType);
    void changeCount(uint corvusType, int delta);
    void setCount(uint corvusType, uint value);
    void reset();
    bool wasUpdated();
};

class RegistryImpl
{
public:
    RegistryImpl(Counter&);

    void update(uint corvusId, uint corvusType, uint plumeCount);
    void remove(uint corvusId);
    void reset();
    uint tryGetType(uint corvusId);
    uint findAnyOfType(uint corvusType);
};

class CorvusRegistry
{
public:
    CorvusRegistry() : counter(), regImpl(counter) {}

    uint getCount(uint corvusType) { return counter.getCount(corvusType); }
    void changeCount(uint corvusType, int delta) { counter.changeCount(corvusType, delta); }
    void setCount(uint corvusType, uint value); // ... тоже делегирование
    void reset();
    bool wasUpdated();

    void update(uint corvusId, uint corvusType, uint plumeCount) { regImpl.update(corvusId, corvusType, plumeCount); }
    void remove(uint corvusId);
    void reset();
    uint tryGetType(uint corvusId);
    uint findAnyOfType(uint corvusType);

private:
    PlumesCounter counter;
    RegistryImpl regImpl; 
};

я смотрю на это и вижу кучу строк кода от которых можно избавиться.
я хочу заменить агрегацию наследованием чтоб выкинуть "ненужные" строчки кода

struct CorvusRegistry : PlumesCounter, RegistryImpl
{
    CorvusRegistry() { regImpl.setCounter(*this); }
};

прав я или не прав?

Здравствуйте, Abyx, Вы писали:

A>для этой задачи написан класс который делегирует работу двум другим классам, решающем соответствующие подзадачи
A>

A>    uint getCount(uint corvusType) { return counter.getCount(corvusType); }
A>    void changeCount(uint corvusType, int delta) { counter.changeCount(corvusType, delta); }
A>    void setCount(uint corvusType, uint value); // ... тоже делегирование
A>    void reset();
A>    bool wasUpdated();
A>

В чем смысл этого кода? Зачем методы агрегированного класса выставлять в интерфейс основного класса?

A>я смотрю на это и вижу кучу строк кода от которых можно избавиться.

Если просто выставить агрегированный класс в публичный интерфейс основного класса, то дублирование кода исчезнет.

Здравствуйте, Abyx, Вы писали:

A>я хочу заменить агрегацию наследованием чтоб выкинуть "ненужные" строчки кода

Без наследования вполне можно обойтись сделав агрегирование видимым для пользователя:

struct CorvusRegistry
{
    CorvusRegistry() { regImpl.setCounter(counter); }

    PlumesCounter counter;
    RegistryImpl regImpl; 
};

Или если религия не позволяет открывать данные, то:

struct CorvusRegistry
{
    CorvusRegistry() { regImpl_.setCounter(counter_); }

    PlumesCounter& counter() { return counter_; }
    RegistryImpl& regImpl() { return regImpl_; }

private:
    PlumesCounter counter_;
    RegistryImpl regImpl_; 
};

Здравствуйте, Abyx, Вы писали:

Непонятно, зачем нужен класс CorvusRegistry в этом примере, если он только и делает, что делегирует все двум классам, которые агрегирует внутри себя.
Если бы у него была какая-то своя внутренняя логика, которая внутри себя использовала PlumesCounter и RegistryImpl, тогда в этом был бы какой-то смысл, но в этом случае у него был бы какой-то свой интерфейс, который бы отражал смысл того, что он делает, а не тупое повторение интерфейсов двух классов, которые он использует для реализации своей логики.

Если же хочется объединить PlumesCounter и RegistryImpl чисто внешне, для того, чтобы клиент понял, что оба класса занимаются воронами, можно создать для них общее пространство имен.

Здравствуйте, Undying, Вы писали:

U>Если просто выставить агрегированный класс в публичный интерфейс основного класса, то дублирование кода исчезнет.

агрегированный класс — это деталь реализации, пользователям класса CorvusRegistry не надо знать с помощью каких классов он работает

Здравствуйте, igna, Вы писали:

I>Без наследования вполне можно обойтись сделав агрегирование видимым для пользователя:

классы PlumesCounter и RegistryImpl это детали реализации CorvusRegistry, коду использующему CorvusRegistry о них знать не надо

к тому же такое решение нарушает Закон Деметры

Здравствуйте, eugene0, Вы писали:

E>Непонятно, зачем нужен класс CorvusRegistry в этом примере, если он только и делает, что делегирует все двум классам, которые агрегирует внутри себя.

это фасад.
кроме того он умеет правильно создавать и настраивать PlumesCounter и RegistryImpl

Здравствуйте, Abyx, Вы писали:

A>классы PlumesCounter и RegistryImpl это детали реализации CorvusRegistry, коду использующему CorvusRegistry о них знать не надо

Это в полной мере относится и к твоему решению.

Здравствуйте, Abyx, Вы писали:

A>к тому же такое решение нарушает Закон Деметры

Это неправильный и вредный закон, нарушающий принцип разбиения сложного на составные части.

Здравствуйте, Abyx, Вы писали:

A>классы PlumesCounter и RegistryImpl это детали реализации CorvusRegistry, коду использующему CorvusRegistry о них знать не надо

Детали реализации, говоришь? Возьми свое решение с публичным наследованием и измени интерфейс какой-нибудь "детали реализации".

Здравствуйте, igna, Вы писали:

A>>классы PlumesCounter и RegistryImpl это детали реализации CorvusRegistry, коду использующему CorvusRegistry о них знать не надо

I>Детали реализации, говоришь? Возьми свое решение с публичным наследованием и измени интерфейс какой-нибудь "детали реализации".

в худшем случае будет

struct CorvusRegistry : PlumesCounter, RegistryImpl
{
    CorvusRegistry() { regImpl.setCounter(*this); }

    void update(uint corvusId, uint corvusType, uint plumeCount)
    {
        RegistryImpl::someMethodName(...);
    }
};

Здравствуйте, Abyx, Вы писали:

A>struct CorvusRegistry : PlumesCounter, RegistryImpl
A>{
A>    CorvusRegistry() { regImpl.setCounter(*this); }

A>    void update(uint corvusId, uint corvusType, uint plumeCount)
A>    {
A>        RegistryImpl::someMethodName(...);
A>    }
A>};

А CorvusRegistry::someMethodName зачем?

Здравствуйте, igna, Вы писали:

I>А CorvusRegistry::someMethodName зачем?

а о нём никто не знает.
кроме того someMethodName можно сделать protected

Здравствуйте, Abyx, Вы писали:

A>а о нём никто не знает.

Тем не менее он будет мусором. Будет такого мусора много, и проживет программа достаточно долго, обязательно кто-нибудь на этот мусор наступит.

A>кроме того someMethodName можно сделать protected

Protected члены относятся к интерфейсу класса. Какой-нибудь другой любитель наследования унаследует от твоего CorvusRegistry; тоже найдет какой-нибудь аргумент в пользу.

Здравствуйте, Abyx, Вы писали:

Правильным решением будет использовать аггреацию и делегирование как вы и делаете.

Если ваша цель уменьшить количество кода, сделать правильно и при этом остаться в рамках С++, то просто забудьте об этом.
В С++ нет возможности этого сделать.

Если у вас этот случай повторяется более одного раза, можно воспользоваться Mixin Pattern , который реализуется с помощью CRTP:

template<typename T>
class PlumesCounterImpl
{
public:
    uint getCount(uint corvusType) { static_cast<T*>(this)->counter.getCount(corvusType); }
    void changeCount(uint corvusType, int delta); // ...
    void setCount(uint corvusType, uint value);
    void reset();
    bool wasUpdated();
};

class CorvusRegistry : PlumesCounterImpl<CorvusRegistry>
{
  ...

private:
 PlumesCounter counter;
}

Если вас устроит хак в виде наследования, то пожалуйста, можете использовать его.
Но потом не жалуйтесь на проблемы в коде

Здравствуйте, _nn_, Вы писали:

__>Если вас устроит хак в виде наследования, то пожалуйста, можете использовать его.
__>Но потом не жалуйтесь на проблемы в коде

а какие именно проблемы могут возникнуть?
компилятор выдавал мне предупреждение "'class1' : inherits 'class2::member' via dominance", но я не понял что в этом плохого

Здравствуйте, Abyx, Вы писали:

A>Здравствуйте, _nn_, Вы писали:

__>>Если вас устроит хак в виде наследования, то пожалуйста, можете использовать его.
__>>Но потом не жалуйтесь на проблемы в коде

A>а какие именно проблемы могут возникнуть?
Проблема наследования, которую вы уже привели.
Можно неявно преобразовать экземпляр класса в базовый класс и это может произойти там где вам это не нужно.

A>компилятор выдавал мне предупреждение "'class1' : inherits 'class2::member' via dominance", но я не понял что в этом плохого
Тут ясно написано что это означает.

Здравствуйте, _nn_, Вы писали:

A>>а какие именно проблемы могут возникнуть?
__>Проблема наследования, которую вы уже привели.
__>Можно неявно преобразовать экземпляр класса в базовый класс и это может произойти там где вам это не нужно.
если вы про срезку — так это с любым наследованием, и от этого помогает noncopyable

к тому же непонятно как можно случайно обратиться к базовому классу, если он деталь реализации (он может быть в namespace detail, иметь имя xyzImpl)

A>>компилятор выдавал мне предупреждение "'class1' : inherits 'class2::member' via dominance", но я не понял что в этом плохого
__>Тут ясно написано что это означает.
я понимаю что это означает, мне непонятно что в этом плохого

Здравствуйте, Abyx, Вы писали:

A>Здравствуйте, _nn_, Вы писали:

A>>>а какие именно проблемы могут возникнуть?
__>>Проблема наследования, которую вы уже привели.
__>>Можно неявно преобразовать экземпляр класса в базовый класс и это может произойти там где вам это не нужно.
A>если вы про срезку — так это с любым наследованием, и от этого помогает noncopyable

A>к тому же непонятно как можно случайно обратиться к базовому классу, если он деталь реализации (он может быть в namespace detail, иметь имя xyzImpl)

В этом случае конечно никто не будет трогать базовый класс.
Но ведь у вас классы PlumesCounter и RegistryImpl общедоступны ?

Если они не общедоступны, то извне разницы не будет никакой.
В реализации, если эти классы используются где-то еще, вы сможете случайно привести ваш класс к ним.

A>>>компилятор выдавал мне предупреждение "'class1' : inherits 'class2::member' via dominance", но я не понял что в этом плохого
__>>Тут ясно написано что это означает.
A>я понимаю что это означает, мне непонятно что в этом плохого
Плохо, что при множественном наследовании с виртуальными функциями будут не всегда очевидные вызовы.

Здравствуйте, Abyx, Вы писали:

A>есть какая-то задача, например слежение за воронами
A>надо
A> 1) вести учет ворон,
A> 2) считать суммарное число перьев у ворон одного типа

A>для этой задачи написан класс который делегирует работу двум другим классам, решающем соответствующие подзадачи
A>

A>class  PlumesCounter
A>{
A>public:
A>    uint getCount(uint corvusType);
A>    void changeCount(uint corvusType, int delta);
A>    void setCount(uint corvusType, uint value);
A>    void reset();
A>    bool wasUpdated();
A>};

A>class RegistryImpl
A>{
A>public:
A>    RegistryImpl(Counter&);

A>    void update(uint corvusId, uint corvusType, uint plumeCount);
A>    void remove(uint corvusId);
A>    void reset();
A>    uint tryGetType(uint corvusId);
A>    uint findAnyOfType(uint corvusType);
A>};

A>class CorvusRegistry
A>{
A>public:
A>    CorvusRegistry() : counter(), regImpl(counter) {}

A>    uint getCount(uint corvusType) { return counter.getCount(corvusType); }
A>    void changeCount(uint corvusType, int delta) { counter.changeCount(corvusType, delta); }
A>    void setCount(uint corvusType, uint value); // ... тоже делегирование
A>    void reset();
A>    bool wasUpdated();

A>    void update(uint corvusId, uint corvusType, uint plumeCount) { regImpl.update(corvusId, corvusType, plumeCount); }
A>    void remove(uint corvusId);
A>    void reset();
A>    uint tryGetType(uint corvusId);
A>    uint findAnyOfType(uint corvusType);

A>private:
A>    PlumesCounter counter;
A>    RegistryImpl regImpl; 
A>};
A>

A>я смотрю на это и вижу кучу строк кода от которых можно избавиться.
A>я хочу заменить агрегацию наследованием чтоб выкинуть "ненужные" строчки кода
A>

A>struct CorvusRegistry : PlumesCounter, RegistryImpl
A>{
A>    CorvusRegistry() { regImpl.setCounter(*this); }
A>};
A>

A>прав я или не прав?

Было херово — стало херово. Что такого особенного в воронах, чтобы их считать отдельно? Нужно воспользоваться подходящим классом коллекции, который поддерживает агрегирующие функции. Из того фреймворка, которым вы пользуетесь. Для дотнета это List<> и LINQ, для плюсов — например, STL'евские контейнеры и for_each.

Ваш личный код должен быть максимально простым и понятным:

enum RavenType
{
    Black = 0,
    White = 1,
}

class Raven
{
    RavenType RavenType { get; set; }
    int FeatherCount { get; set; }
}

Соответственно, задачи типа подсчета перьев воронов одного типа решаются так:

var ravens = ...;
var totalFeathers = ravens.Where(x => x.RavenType == RavenType.White).Sum(x => x.FeatherCount);

или вызовом for_each(). Если хочется избежать копипаста, сделайте свой extension-метод с констрейнтом для коллекций воронов, а уж плюсовый предикат так или иначе придется объявить.

P.S. Потом опять будут говорить, что ООП умерло. Декомпозицию надо делать нормально, а не дурные паттерны фигачить.

	От:	Abyx
	Дата:	12.05.11 22:40
	Оценка:

	От:	Undying
	Дата:	13.05.11 04:59
	Оценка:

	От:	igna
	Дата:	13.05.11 06:50
	Оценка:

	От:	eugene0
	Дата:	13.05.11 07:56
	Оценка:

	От:	Abyx
	Дата:	13.05.11 09:16
	Оценка:

От:	_nn_	www.nemerleweb.com
Дата:	17.05.11 12:58
Оценка: