search

alexanderBaranov / OOP

OOP
0 stars 0 forks source link

Замечания по программе "Адресная книга" #10

Open alexey-malov opened 8 years ago

alexey-malov commented 8 years ago

Именование констант должно быть в виде SOME_CONSTANT_NAME

проверено

alexey-malov commented 8 years ago

Жестко заданное имя файл в самом классе надо заменить на передаваемое извне. Это облегчит тестирование класса и не будет требовать прав на запись в каталог с программой.

Тесты не должны модифицировать свои исходные файлы. Вместо этого они должны работать с копией, создаваемой (желательно) во временном каталоге. В boost::filesystem есть функция, позволяющие узнать путь к каталогу для временных файлов.

alexey-malov commented 8 years ago 
    subscribers const FindByName(const std::string name) const;
    subscribers const FindByAddress(const std::string address) const;
    subscribers const FindByTelephone(const std::string telephone) const;
    subscribers const FindByEmail(const std::string email) const;

Лучше возвращать массив не константный. Это позволит заменить копирование на перемещение.

проверено

alexey-malov commented 8 years ago

class CSubscriber { public: CSubscriber(); ~CSubscriber();

    void SetIndex(std::string index);
    int const& GetIndex() const;

Почему индекс возвращается по константной ссылке? Почему индекс передается в виде строки?

alexey-malov commented 8 years ago 
class CSubscriber
{
public:
    CSubscriber();
...
    int m_index;
    std::string m_name;
    std::string m_surname;
    std::string m_patronymic;
    std::vector<std::string> m_email;
    std::vector<std::string> m_telephoneNumber;
    std::string m_street;
    std::string m_house;
    std::string m_apartment;
    std::string m_city;
};

Поле index не инициализируется ни в конструкторе, ни в объявлении класса, а значит, абонент после своего создания невалиден.

Задача конструктора - создать готовый к использованию объект. Здесь же объект недоинициализирован.

alexey-malov commented 8 years ago

Сделать, чтобы поиск абонента по индексу выполнялся быстрее O(N)

alexey-malov commented 8 years ago 
    void UpdateSubscriber(const int index,
                        const std::string name,
                        const std::string surname,
                        const std::string patronymic,
                        const std::string email,
                        const std::string telephonNamber,
                        const std::string street,
                        const std::string house,
                        const std::string apartment,
                        const std::string city);

строки передавать по констнатной ссылке

alexey-malov commented 8 years ago 
    void SetIndex(std::string index);
    int const& GetIndex() const;

    void SetName(std::string name);
    std::string const& GetName()  const;

    void SetSurname(std::string surname);
    std::string const& GetSurname()  const;

    void SetPatronymic(std::string patronymic);
    std::string const& GetPatronymic()  const;

    void SetEmail(std::string email);
    std::string const GetEmail()  const;

    void SetTelephoneNumber(std::string telephoneNumber);
    std::string const GetTelephoneNumber()  const;

    void SetStreet(std::string street);
    std::string const& GetStreet()  const;

    void SetHouse(std::string house);
    std::string const& GetHouse()  const;

    void SetApartment(std::string apartment);
    std::string const& GetApartment()  const;

    void SetCity(std::string city);
    std::string const& GetCity() const;

