# Tag Info

14

In real life, I would use knowledge of existing libraries to pull out an almost-finished solution. In particular, I think it is an example for Boost.Spirit, which is what I would use for a real problem of this nature. I applaud you for checking here as well as whatever else you are doing to learn from your experience and improve your skills. Library ...

13

Loki Astari already covered a number of good points, which I will not repeat. Algorithms and data structures The standard library contains a number of ready-made algorithms and data structures that can make your code easier to read and understand. For example, you have these lines: opp_count = 0; // line 393, new lines removed for brevity for (i = 0; i &...

13

Naming and Style Based on the naming guidlines method names should be using PascalCase casing. you should not prefix the variablenames with the datatype -> listOfParsedExpressions variable names should be meaningful so you or Mr.Maintainer will understand them in 6 months also -> const int TKNLGHT = 4; you should use braces {} for single if ...

12

Things you did well on: Organization Use of comments Using switch statements instead of multiple if-elses. Things that could be improved: Compilation I couldn't compile this program with this method written the way it is. int len(char * p) { return int(strlen(p)); } I'm going to assume that is due to the issues of you compiling it with g++, and me ...

12

Common beginner mistakes Stop doing this: using namespace std; See Why is “using namespace std;” considered bad practice? Namespaces All your functions seem to have the prefix rdo_ bool rdo_ws(char c) char rdo_expr_item_type(char c) string rdo_opp_to_string(char opp) bool rdo_is_num(string is_num) void rdo_count_opp(bool to_count_or_not) string ...

11

If I were going to carry out this general task, I'd probably use a recursive descent parser. A simple version to handle +, -, * and / with the correct precedence can look something like this: #include <iostream> #include <string> #include <cctype> int expression(); char token() { char ch; std::cin >> ch; return ch; } ...

11

How do you even use this code? You read through 5 different methods, each doing something specific -- How do you determine which one is used? The RemoveAll extension is expensive: you're creating a new string for each removal of a character. I would suggest a different approach where you unwrap the string into an array of characters, take all the characters ...

11

Your code has a pretty massive flaw - it only accepts single digit numbers. Trying to use new CalculatorExpression("10+1") falls over (specifically your assertion fails). A calculator that can only handle single digit numbers isn't really that useful. I'd recommend you changing the signature of your ToPostFix method to return an IEnumerable<string> ...

10

There's a lot that can be reviewed here, but I'll mention some best-practices at a glance: Keep your indentation and whitespace consistent! You seem to be primarily using four spaces for indentation, but you don't do this everywhere. What's even odd is that the body of evaluate() is not indented (unlike the other functions). One problem with this, ...

10

Class Program The removeSpaces() method is violating the single responsibility principle as it remove spaces from the given input checks for the word exit checks if the given expression contains a valid equotation So these 3 responsibilities should be extracted to 3 methods. Let us start with removing the spaces. We just use the Replace() ...

10

