парсер мат строк

[flax]

Добрый день
возник следующий вопрос, в программировании не сильно большой гуру
пошел на собеседование , дали тестовое задание парсер мат выражений, написал все работает , в фидбеке написали что код ужасный
могли бы подсказать где ужастность?

main.cpp

#include <iostream>
#include <string>
#include <conio.h>
#include "parser.h"

using namespace std;



void main()
{
	char str[64];
	Parser pr;

	while(1)
	{
		system("CLS");
		cout<<"Input math expr:\t";
		cin>>str;
		
		try
		{
			cout<<str<< " = "<<pr.calculateExp(str)<<endl;
		}
		catch (char* e)
		{
			cout<<e<<endl;
		}

		cout<<"Would you like to continue? (y/n)\t";
		if (getch()==(int)'Y')
			continue;
		else break;
	}

	cout<<endl;
}

Parser.h

#include <iostream>
using namespace std;

const int iSize = 20;


template <class T> 
class MyStack
{
	
	T m_stackData[iSize];
	int m_iStackSize;
	friend class Parser;

	
	//friend class Parser;

	void push(T x)
	{
		if (m_iStackSize<iSize)
			m_stackData[m_iStackSize++] = x;
	}

	T pop ()
	{
		if (m_iStackSize>0)
			return m_stackData[--m_iStackSize];
		else return -1;
	}

	T top ()const
	{
		if (m_iStackSize>0)
			return m_stackData[m_iStackSize-1];
		else return -1;
	}

public:
	MyStack():m_iStackSize(0){}
};


class Parser
{
	int priorityOperation(char c);
	void make_poland(char * str_expr, char * Output);
	double calculateResult(const char *strPoland);

public:

	double calculateExp( char *strMathExpr);
	
		

};

Parser.cpp

#include "parser.h"


int Parser::priorityOperation(char c)
{      
	switch(c) 
	{
		case '(': return 1;
		case '+': case '-': return 2;
		case '*': case '/': return 3;
		default: return 0;
	}
}

void Parser::make_poland(char * strInputExpr, char * strOutput )
{
	int iOperation = 0;
	MyStack <char> stChar;
	int iCountSkobki = 0;
	char curc;

	if (strlen(strInputExpr)!=0)
	{
		while (isspace(*strInputExpr))
			*strInputExpr++;

		if (!isdigit(*strInputExpr) && *strInputExpr!='(')
			throw (char*)"Incorect input string";
		else
		{
			while(*strInputExpr!='\0')
			{
				if (isdigit(*strInputExpr) || *strInputExpr=='.')
				{
					*strOutput++=*strInputExpr;
					iOperation=0;
				}
				else
				{
					*strOutput++=' ';

					switch(*strInputExpr)
					{
					case '(':
						stChar.push(*strInputExpr);
						iCountSkobki++;
						iOperation = 0;
						break;
					
					case '+': case '-': case '*': case '/':
						{
							if (strlen(strInputExpr) == 0)
								throw (char *)"syntaxes error";

							if (!iOperation)
							{
								iOperation = 1;

								while(priorityOperation(*strInputExpr)<=priorityOperation(stChar.top()))
									*strOutput++ = stChar.pop();

								if (priorityOperation(*strInputExpr)>priorityOperation(stChar.top()))
									stChar.push(*strInputExpr);
							}
						}
						break;

					case ')':
						while ((curc=stChar.pop())!='(' && iCountSkobki>0)
							*strOutput++ = curc;
						iCountSkobki--;

						break;
					}
				}
				*strInputExpr++;
			}
		}
	}
	else throw(char*)"input string is empty";

	while(stChar.top() != -1)
		*strOutput++=stChar.pop();
	
	if(iCountSkobki) 
		throw (char*)"incorrect count brackets"; 

	*strOutput = '\0';

}

double Parser::calculateResult(const char *strPoland)
{
	string str;
	MyStack<double> val_stack;
	char* err;
	double n1, n, res = 0;


	for (size_t i=0;i<strlen(strPoland);i++)
	{
		if(strPoland[i] == ' ')continue;
		
		while(i<strlen(strPoland) && (isdigit(strPoland[i]) || strPoland[i]=='.'))
		{
			str += strPoland[i++];
		}

		if(str.length()!=0) 
		{
			val_stack.push(strtod(str.c_str(), &err));
			str.clear();
			if(i >= strlen(strPoland))
				break;                 //end of the input string

			if(strPoland[i] == ' ') continue;

		}

		n = val_stack.pop();         //the first current number
		n1 = val_stack.pop();        //the second current number

		switch(strPoland[i])
		{
		case '+': res = n + n1; break;
		case '-': res = n1 - n; break;
		case '*': res = n * n1; break;
		case '/': res = n1 / n; break;
		default: throw (char*)"error";
		}
		val_stack.push(res);

	}

	return val_stack.pop();
}

