Kalkulator ONP w C++17

0

W ramach nauki nowego standardu C++17 napisałem kalkulator ONP. Interesujące będą dwa pliki(pliki które implementują daną operację nie są zbytnio interesujące):

class Operation {
public:
    virtual double calculate(const double &a, const double &b) = 0;
};

template<typename T>
std::unique_ptr<Operation> createOperationUnique() {
    return std::make_unique<T>();
}

template<typename T>
std::shared_ptr<Operation> createOperationShared() {
    return std::make_shared<T>();
}
int main() {
    std::string line;
    std::cout << "> ";
    std::getline(std::cin, line);
    std::istringstream iss(line);
    std::vector<std::string> vector(std::istream_iterator<std::string>{iss}, std::istream_iterator<std::string>());
    std::deque<std::string> deque(vector.rbegin(), vector.rend());
    std::stack<std::string> stack(deque);
    std::unordered_map<std::string, std::function<std::unique_ptr<Operation>()>> operations;
    operations["+"] = createOperationUnique<Add>;
    operations["-"] = createOperationUnique<Sub>;
    while (stack.size() > 2) {
        auto a = stack.top(); stack.pop();
        auto b = stack.top(); stack.pop();
        auto it = operations.find(stack.top());
        if (it == operations.end()) {
            std::cerr << "Not supported operation\n";
            std::cin.get();
            exit(1);
        }
        double da = std::stod(a, nullptr);
        double db = std::stod(b, nullptr);
        auto op = it->second();
        double result = op->calculate(da, db);
        stack.top() = std::to_string(result);
    }
    std::cout << stack.top() << std::endl;
    std::cin.get();
}

Ja wiem że jest to prosty kod, ale mam kilka pytań:

  1. Czy używanie mapy jest dobrym pomysłem w kontekście eliminacji if-else do wybrania właściwej klasy? (W przypadku tego programu lepszy pomysłem by było po prostu dać gotowy obiekt danej klasy, ale chodzi mi o nakreślenie tego pytania)
  2. Pytanie jak tutaj by można było użyć klasy string_view, żeby pozbyć się string i czy by to miało sens?
  3. Co można by było tutaj poprawić/ulepszyć w kontekście C++17?
  4. Co z std::forward bo często to widzę i nie mogę zbytnio zrozumieć jak z tego korzystać i czy tym programie by miało to jakieś zastosowanie?

Z góry dziękuję za odpowiedź :)

0
Mistrzowski Karp napisał(a):

  std::vector<std::string> vector(std::istream_iterator<std::string>{iss}, std::istream_iterator<std::string>());
  std::deque<std::string> deque(vector.rbegin(), vector.rend());
  std::stack<std::string> stack(deque);

Po co ci trzy kontenery z tym samym?

0

Mapa ma w takich wypadkach kiepską wydajność. Łatwiej zrobić coś takiego (ew. zastąp std::array):

std::function<std::unique_ptr<Operation>()> operations[128];
operations['+'] = createOperationUnique<Add>;
operations['-'] = createOperationUnique<Sub>;
0

@Azarien: do ONP jest porzebny stack, a nie mogłem znaleźć jak z vector uzyskać stack, albo jak w prosty sposób zrobić split na wyrażeniu, żeby w rezultacie mieć stack zamiast vector, dlatego też tak to wygląda

@enedil: no tak tylko co w przypadku gdybym chciał dodać inne operacje takie jak np: sqrt to już stringa jako pozycji w tablicy nie użyję

0

A nie Możesz użyć stosu z STL? http://www.cplusplus.com/reference/stack/stack/

0

@lion137: a skąd ten pomysł że ja nie używam tego?

1
Mistrzowski Karp napisał(a):
  1. Czy używanie mapy jest dobrym pomysłem w kontekście eliminacji if-else do wybrania właściwej klasy? (W przypadku tego programu lepszy pomysłem by było po prostu dać gotowy obiekt danej klasy, ale chodzi mi o nakreślenie tego pytania)

Ciężko powiedzieć. Przy 4 operacjach raczej sensu nie ma.

  1. Pytanie jak tutaj by można było użyć klasy string_view, żeby pozbyć się string i czy by to miało sens?

Nie miało by.
Ty masz tutaj dziwne te linijki

 std::vector<std::string> vector(std::istream_iterator<std::string>{iss}, std::istream_iterator<std::string>());
    std::deque<std::string> deque(vector.rbegin(), vector.rend());
    std::stack<std::string> stack(deque);

