Rechner, Verbesserungsvorschläge

Hallo Leute,

ich habe mich mal an einen Parser für einen recht grundlegenden Rechner gesetzt und wollte eure Meinung bezüglich Verbesserungsvorschläge einholen. Bitte nicht beachten, dass sich in diesem Beitrag alles in einer Datei befindet.

#include <iostream>
#include <vector>
#include <sstream>

using std::cin;
using std::cout;
using std::endl;
using std::istringstream;
using std::string;
using std::getline;

class StackException
{
private:
	const char* msg;

public:
	StackException(const char* _msg)
		: msg(_msg)
	{}

	const char* what()
	{
		return msg;
	}
};

class CalculateException
{
private:
	const char* msg;

public:
	CalculateException(const char* _msg)
		: msg(_msg)
	{}

	const char* what()
	{
		return msg;
	}
};

template <typename T>
class Stack
{
private:
	std::vector<T> stack;

public:
	void push(T value)
	{
		stack.insert(stack.begin(), value);
	}
	T pop()
	{
		if (stack.empty())
			throw StackException("pop on empty stack");

		T value = stack.front();
		stack.erase(stack.begin());
		return value;
	}
};

class Calculator
{
private:
	Stack<int> stack;
	int result;

public:
	Calculator()
		: result(0)
	{
	}

	void insert(int n)
	{
		stack.push(n);
	}

	void divide()
	{
		try
		{
			int right = stack.pop();
			int left = stack.pop();

			if (right == 0)
				throw CalculateException("Division by zero");

			stack.push(left / right);
		}
		catch (StackException& ex)
		{
			throw CalculateException(ex.what());
		}
	}

	void multiply()
	{
		try
		{
			stack.push(stack.pop() * stack.pop());
		}
		catch (StackException& ex)
		{
			throw CalculateException(ex.what());
		}
	}

	void subtract()
	{
		try
		{
			int right = stack.pop();
			int left = stack.pop();
			stack.push(left - right);
		}
		catch (StackException& ex)
		{
			throw CalculateException(ex.what());
		}
	}

	void add()
	{
		try
		{
			stack.push(stack.pop() + stack.pop());
		}
		catch (StackException& ex)
		{
			throw CalculateException(ex.what());
		}
	}

	int getResult()
	{
		try
		{
			return stack.pop();
		}
		catch (StackException& ex)
		{
			throw CalculateException(ex.what());
		}
	}
};

class CalculatorParser
{
private:
	Calculator calculator;
	istringstream stream;

	int result;

	void reset(const string& line)
	{
		calculator = Calculator();

		stream.str(line);
		stream.clear();
	}

public:
	CalculatorParser(const string& line)
	{
		result = 0;
		parse(line);
	}

	int getResult()
	{
		return result;
	}

	int parse(const string& line)
	{
		try
		{
			reset(line);

			int number = 0;
			char operation = 0;
			while (stream >> number)
			{
				calculator.insert(number);

				if (operation != 0)
				{
					switch (operation)
					{
					case '+':
						calculator.add();
						break;
					case '-':
						calculator.subtract();
						break;
					case '*':
						calculator.multiply();
						break;
					case '/':
						calculator.divide();
						break;
					}
					operation = 0;
				}

				stream >> operation;
			}

			return result = calculator.getResult();
		}
		catch (CalculateException& ex)
		{
			cout << ex.what() << endl;
			return result = 0;
		}
	}
};

int main()
{
	string line;
	getline(cin, line);

	CalculatorParser parser(line);
	cout << "Ergebnis:" << parser.getResult() << endl;

	cin.get();
	return 0;
}

Ruvi

Hi,
also ohne mir jetzt den Calculation Teil angesehen zu haben wird mir auf den ersten Blick bei den ganzen exceptions und try catch Blöcken ganz schwindelig.

Ich persönlich kann jetzt auf den ersten Blick nicht erkennen, warum es nötig war einen Stack selbst zu implementieren, warum hast Du nicht ein std::stack genommen?

Mir persönlich gefällt auch nicht, dass du nicht die std::exceptions genommen hast.
Wenn Du eigene Fehlerklassen hast dann würde ich die an deiner Stelle von der jeweiligen std::exception Klasse ableiten. Bei deinem Projekt ist es nicht der Fall aber vielleicht gibt es weiter oberen weitere Exception Catch Blöcke und einige von denen wollen e.g. std::runtime_error catchen und müssen dann extra für deine Exceptions noch einen extra Catch Block einbauen.

Das du teilweise eine Exceptions wirfst und die im Level darüber fängst um dann ein neues anderes Exception Objekt zusammenzubauen ist nach meiner Meinung überflüssig.

Th69

Sieht schon richtig gut aus! Wenn du jetzt noch Klammern (z.B. "5*(3+4)") hinzufügen könntest sowie den Parser generisch (d.h. mit beliebigen Datentypen anstatt nur int, z.B. double), dann hätte ich nichts mehr auszusetzen.

OK, beliebige Funktionen als Terminalsymbole wären auch noch nett

Als Anregung kannst du dir mal meinen Parser-Artikel durchlesen (wenn auch für C#): Parser für mathematische Formeln.

Ruvi

P.S.: Um so länger ich überlege, fände ich es besser wenn dein Parser alle Fehler ausschließen würde und da eine Exceptions geworfen wird falls das eingegebene voraussichtlich nicht bearbeitet werden könnte.

Dann kannst du im Grunde alle Exceptions außer einer im Parser rausschmeißen und alles andere gegen Assertions austauschen.

Aber wie gesagt das ist meine persönliche Meinung, wann immer ich eine Exception sehe frage ich mich "Muss das wirklich sein?".
Ich gehöre da eher zu der "So wenig Exceptions wie möglich (aber soviel wie nötig)" Fraktion.

johan

Ich empfehle für einen Rechner mit Expression Parser da immer das hervorragende Beispiel "Ein Tischrechner" aus Stroustrups "Die C++ Programmiersprache" (mindestens ab 2. Auflage enthalten). Hat sogar Klammern und Variablen dabei (z. B. Mwst=1.19; Brutto=3,98*Mwst - das Semikolon trennt Ausdrücke) und ist trotzdem um einiges kürzer und leichter verständlich, als Deine Version.

Ich habe mir noch den Potenz-Operator (^) und alle mathematischen Funktionen von C++ mit einer Variablen dazu programmiert und habe jetzt einen tollen numerischen Expression Parser

Marthog

Die try- und catch muessen raus. Es ist nicht sinn der Sache, exceptions abzufangen und dann weiterzuleiten. Exceptions sind ja gerade dazu da, um das weiterreichen von Fehlern nicht mehr noetig zu machen, indem sie den Code ueberspringen, bis sich jemand darum kuemmert.