- 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.
- 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ć)
- 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));
...
- 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
- zakłada ze będzie mieć 2 liczby, op, 2 liczby, op
- 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();
}