tokenize The tokenize subroutine could be simplified: sub tokenize { my ($code) = @_; die "Invalid Arguments" unless defined$code; return $code =~ m!\d+|[-+*/.]!g; } Changes include: Shorter parameter name One-line validation Use global match in list context to produce a list of all matches Simpler regex that avoids leaning toothpick ... 10 Just one small point that @200_success probably left out so I can say something too: when matching something against$_, for example in if ($_ =~ /[0-9]+/) { ... }, you can simply omit the$_: if (/[0-9]+/) { # ... } It's "the default input and pattern-searching space". Read more about the \$_ variable in man perlvar or on perldoc.perl.org. The main ...

10

Use singular names for enums enum Operators { Addition, Subtraction, Multiplication, Division, Modulo } Plural name for enums is a bit unnatural in practice. Look at these uses: private static Operators getNextOperator() { ... } Operators op = getNextOperator(); It looks as if the type is some kind of collection, like a set or list or map. The ...

10

I see a number of things that may help you improve your code. Add error checking Data input usually requires a lot of careful consideration for most software. Checking for malformed input and handling it gracefully often takes a considerable amount of thought, time and code. In this code we have a 100 character input buffer, but nothing prevents a user ...

10

Don’t write using namespace std;. You can, however, in a CPP file (not H file), or inside a function, put individual using std::string; etc. (See SF.7.) Did you know that there is a stack container adaptor in the standard library? And basing it on a linked list is slow — use a vector. I'll note some things about the Stack code for general teaching, since ...

9

Design #define VAL 1 #define SUB '-' #define ADD '+' etc. are too generically named. Suggest a naming convention that does not so readily conflict with other code. Same for function names push(), resize(), pop(), peek(). In general, the .h files add name all over the name-space. Suggest a corner of it instead. I would expect code guards in the .h files ...

9

Some quick observations: Don't use using namespace std Prefer using \n over std::endl Prefer prefix over postfix You're mixing C and C++ here. Use <cstring>/<string> (depending on what you need) instead of string.h. For input have a look at the C++ I/O library Some of your code is windows only ( system("pause") and system("cls") ) Don't define ...

8

I agree with that a different approach would be better, so just a few random notes, mostly about the current code: It's good to know that it's already in the JDK, in form a script engine. So, if you don't want to reinvent the wheel just use it: import javax.script.ScriptEngine; import javax.script.ScriptEngineManager; final ScriptEngineManager ...

8

Reverse Polish Notation does not need parentheses, so that should actually be invalid input and should not be checked. To determine if something is an operand you should be able to use stdin.hasNextInt(). If that is false, then you should be able to use stdin.next() to get whatever the operator is. This could greatly simplify your type checking. Instead of ...

8

Your solution is very limited. As it only take a string in in the following format x1 x2 x3, and apply to the function x1+x2*x3. To build an expressional calculator, you need to go through these steps : Segmentate the expression into a list of tokens Parse the tokens into recognizable lexemes(numbers, operator, parenthesis, etc) Transform the list into a ...

8

I've a number of concerns about this code, but for a first attempt it's good enough if it works with the test inputs you've been given. Let's run through the code and see what comes up - good and bad. I'm going to be quite rough with you - but you'll be a better coder for it. I have included (incomplete) code examples, but not enough that you could copy/...

8

@tim has covered a bunch of what I was going to add, but here are some additional points: You should anticipate having whitespace in the input. A "simple" replace-all would suffice: equation = equation.replaceAll("\\s+", ""); Your solution of converting - to +- is novel, and effective, but it really should have a comment on it explaining that you are ...

8

Following PEP-8 High level descriptions of functions should be docstrings The name should be snake_case def is_operator(string): """ Returns True if string is an operator otherwise returns False.""" if string == '+' or string == '-' or string == '*' or string == '/': return True else: return False Using a constant to simplify ...

8

Generalities and trivialities: I recommend #includeing all wanted system headers before any private headers. This avoids any possibility of changing the meaning of any system header by defining a macro that happens to be meaningful to it. static functions and variables generally should not be declared in header files, unless you actually want every file ...

8

isInteger is not needed. Use std::isdigit. Trust the logic. An else if condition is mutually exclusive with if condition. There is no need to test for it - you already know it is true. A simple else is enough. But see also the next point. Notice that all code paths in the else clause necessarily push(c). Factor it out: if ((stack.size() > 0) { if ((...

8

Spacing I'd like to see some more blank lines. They help separate code into blocks. Without a blank line here and there, the code looks like a wall of text. As someone who answers questions on Stack Overflow, I can tell you a wall of text is hard to read; I usually just skip such questions. Of course, someone reading your source code may be forced not to ...

8

On top of the other very good answers: Code organisation and tests It is a good idea to separate the logic about input/output from the logic computing results. In your case, you could have a function taking an expression as a string and returning a number. This makes your code easier to understand, easier to reuse and more testable. Also, it is a good ...

7

What you did well Taking advantage of the shell to split your expression into tokens is smart. It saves you from the trouble of having to write a tokenizer. Beware, though, of a usability issue with Unix shells, where * and \ have special significance and need to be quoted or escaped. Bugs Compilation error: This was just carelessness. getop.c:31:6: ...

7

Personally I would use lex(flex) and yacc(bison) to do the same thing. Edit: After just doing it. I change my mind. If all I was doing was an expression parser I would write a recursive descent parser like "Jerry Coffin" did. But if there was any chance that it would grow beyond just a simple expression parser (like having variables and state or any more ...

Only top voted, non community-wiki answers of a minimum length are eligible