patterncsharpMinor
Parser for an arithmetic expression
Viewed 0 times
parserexpressionforarithmetic
Problem
I am a student, and I am doing websites as freelancer. Lately I have started learning design patterns, refactoring, and test-driven development so that I can improve the maintainability, performance, and readability of my code.
I did some refactoring with method extracting in this case, but I got feedback that my code is trash.
Here is the main method. The whole
```
/* main functionality
*
* parameter: expression - accepts string parameter and parses him, so it can calculate
*/
static double ParseCalculationString(string expression)
{
// currentNumberString will save current number
string currentNumberString = "";
// temporaryNumberString will be used for multiply/divide operations
string temporaryNumberString = "";
// we use this to access previous operator for calculating between numbers
char previousOperator = ' ';
// we use this for current operator that we get from from expression[i]
char currentOperator = ' ';
/* this is used for sum and subtract operations, we save operator and number here
so we can finish multiply and divide operations first, then we use number on hold
and operator on hold to add or deduct to total result. */
string numberOnHold = "";
char operatorOnHold = ' ';
/* total is used for saving total result, and currentNumberDouble for saving the number
in the middle of multiply/divide operations /
double total = 0.0;
double currentNumberDouble = 0.0;
/ We use this two flags for marking the operation type, so we know what to calculate first /
bool flagMultiplyDivide = false;
bool flagSumSubtract = false;
int i;
for (i = 0; i < expression.Length; i++)
{
// checking if character is number, if yes we build number from the string until the character is symbol
if (char.IsNumber(expression[i]))
{
currentNumberString = currentNumberString + expression[i];
}
/
I did some refactoring with method extracting in this case, but I got feedback that my code is trash.
Here is the main method. The whole
Program class is on GitHub.```
/* main functionality
*
* parameter: expression - accepts string parameter and parses him, so it can calculate
*/
static double ParseCalculationString(string expression)
{
// currentNumberString will save current number
string currentNumberString = "";
// temporaryNumberString will be used for multiply/divide operations
string temporaryNumberString = "";
// we use this to access previous operator for calculating between numbers
char previousOperator = ' ';
// we use this for current operator that we get from from expression[i]
char currentOperator = ' ';
/* this is used for sum and subtract operations, we save operator and number here
so we can finish multiply and divide operations first, then we use number on hold
and operator on hold to add or deduct to total result. */
string numberOnHold = "";
char operatorOnHold = ' ';
/* total is used for saving total result, and currentNumberDouble for saving the number
in the middle of multiply/divide operations /
double total = 0.0;
double currentNumberDouble = 0.0;
/ We use this two flags for marking the operation type, so we know what to calculate first /
bool flagMultiplyDivide = false;
bool flagSumSubtract = false;
int i;
for (i = 0; i < expression.Length; i++)
{
// checking if character is number, if yes we build number from the string until the character is symbol
if (char.IsNumber(expression[i]))
{
currentNumberString = currentNumberString + expression[i];
}
/
Solution
In general comments should be used to say why you are doing something, not what you are doing. You should strive to have variable names that describe what they contain.
Don't be scared to create sub-functions. If you feel like you need to explain what a block of code is doing, there is a chance that block can be turned into a separate function. Now you can give the new function a descriptive name that will help explain what is being done. This will also help people from getting lost in the number of nested if blocks.
Define constants for your operators, possible make them an enum since they form a logical grouping. It will also make it easier to find places where you use the constants. Otherwise you have to do a text search for
Don't be scared to create sub-functions. If you feel like you need to explain what a block of code is doing, there is a chance that block can be turned into a separate function. Now you can give the new function a descriptive name that will help explain what is being done. This will also help people from getting lost in the number of nested if blocks.
Define constants for your operators, possible make them an enum since they form a logical grouping. It will also make it easier to find places where you use the constants. Otherwise you have to do a text search for
+ and will end up finding places where that is used in the code and not as a parsing operator. Another benefit of defining constants is to avoid magic numbers. This is not something you are doing here, but is related to the same idea. It is a good practice to get into for values that contain a special meaning.Context
StackExchange Code Review Q#29855, answer score: 6
Revisions (0)
No revisions yet.