Как можно улучшить код

Question

Имеется код класса для реализации словаря через std::map:

template<class Key, class Value>
class dictionary
{
public:
    virtual ~dictionary() = default;

    virtual const Value& get(const Key& key) const = 0;
    virtual void set(const Key& key, const Value& value) = 0;
    virtual bool is_set(const Key& key) const = 0;
};


template<class Key, class Value>
class Dictionary : public dictionary<Key, Value>
{
    std::map<Key, Value> _key;
public:
    Dictionary() = default;
    Dictionary(const Dictionary&) = delete;
    Dictionary& operator=(const Dictionary&) = delete;
    Dictionary& operator=(Dictionary&&) = delete;
    Dictionary(Dictionary&&) = delete;
    ~Dictionary() override = default;
public:
    const Value& get(const Key& key) const override
    {
        const auto i = _key.find(key);
        if (i == _key.end())
        {
            throw KeyNotFoundException<int>(key);
        }
        return this->_key.at(key);
    }
    void set(const Key& key, const Value& value) override
    {
        const auto i = _key.find(key);
        if (i != _key.end())
        {
            _key.at(key) = value;
        }
        else _key.emplace(key, value);
    }
    bool is_set(const Key& key) const override
    {
        const auto i = _key.find(key);
        return i == _key.end() ? false : true;
    }
};

Как можно улучшить код для get и set?

Answer 1

У вас достаточно странное использование метода std::map.find(). Вы пытаетесь найти элемент, и по полученному итератору уже судить найден ли он, а потом, если он есть, снова искать элемент методом std::map.at().

Можно предложить три улучшения:

Кардинально не меняет код, вместо метода find и проверки итератора на конец можно использовать метод contains, но это если Вы захотите перейти на С++20, но опять же код будет не оптимален.
Это решение предпочтительнее. С помощью метода find вы получаете итератор, который указывает на элемент (если таковой есть), поэтому после проверки if (i == _key.end()) Вы можете в возврате функции обратится к итератору и получить объект, на который он указывает и не искать повторно.
Можно использовать только метод std::map.at().Работа с вариантом, когда нет элемента может быть такая: в этом же методе ловить исключение std::out_of_range, и пробрасывать своё, или же посылать это исключение дальше. Но тут важно помнить, что выбросить и принять исключение это относительно дорогая операция, поэтому 2 вариант и предпочтительнее.

Правда, зачем вы создаёте свой "словарь" на основе std::map, если std::map не только отлично с этим справляется, но и отлично сочетается с остальной частью библиотеки STL для C++.

Answer 2

Вызовы _key.at не нужны, раз у вас под рукой есть итератор.
map::insert_or_assign вставляет ключ или обновляет его значение:
KeyNotFoundException<int> - int выглядит подозрительно.

    const Value& get(const Key& key) const override
    {
        const auto i = _key.find(key);
        if (i == _key.end())
        {
            throw KeyNotFoundException<Key>(key);
        }
        return i->second;
    }
    void set(const Key& key, const Value& value) override
    {
        _key.insert_or_assign(key, value);
    }

БЛОГ НА HUSL

Как можно улучшить код

Ответы (2 шт):