double Parser::calculateExp( char *strMathExpr)
{
	char strPoland[64];
	make_poland(strMathExpr,strPoland);

	return calculateResult(strPoland);
}

[Stilet]

в фидбеке написали что код ужасный
могли бы подсказать где ужастность?

Да расслабься, они просто уже взяли другого кодера, и не знали как тебе об этом сообщить

А вообще лично мне не понравилась бы make_poland(). Но только потому что выглядит сложно - т.е. если ты начнешь проект, потом вдруг уволишься, а твои коды придется кому-то дорабатывать, бедный доработчик может моск сломать в условиях кратчайших сроков, поставленных заказчиком.

[challengerr]

Абстракция и реализация слабая Где-то на форуме я видел крутую реализацию парсера: то ли тут, то ли на киберфоруме. Проблемы кода:
1. размер стека 20. Фиксированный размер это неудачное архитектурное решение. Должен быть построен конечный автомат. А если там буде 5+5+.. + 5 миллион раз с миллионом пар скобок, ваш стек сразу сломается?
2.фиксированная длина строки ввода это неправильно.

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

[Stilet]

А если там буде 5+5+.. + 5 миллион раз с миллионом пар скобок, ваш стек сразу сломается?

Цитирую автора:

дали тестовое задание парсер мат выражений

Соответственно автор написал демо версию.

[challengerr]

Как это делается правильно , код в теме http://www.cyberforum.ru/cpp-beginne...ead671878.html , сообщение от 16.10.2012 21:40

 /* 
 * Grammar
 * 
 * <expr> ::=
 *      <expr> '+' <term>
 * |    <expr> '-' <term>
 * |    <term>
....

[_Bers]

Сообщение от challengerr

Как это делается правильно , код в теме http://www.cyberforum.ru/cpp-beginne...ead671878.html , сообщение от 16.10.2012 21:40

 /* 
 * Grammar
 * 
 * <expr> ::=
 *      <expr> '+' <term>
 * |    <expr> '-' <term>
 * |    <term>
....

Версия ТС мне нравится больше.

[rrrFer]

challengerr
Ты бы на сайбер не ссылался. ИМХО это не только мне глаз режет. Ну можешь попробовать на сайбере ссылку на программерзфорум оставить. Пару раз попробуешь, потом бан получишь (я получил).
Тем более код там тоже не очень приятный.

ТС, лично мне не нравится твой стек. Не понятно зачем это велосипед, когда стек есть в стандартной библиотеке. Тем более твой велосипед снаружи выглядит как велосипед, а внутри у него самокат (стек не должен ломаться при добавлении 21 элемента, поэтому он обычно как динамический список строится, а у тебя внутри массив из 20 элементов).

Если уж ты сделал свой стек, да он у тебя шаблонный, то он должен работать нормально и с любыми типами. Я беру вот этот кусок только:

T top ()const
	{
		if (m_iStackSize>0)
			return m_stackData[m_iStackSize-1];
		else return -1;
	}

Что если тип элементов стека не кастуется в int? - return -1 вызывает конструктор T(int).

Я особо не вчитывался, но вцелом вроде бы сам парсер выглядит симпатично.

[challengerr]

Сообщение от rrrFer

challengerr
Ты бы на сайбер не ссылался. ИМХО это не только мне глаз режет. Ну можешь попробовать на сайбере ссылку на программерзфорум оставить. Пару раз попробуешь, потом бан получишь (я получил).
Тем более код там тоже не очень приятный.

В 2013 я там потерял эккаунт, и после регистрации нового эккаунта получил бан. Ссылки на другие форумы они вырезали.

[rrrFer]

Ссылки на другие форумы они вырезали.

Они вообще внешние ссылки вырезали, даже неактивные ссылки. На мой блог тоже обещали вырезать (хотя не я их там оставил, но ко мне претензии были).
А тут все ссылки активные и без nofollow/noindex. Жирно сильно сайберу, я считаю.