    bool FindByName(std::string name) const;
    bool FindByAddress(std::string address) const;
    bool FindByTelephoneNumber(std::string telephoneNumber) const;
    bool FindByEmail(std::string email) const;

строки передавать по константной ссылке. возвращать либо по коснтантной ссылке, либо по неконстантному значению.

alexey-malov commented 8 years ago 
class CSubscriber
{
    bool FindByName(std::string name) const;
    bool FindByAddress(std::string address) const;
    bool FindByTelephoneNumber(std::string telephoneNumber) const;
    bool FindByEmail(std::string email) const;

Когда мы что-либо ищем, мы ожидаем получить искомую сущность, а не true/false

CSubscripber subscriber;
subscriber.FindByName("Ivan")

Методы переименовать так, чтобы при их использовании было ясно, что они делают

**UPDATE***: Имелось в виду, что надо изменить название метода, например на: HasName, HasAddress, HasPhoneNumber, HasEmail:

if (subscriber.HasEmail("sdlkfjsdlf"))
alexey-malov commented 8 years ago 
private:
    std::vector<std::string> const CSubscriber::ParseName(std::string line) const;
    std::vector<std::string> const ParseAddress(std::string line) const;
    void ParseEmail(std::string line, std::vector<std::string> &outValues) const;
    void ParseTelephoneNumbers(std::string line, std::vector<std::string> &outValues) const;

явное указание имени класса внутри объявления (CSubscriber::ParseName ) имеет смысл, когда ты хочешь перегрузить метод специфичного базового класса при множественном наследовании. строки передавать по константной ссылке, если не планируется их менять

alexey-malov commented 8 years ago

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

alexey-malov commented 8 years ago 
bool CSubscriber::CompareVectors(const vector<string>&vec, const vector<string>&bdVec) const
{
    if (vec.empty() || bdVec.empty())
    {
        return false;
    }

    int countOfEquals = 0;
    for_each(vec.begin(), vec.end(), [&countOfEquals, &bdVec](string str)
    {
        if (find(bdVec.begin(), bdVec.end(), str) != bdVec.end()){
            countOfEquals++;
        }
    });

    return countOfEquals == vec.size();
}

Название метода не адекватно отражает его поведение. Нельзя использовать Compare. У тебя эта операция не транзитивная: Если Compare(x, y) == true, из этого не следует что Compare(y, x) == true

Метод не использует ни данные класса, ни его нестатические методы. Поэтому его следует сделать статическим.

Параметр лямбды принимать по констатнтной ссылке.

Использовать разность множеств искомых компонентов имени/адреса и содержащихся у абонента. Если в результате получили пустое множество, то найдено. посмотри функции set_intersection, set_union, set_symmetric_difference, set_difference (на cplusplus.com) (есть версии как в std, так и в boost, работающие с диапазонами)

Альтернативный вариант: все из элементов искомой коллекции находятся в элементах коллекции, где осуществляется поиск. Ознакомься с алгоритмами: all_of, any_of, none_of (есть версии как в std, так и в boost, работающие с диапазонами)

Сделать оба варианта (со множествами)

alexey-malov commented 8 years ago 
bool CSubscriber::FindByEmail(string email) const
{
    boost::algorithm::to_lower(email);

    vector<string> temp;
    for each (string email in m_email)
    {
        boost::algorithm::to_lower(email);
        temp.push_back(email);
    }

    return find(temp.begin(), temp.end(), email) != temp.end();
}

for each - нет такого оператора в C++