W ogole nie rozumiem po co Ty to tak robisz. (Po co Ci jakies dodakowe vectory, deque)
Jak juz chcesz uzyc tego istream_iterator, to zrob to tak.

    for(auto it = std::istream_iterator<std::string>{iss}; it != std::istream_iterator<std::string>(); ++it) {
        // XXX
        stack.emplace(*it);
    }

z tym ze wtedy większość logiki ONP powinieneś mieć tam w XXX (ale to i tak chyba jakoś tak powinno wyglądać)

  1. Co można by było tutaj poprawić/ulepszyć w kontekście C++17?
  • nie widzę sensu w użyciu functions
  • nie widzę sensu w użyciu dziedziczenia
  • nie widzę sensu w użyciu unordered_map

Ten kod trochę wygląda jakbyś chciał w nim użyć jak najwięcej c++11.
Dla mnie przerost formy nad treścią.

No ale może taki był cel tego zadania. Co nie zmienia faktu, że przez próbe wpakowania operacji do common type, musisz tworzyć swoje operacje przez new co jest bez sensu, dotego trzymasz je w kosztownych functions.
To już lepiej jak chcesz użyć czegoś z c++17 to użyj variant i zrób coś jak niżej

struct Add {
    double calculate(const double &a, const double &b) const {
        return a + b;
    };
};

struct Sub {
    double calculate(const double &a, const double &b) const {
        return a - b;
    };
};

std::variant<Add, Sub> create_operation(const std::string& operation) {
    if (operation == "+") {
        return Add{};
    } else if (operation == "-") {
        return Sub{};
    } else {
        throw std::runtime_error("unimplemented operation");
    }
}
...
auto result = std::visit([&](auto &&e) { return e.calculate(a, b); }, op);
stack.push(std::to_string(result));
...
  1. Co z std::forward bo często to widzę i nie mogę zbytnio zrozumieć jak z tego korzystać i czy tym programie by miało to jakieś zastosowanie?

Tutaj nie ma zastosowania.

? no tak tylko co w przypadku gdybym chciał dodać inne operacje takie jak np: sqrt to już stringa jako pozycji w tablicy nie użyję

No niestety twoje rozwiązanie ONP jest i tak błednę bo
1) zakłada ze będzie mieć 2 liczby, op, 2 liczby, op
2) jeśli op jest unary, to wszystko Ci się sypie

:), powodzenia w poprawkach

dodatkow komentarze do kodu który istnieje ale jest błedny

// funkcja do wywalenia,  tylko zaciemnia
template<typename T>
std::unique_ptr<Operation> createOperationUnique() {
    return std::make_unique<T>();
}

// funkcja do wywalenia,  tylko zaciemnia
template<typename T>
std::shared_ptr<Operation> createOperationShared() {
    return std::make_shared<T>();
}

int main() {
    std::string line;
    std::cout << "> ";
    std::getline(std::cin, line);
    std::istringstream iss(line);
    // za duzo kontenerów
    std::vector<std::string> vector(std::istream_iterator<std::string>{iss}, std::istream_iterator<std::string>());
    std::deque<std::string> deque(vector.rbegin(), vector.rend());
    std::stack<std::string> stack(deque);

    // to polaczenie  std::function<std::unique_ptr<Operation>()> nie jest zbyt udane
    std::unordered_map<std::string, std::function<std::unique_ptr<Operation>()>> operations;
    operations["+"] = createOperationUnique<Add>;
    operations["-"] = createOperationUnique<Sub>;

    while (stack.size() > 2) {
        // wydaje mi sie ze to jest zla logika do robienia ONP (ale w sumie nie sprawdzalem tego)
        auto a = stack.top(); stack.pop();
        auto b = stack.top(); stack.pop();
        auto it = operations.find(stack.top());
        if (it == operations.end()) {
            std::cerr << "Not supported operation\n";
            // wywal te cin.get(), so ci do niczego nie potrzebne
            std::cin.get();
            exit(1);
        }
        // po co jest a, b , da, db, czemu od razu nie robic std::stod(stack.top())
        // + nullptr tutaj jest nie potrzebny, to jest default
        // + brak error handlingu, tutaj powinny byc jakies try/catche, a pewnie powyzej while'a
        // + jak chcesz fr z c++17 to mozesz rzucic okiem na std::from_chars
        double da = std::stod(a, nullptr);
        double db = std::stod(b, nullptr);
        auto op = it->second();
        double result = op->calculate(da, db);
        // dziwna jest ta podmiana, czemu nie zrobic pop i push nowa wartosc, bylo by czytelniej
        stack.top() = std::to_string(result);
    }
    std::cout << stack.top() << std::endl;

    // do wywalenia
    std::cin.get();
}

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0