  1. хранить email-адреса в нижнем регистре и искать сразу через find
  2. не менять хранение и использовать any_of
  3. не менять хранение и использовать find_if
alexey-malov commented 8 years ago

рекомендую посмотреть на примеры. https://github.com/alexey-malov/stl-samples/tree/master/algorithms

alexey-malov commented 8 years ago

поиск по имени и адресу реализовать при помощи операций над множеством искомых и хранимых элементов, либо через алгоритмы: all_of, any_of, none_of

alexey-malov commented 8 years ago

Текущий механизм работы с индексами реализован неэффективно: -Удаление по индексу выполняется за линейное время -Обновление по индексу выполняется за линейное время -Вставка выполняется тоже за линейное время

Алгоритм выбора STL контейнера: http://habrahabr.ru/company/infopulse/blog/194726/

alexey-malov commented 8 years ago

Загрузку и сохранение адресной книги лучше выделить в отдельные функции или класс

alexey-malov commented 8 years ago

Нет слова finded. find->found-found

alexey-malov commented 8 years ago 
    for_each(m_subscribers.begin(), m_subscribers.end(),
        [&findedSubscribers, &telephone](shared_ptr<CSubscriber> subscriber)
    {
        if (subscriber->FindByTelephoneNumber(telephone))
        {
            findedSubscribers.push_back(subscriber);
        }
    });

for_each - ничем не лучше обычного цикла. Здесь лучше подходят: -Алгоритм copy_if -либо boost::range + boost::adaptors::filtered

вот еще примеры https://github.com/alexey-malov/ips-oop2015/blob/master/samples/02/stl-containers/stl-containers.cpp

alexey-malov commented 8 years ago

Код взаимодействия с пользователем следует выделить в класс (или классы), введя сущности наподобие "меню"

alexey-malov commented 8 years ago
alexey-malov commented 8 years ago

Пример того, как можно организовать меню в приложении https://github.com/alexey-malov/ips-oop2015/tree/master/lab3

alexey-malov commented 8 years ago 
class CAddressBook
{
public:
    CAddressBook(const std::string& dataBaseFile);
    CAddressBook();

Конструктор по-умолчанию либо выпилить вообще, либо реализовать по-нормальному. Можно с помощью delegating constructor https://thenewcpp.wordpress.com/2013/07/25/delegating-constructors/

alexey-malov commented 8 years ago

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

Подобным недостатком обладают также XML и JSON, где ошибка в названии тэга и атрибута, а также в названии свойства объекта приводит к необходимости подобных изменений.

Решение: внимательно следить за грамматикой, либо использовать формат, в котором названия полей не используются (используется фиксированный порядок, либо бинарный формат, в котором используются вместо строк числовые значения).

alexey-malov commented 8 years ago 
class CSubscriber
{
public:
    const CSubscriber* FindByName(const std::string& name) const;
    const CSubscriber* FindByAddress(const std::string& address) const;
    const CSubscriber* FindByTelephoneNumber(const std::string& telephoneNumber) const;
    const CSubscriber* FindByEmail(const std::string& email) const;
};

Неудачный интерфейс для взаимодействия с абонентом. Складывается ощущение, что подписчик где-то ищет подписчика, а на самом деле возвращает либо nullptr, либо this. Чем не угодил bool.

alexey-malov commented 8 years ago
alexey-malov commented 8 years ago

Временные файлы, создаваемые во время работы тестов должны удаляться (например, в деструкторе фикстуры)

alexey-malov commented 8 years ago 
        boost::filesystem::path temp = boost::filesystem::temp_directory_path();
        temp.append(boost::filesystem::unique_path().native());

Код не компилируется. Используй /= для конкатенации фрагмена пути (native() можно не вызывать)

alexey-malov commented 8 years ago 
    std::string NewSubscriber(
        const std::string& name,
        const std::string& surname,
        const std::string& patronymic,
        const std::string& email,
        const std::string& telephonNamber,
        const std::string& street,
        const std::string& house,
        const std::string& apartment,
        const std::string& city);

Возврат строки вызывает вопросы. При дальнейшем анализе выясняется, что возвращается сообщение об ошибке или пустая строка в случае успеха. Такой подход не очень хороший. Теперь ты знаешь про исключения.

alexey-malov commented 8 years ago 
    bool ModifySubscriber(std::shared_ptr<CSubscriber>& subscriber,
                        const std::string& name,
                        const std::string& surname,
                        const std::string& patronymic,
                        const std::string& email,
                        const std::string& telephonNamber,
                        const std::string& street,
                        const std::string& house,
                        const std::string& apartment,
                        const std::string& city);

-передача shared_ptr по ссылке здесь читается как "функция будет/может изменять значение указателя". Лучше передавать указатель по константной ссылке, а еще лучше, просто ссылку на CSubscriber. Во-первых, быстрее. Во-вторых, цель - модифицировать абонента, не важно что на него ссылается.

alexey-malov commented 8 years ago 
void CAddressBook::UpdateSubscriber(const int index,
    const std::string& name,
    const std::string& surname,
    const std::string& patronymic,
    const std::string& email,
    const std::string& telephonNamber,
    const std::string& street,
    const std::string& house,
    const std::string& apartment,
    const std::string& city)
{
    auto& subscriber = m_subscribers[index];

обращение к элементу массива по его индексу происходит без каких-либо проверок. Лучше at использовать, он бросит исключение при выходе за пределы

alexey-malov commented 8 years ago 
string CAddressBook::NewSubscriber(
    const std::string& name,
    const std::string& surname,
    const std::string& patronymic,
    const std::string& email,
    const std::string& telephonNamber,
    const std::string& street,
    const std::string& house,
    const std::string& apartment,
    const std::string& city)
{
    int newIndex = m_subscribers.size();
    bool setIndex = false;
    for (size_t i = 0; i < m_subscribers.size(); i++)
    {
        if (m_subscribers[i]->HasEmail(email))
        {
            return "Такой email уже есть";
        }
    }

    auto subscriber = make_shared<CSubscriber>();
    if (ModifySubscriber(
        subscriber,
        name,
        surname,
        patronymic,
        email,
        telephonNamber,
        street,
        house,
        apartment,
        city))
    {
        m_subscribers.push_back(subscriber);
    }

    return "";
}

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

alexey-malov commented 8 years ago

telephonNamber переименовать в phoneNumber

alexey-malov commented 8 years ago

Вместо длинной цепочки однотипных действий

    if (!name.empty())
    {
        subscriber->SetName(name);
        isModify = true;
    }

можно выделить эту операцию в функцию (или лямбду, что проще):

    // Указатель на метод класса CSubscriber, которая принимаеть const string& и возвращает void
    typedef void (CSubscriber::*StringPropertySetter)(const string& value);

    auto UpdateSubscriberProperty = [&](StringPropertySetter setter, const string& value){
        if (!value.empty())
        {
            // Вызываем у объекта subscriber метод, адрес которого хранится в указателе setter
            // subscriber нужен, чтобы сеттеру неявно передать this
            ((*subscriber).*setter)(value);
            isModify = true;
        }
    };

    UpdateSubscriberProperty(&CSubscriber::SetName, name);
    // Для других сеттеров можно сделать аналогично предыдущей строке

В C++14 (например VS2015), можно использовать полиморфные лямбды для работы с произвольными сеттерами, вместо того, чтобы для каждого типа сеттера городить свою лямбду:

#include <float.h>
#include <math.h>
#include <string>

using namespace std;

struct Object
{
    void SetAge(int)
    {

    }
    void SetWeight(double)
    {

    }
    void SetName(const string&)
    {

    }
};

// Семейство функций, которые будут использоваться для проверки необходимости установки значения свойства
// (в реальности удобнее было бы использовать boost::optional)
bool IsDefined(int x)
{
    return x != 0;
}
bool IsDefined(double x)
{
    return !isnan(x);
}
bool IsDefined(const string& s)
{
    return !s.empty();
}

int main()
{
    bool hasModified = false;

    Object obj;

    // Полиморфная лямбда-функция, которая может работать с сеттерами и значениями произвольного типа
    auto UpdateProperty = [&](auto setter, const auto & value)
    {
        if (IsDefined(value))
        {
            (obj.*setter)(value);
            hasModified = true;
        }
    };

    UpdateProperty(&Object::SetAge, 10);
    UpdateProperty(&Object::SetWeight, 15.3);
    UpdateProperty(&Object::SetName, "Ivan susanin");

    return 0;
}

C optional-параметрами все становится намного проще и более явным (для значений, которые не должны изменять свойства объекта, используется boost::none):

void TestWithOptional()
{
    using boost::optional;
    optional<string> optionalName = "Ivan";
    optional<int> optionalAge = boost::none; // не указан
    optional<double> optionalWeight = 30.4;

    bool hasModified = false;

    Object obj;

    auto UpdateProperty = [&](auto setter, const auto & optValue)
    {
        if (optValue.is_initialized())
        {
            (obj.*setter)(*optValue);
            hasModified = true;
        }
    };

    UpdateProperty(&Object::SetAge, optionalAge); // не изменит hasModified
    UpdateProperty(&Object::SetWeight, optionalWeight);
    UpdateProperty(&Object::SetName, optionalName);
}
alexey-malov commented 8 years ago 
void CAddressBook::ParseBaseData(string line, vector<string> &outValues)
{
    boost::regex expression("(?:\\s*(\\w+)\\s*\\[([^\\]]*)\\]\\s*)");
    boost::regex_split(back_inserter(outValues), line, expression);
}

почему бы не вернуть вектор вместо передачи его по ссылке? переименовать в ParseDatabase

alexey-malov commented 8 years ago 
        if (values[i] == NAME)
        {
            subscriber->SetName(values[++i]);
        }

        if (values[++i] == SURNAME)
        {
            subscriber->SetSurname(values[++i]);
        }

        if (values[++i] == PATRONYMIC)
        {
            subscriber->SetPatronymic(values[++i]);
        }

        if (values[++i] == EMAIL)
        {
            subscriber->SetEmail(values[++i]);
        }

        if (values[++i] == TELEPHONE_NUMBER)
        {
            subscriber->SetTelephoneNumber(values[++i]);
        }

        if (values[++i] == STREET)
        {
            subscriber->SetStreet(values[++i]);
        }

        if (values[++i] == HOUSE)
        {
            subscriber->SetHouse(values[++i]);
        }

        if (values[++i] == APARTMENT)
        {
            subscriber->SetApartment(values[++i]);
        }

        if (values[++i] == CITY)
        {
            subscriber->SetCity(values[++i]);
        }

используется оператор [] без проверки на допустимость индексов. В тэгах внутри базы данных практического смысла мало, т.к. парсер жестко завязан на порядок следования элементов. Более того, если тэг не распознается, то следом будет сравниваться значение тэга, а не его имя. Поскольку данные в файле хранятся построчно, то и парсить тоже желательно построчно. Любая нехватка или излишек в данных приведут к тому, что следующие несколько строк не будут распарсены из-за смещения курсора и логики программы Тоже однотипный код можно вынести в лямбду

alexey-malov commented 8 years ago 
        str.append(SURNAME + "[" + subscriber->GetSurname() + "],");
        str.append(PATRONYMIC + "[" + subscriber->GetPatronymic() + "],");
        str.append(EMAIL + "[" + subscriber->GetEmail() + "],");
        str.append(TELEPHONE_NUMBER + "[" + subscriber->GetTelephoneNumber() + "],");
        str.append(STREET + "[" + subscriber->GetStreet() + "],");
        str.append(HOUSE + "[" + subscriber->GetHouse() + "],");
        str.append(APARTMENT + "[" + subscriber->GetApartment() + "],");
        str.append(CITY + "[" + subscriber->GetCity() + "];");

Тэгирование лучше выделить в отдельную функцию:

AppendProperty(str, CITY, subscriber->GetCity());
alexey-malov commented 8 years ago 
    auto it = copy_if(m_subscribers.begin(), m_subscribers.end(), back_inserter(foundSubscribers),
        [&name](shared_ptr<CSubscriber> subscriber)
    {
        return subscriber->HasName(name);
    });

принимать shared_ptr по константной ссылке. в других местах аналогично

alexey-malov commented 8 years ago
alexey-malov commented 8 years ago 
void PrintSubscribers(const subscribers values){}
...
void FindByName(const shared_ptr<CAddressBook>addressBook){}
...
void FindByAddress(const shared_ptr<CAddressBook>addressBook){}
...
void FindByEmail(const shared_ptr<CAddressBook>addressBook){}
void FindByTelephoneNumber(const shared_ptr<CAddressBook>addressBook){}
void FindByAllParams(const shared_ptr<CAddressBook>addressBook){}
void Find(const shared_ptr<CAddressBook>addressBook, const string command){}
void FindSubscriber(const shared_ptr<CAddressBook>addressBook){}
bool AddNewSubscriber(const shared_ptr<CAddressBook>addressBook, string& error){}
void EditSubScriber(const shared_ptr<CAddressBook>addressBook, int index){}
void DeleteSubscriber(const shared_ptr<CAddressBook>addressBook, const int index){}
void UpdateSubscriber(const shared_ptr<CAddressBook>addressBook){}

по константной ссылке инты передавать по константному значению смысла не имеет, разве что для успоения души, что функция даже копию значения не может изменить

alexey-malov commented 8 years ago 
struct AddressBook
{
    AddressBook()
    {
        path m_tempFile = temp_directory_path();
        m_tempFile /= unique_path();

        copy_file(DATA_BASE, m_tempFile);

        ab.LoadSubscribersFromDataBaseFile(m_tempFile.string());
    };

    ~AddressBook()
    {
        remove(m_tempFile);
    };

    path m_tempFile;
    CAddressBook ab;
};

удаление временного файла не происходит, потому как переменная-член класса m_tempFile в конструкторе замещена одноименной локальной переменной

alexey-malov commented 8 years ago 
            auto SetSubscriberPropertyForKey = [&](string key, StringPropertySetter setter){

                if (values.at(i) == key)
                {
                    ((*subscriber).*setter)(values.at(++i));
                }
            };

key принимать в лямбде по константной ссылке

alexey-malov commented 8 years ago

При отстствии базы данныхз надо сообщить об ее отсутствии, либо создать пустую. Сейчас assert выстреливает

alexey-malov commented 8 years ago 
        for (size_t i = 0; i < values.size(); i++)
        {
            typedef void (CSubscriber::*StringPropertySetter)(const string& value);

            auto SetSubscriberPropertyForKey = [&](string key, StringPropertySetter setter){

                if (values.at(i) == key)
                {
                    ((*subscriber).*setter)(values.at(++i));
                }
            };

            SetSubscriberPropertyForKey(NAME, &CSubscriber::SetName);
            SetSubscriberPropertyForKey(SURNAME, &CSubscriber::SetSurname);
            SetSubscriberPropertyForKey(PATRONYMIC, &CSubscriber::SetPatronymic);
            SetSubscriberPropertyForKey(EMAIL, &CSubscriber::SetEmail);
            SetSubscriberPropertyForKey(TELEPHONE_NUMBER, &CSubscriber::SetTelephoneNumber);
            SetSubscriberPropertyForKey(STREET, &CSubscriber::SetStreet);
            SetSubscriberPropertyForKey(HOUSE, &CSubscriber::SetHouse);
            SetSubscriberPropertyForKey(APARTMENT, &CSubscriber::SetApartment);
            SetSubscriberPropertyForKey(CITY, &CSubscriber::SetCity);
        }

при чтении базы все данные смещаются (значения параметров начинают интерпретироваться как имена ключей) Например, здесь будет считан только один абонент: "фамилие де бержерак email телефон" database.txt

alexey-malov commented 8 years ago 
    void CAddressBook::AppendProperty(
        std::string& str,
        const std::string& property,  
        const std::string& value);

метод не использует переменных класса CAddressBook, поэтому сделать метод статическим. Аналогичное замечание и насчет ParseDataBase

alexey-malov commented 8 years ago 
bool CAddressBook::Updated()
{
    return m_updateBD;
}

метод не изменяет состояние объекта, и должен быть константным

alexey-malov commented 8 years ago 
bool CAddressBook::ModifySubscriber(const shared_ptr<CSubscriber>& subscriber,
                                const std::string& name,
                                const std::string& surname,
                                const std::string& patronymic,
                                const std::string& email,
                                const std::string& phoneNumber,
                                const std::string& street,
                                const std::string& house,
                                const std::string& apartment,
                                const std::string& city)
{
    typedef void (CSubscriber::*StringPropertySetter)(const string& value);

    auto UpdateSubscriberProperty = [&](StringPropertySetter setter, const string& value){
        if (!value.empty())
        {
            ((*subscriber).*setter)(value);
            m_updateBD = true;
        }
    };

    UpdateSubscriberProperty(&CSubscriber::SetName, name);
    UpdateSubscriberProperty(&CSubscriber::SetSurname, surname);
    UpdateSubscriberProperty(&CSubscriber::SetPatronymic, patronymic);

    string lowercaseEmail(email);
    boost::algorithm::to_lower(lowercaseEmail);
    UpdateSubscriberProperty(&CSubscriber::SetEmail, lowercaseEmail);
    UpdateSubscriberProperty(&CSubscriber::SetTelephoneNumber, phoneNumber);
    UpdateSubscriberProperty(&CSubscriber::SetStreet, street);
    UpdateSubscriberProperty(&CSubscriber::SetHouse, house);
    UpdateSubscriberProperty(&CSubscriber::SetApartment, apartment);
    UpdateSubscriberProperty(&CSubscriber::SetCity, city);

    return m_updateBD;
}

Если метод однажды вернет true, то все последующие вызовы будут возвращать true, в том числе и когда никаких модификаций произведено не было, что повлияет на работу метода AddNewSubscriber. Написать тест, проверяющий, что добавление пустого абонента после непустого, не теряет информацию об ошибке

alexey-malov commented 8 years ago 
bool CSubscriber::EqualVectors(const vector<string>&vec, const vector<string>&bdVec) const
{
    if (vec.empty() || bdVec.empty())
    {
        return false;
    }

    bool vectorsEqual = false;
    vectorsEqual = all_of(vec.begin(), vec.end(), [&vectorsEqual, &bdVec](const string& strOfVector1)
    {
        return any_of(bdVec.begin(), bdVec.end(), bind2nd(equal_to<string>(), strOfVector1));
    });

    return vectorsEqual;
}

метод не использует переменные-члены класса, поэтому его следует сделать статическим, чтобы не писать .begin(), .end() можно воспользоваться версиями этих алгоритмов из boost.range: all_of(vec,[]{}) аналогично в других функциях vectorsEqual не используется внутри лямбды, поэтому его можно не захватывать

чтобы каждый раз не писать boost::regex, можно в начале файла написать: using boost::regex;

alexey-malov commented 8 years ago

Вот так можно сделать меню:

#include <vector>
#include <string>
#include <functional>
#include <iostream>
#include <sstream>
#include <boost/range/algorithm/find_if.hpp>

using namespace std;
using namespace std::placeholders;
using namespace boost::range;

struct MenuItem
{
    string text;
    string shortcut;
    function<bool(istream & args, istream & input, ostream & output)> action;
};

class CMenu
{
public:
    template <typename MenuItems>
    CMenu(MenuItems && items)
        :m_items(begin(items), end(items))
    {
    }

    void InteractWithUser(istream & input, ostream & output)
    {
        /*Print menu items to output*/
        string line;
        string shortcut;
        while (getline(input, line))
        {
            /*Print menu items to output*/

            istringstream cmdStream(line);
            if (cmdStream >> shortcut)
            {
                auto it = find_if(m_items, [&](const MenuItem& item){
                    return item.shortcut == shortcut;
                });
                if (it != m_items.end())
                {
                    if (!it->action(cmdStream, input, output))
                    {
                        break;
                    }
                }
                else
                {
                    output << "Command " << shortcut << " is not recognized" << endl;
                }
            }
        }
    }
private:
    vector<MenuItem> m_items;
};

bool SayHello(istream & args, istream & /*input*/, ostream & output)
{
    string name;
    getline(args, name);
    output << "Hello, " << name << "!" << endl;
    return true;
}

bool SayGoodbye(istream & args, istream & /*input*/, ostream & output)
{
    string name;
    getline(args, name);
    output << "Goodbye, " << name << "!" << endl;
    return true;
}

bool Greet(const string& prefix, istream & args, istream & /*input*/, ostream & output)
{
    string name;
    getline(args, name);
    output << prefix << name << "!" << endl;
    return true;
}

bool Quit(istream & /*args*/, istream & /*input*/, ostream & /*output*/)
{
    return false;
}

int main()
{
    const MenuItem items[] = {
        //{ "hello <name>", "hello", SayHello},
        //{ "goodbye <name>", "goodbye", SayGoodbye},
        { "hello <name>", "hello", bind(Greet, "Hello, ", _1, _2, _3) },
        { "goodbye <name>", "goodbye", bind(Greet, "Goodbye, ", _1, _2, _3) },
        { "quit", "quit", Quit },
    };
    CMenu menu(items);
    menu.InteractWithUser(cin, cout);
    return 0;
}
alexey-malov commented 8 years ago 
bool Updated() const;

лучше назвать IsModified() (переменную аналогично переименовать)

alexey-malov commented 8 years ago

При использовании адресной книги клиенты могут модифицировать абонентов напрямую, в результате адресная книга не узнает об этих модификациях. Есть разные способы исправить ситуацию: -Добавить IsModified в CSubscriber, но тогда книге потребует О(n) операций для того, чтобы узнать о наличии модификаций -можно возвращать коллекцию shared_ptr, тогда модифицировать будет нельзя -Использовать паттерн наблюдатель, чтобы при модификации абонента адресная книга выставила у себя флажок m_isModified.