Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

This function is odd:

char checknumber(char ch) 
{
    if ((ch >= '0' && ch <= '9') || ch == '.') return 1; else return 0;
}

To make a function boolean, this postthis post explains some good options.

Even if you don't change the return type, the implementation can be written simpler:

return ((ch >= '0' && ch <= '9') || ch == '.');

I would also reorder the expressions in the range, as the logic is more clear when the values of the terms are organized in increasing order:

return '0' <= ch && ch <= '9' || ch == '.';

I removed the unnecessary parentheses, but if they help you understand the expression you may leave them in.

Undefined behavior

In this function, if none of the operators are matched, then the precedence variable will be returned, even though it was never assigned.

int precedence(char ch)
{
    int precedence;
    switch (ch) 
    {
    case '+':
    case '-':
        precedence = 0;
        break;
    case '*':
    case '/':
        precedence = 1;
        break;
    case '^':
        precedence = 2;
    }
    return precedence;
}

What will be the value then? 0? Most probably it's indeterminateindeterminate. It would be better to explicitly define a default value.

Also this function can be simplified as @SuperBiasedMan showed for the other switch block:

int precedence(char ch)
{
    switch (ch) 
    {
    case '+':
    case '-':
        return 0;
    case '*':
    case '/':
        return 1;
    case '^':
        return 2;
    }
    return INT_MAX;
}

To get INT_MAX, you'd need to add #include <limits.h>.

One statement per line

Avoid multiple statements per line like these:

printf("Enter the Expression: "); scanf("%[^\n]", expression);
// ...

double numbers[5]; int nsi = 0;
char operators[5]; int osi = 0;
char numbuf[16]; int nbi = 0;
char ch; int  i = 0;

Code is easiest to read from top to bottom. I suggest to rewrite this to have one statement per line.

This function is odd:

char checknumber(char ch) 
{
    if ((ch >= '0' && ch <= '9') || ch == '.') return 1; else return 0;
}

To make a function boolean, this post explains some good options.

Even if you don't change the return type, the implementation can be written simpler:

return ((ch >= '0' && ch <= '9') || ch == '.');

I would also reorder the expressions in the range, as the logic is more clear when the values of the terms are organized in increasing order:

return '0' <= ch && ch <= '9' || ch == '.';

I removed the unnecessary parentheses, but if they help you understand the expression you may leave them in.

Undefined behavior

In this function, if none of the operators are matched, then the precedence variable will be returned, even though it was never assigned.

int precedence(char ch)
{
    int precedence;
    switch (ch) 
    {
    case '+':
    case '-':
        precedence = 0;
        break;
    case '*':
    case '/':
        precedence = 1;
        break;
    case '^':
        precedence = 2;
    }
    return precedence;
}

What will be the value then? 0? Most probably it's indeterminate. It would be better to explicitly define a default value.

Also this function can be simplified as @SuperBiasedMan showed for the other switch block:

int precedence(char ch)
{
    switch (ch) 
    {
    case '+':
    case '-':
        return 0;
    case '*':
    case '/':
        return 1;
    case '^':
        return 2;
    }
    return INT_MAX;
}

To get INT_MAX, you'd need to add #include <limits.h>.

One statement per line

Avoid multiple statements per line like these:

printf("Enter the Expression: "); scanf("%[^\n]", expression);
// ...

double numbers[5]; int nsi = 0;
char operators[5]; int osi = 0;
char numbuf[16]; int nbi = 0;
char ch; int  i = 0;

Code is easiest to read from top to bottom. I suggest to rewrite this to have one statement per line.

This function is odd:

char checknumber(char ch) 
{
    if ((ch >= '0' && ch <= '9') || ch == '.') return 1; else return 0;
}

To make a function boolean, this post explains some good options.

Even if you don't change the return type, the implementation can be written simpler:

return ((ch >= '0' && ch <= '9') || ch == '.');

I would also reorder the expressions in the range, as the logic is more clear when the values of the terms are organized in increasing order:

return '0' <= ch && ch <= '9' || ch == '.';

I removed the unnecessary parentheses, but if they help you understand the expression you may leave them in.

Undefined behavior

In this function, if none of the operators are matched, then the precedence variable will be returned, even though it was never assigned.

int precedence(char ch)
{
    int precedence;
    switch (ch) 
    {
    case '+':
    case '-':
        precedence = 0;
        break;
    case '*':
    case '/':
        precedence = 1;
        break;
    case '^':
        precedence = 2;
    }
    return precedence;
}

What will be the value then? 0? Most probably it's indeterminate. It would be better to explicitly define a default value.

Also this function can be simplified as @SuperBiasedMan showed for the other switch block:

int precedence(char ch)
{
    switch (ch) 
    {
    case '+':
    case '-':
        return 0;
    case '*':
    case '/':
        return 1;
    case '^':
        return 2;
    }
    return INT_MAX;
}

To get INT_MAX, you'd need to add #include <limits.h>.

One statement per line

Avoid multiple statements per line like these:

printf("Enter the Expression: "); scanf("%[^\n]", expression);
// ...

double numbers[5]; int nsi = 0;
char operators[5]; int osi = 0;
char numbuf[16]; int nbi = 0;
char ch; int  i = 0;

Code is easiest to read from top to bottom. I suggest to rewrite this to have one statement per line.

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

This function is odd:

char checknumber(char ch) 
{
    if ((ch >= '0' && ch <= '9') || ch == '.') return 1; else return 0;
}

To make a function boolean, this post explains some good options.

Even if you don't change the return type, the implementation can be written simpler:

return ((ch >= '0' && ch <= '9') || ch == '.');

I would also reorder the expressions in the range, as the logic is more clear when the values of the terms are organized in increasing order:

return '0' <= ch && ch <= '9' || ch == '.';

I removed the unnecessary parentheses, but if they help you understand the expression you may leave them in.

Undefined behavior

In this function, if none of the operators are matched, then the precedence variable will be returned, even though it was never assigned.

int precedence(char ch)
{
    int precedence;
    switch (ch) 
    {
    case '+':
    case '-':
        precedence = 0;
        break;
    case '*':
    case '/':
        precedence = 1;
        break;
    case '^':
        precedence = 2;
    }
    return precedence;
}

What will be the value then? 0? Most probably it's indeterminate. It would be better to explicitly define a default value.

Also this function can be simplified as @SuperBiasedMan@SuperBiasedMan showed for the other switch block:

int precedence(char ch)
{
    switch (ch) 
    {
    case '+':
    case '-':
        return 0;
    case '*':
    case '/':
        return 1;
    case '^':
        return 2;
    }
    return INT_MAX;
}

To get INT_MAX, you'd need to add #include <limits.h>.

One statement per line

Avoid multiple statements per line like these:

printf("Enter the Expression: "); scanf("%[^\n]", expression);
// ...

double numbers[5]; int nsi = 0;
char operators[5]; int osi = 0;
char numbuf[16]; int nbi = 0;
char ch; int  i = 0;

Code is easiest to read from top to bottom. I suggest to rewrite this to have one statement per line.

This function is odd:

char checknumber(char ch) 
{
    if ((ch >= '0' && ch <= '9') || ch == '.') return 1; else return 0;
}

To make a function boolean, this post explains some good options.

Even if you don't change the return type, the implementation can be written simpler:

return ((ch >= '0' && ch <= '9') || ch == '.');

I would also reorder the expressions in the range, as the logic is more clear when the values of the terms are organized in increasing order:

return '0' <= ch && ch <= '9' || ch == '.';

I removed the unnecessary parentheses, but if they help you understand the expression you may leave them in.

Undefined behavior

In this function, if none of the operators are matched, then the precedence variable will be returned, even though it was never assigned.

int precedence(char ch)
{
    int precedence;
    switch (ch) 
    {
    case '+':
    case '-':
        precedence = 0;
        break;
    case '*':
    case '/':
        precedence = 1;
        break;
    case '^':
        precedence = 2;
    }
    return precedence;
}

What will be the value then? 0? Most probably it's indeterminate. It would be better to explicitly define a default value.

Also this function can be simplified as @SuperBiasedMan showed for the other switch block:

int precedence(char ch)
{
    switch (ch) 
    {
    case '+':
    case '-':
        return 0;
    case '*':
    case '/':
        return 1;
    case '^':
        return 2;
    }
    return INT_MAX;
}

To get INT_MAX, you'd need to add #include <limits.h>.

One statement per line

Avoid multiple statements per line like these:

printf("Enter the Expression: "); scanf("%[^\n]", expression);
// ...

double numbers[5]; int nsi = 0;
char operators[5]; int osi = 0;
char numbuf[16]; int nbi = 0;
char ch; int  i = 0;

Code is easiest to read from top to bottom. I suggest to rewrite this to have one statement per line.

This function is odd:

char checknumber(char ch) 
{
    if ((ch >= '0' && ch <= '9') || ch == '.') return 1; else return 0;
}

To make a function boolean, this post explains some good options.

Even if you don't change the return type, the implementation can be written simpler:

return ((ch >= '0' && ch <= '9') || ch == '.');

I would also reorder the expressions in the range, as the logic is more clear when the values of the terms are organized in increasing order:

return '0' <= ch && ch <= '9' || ch == '.';

I removed the unnecessary parentheses, but if they help you understand the expression you may leave them in.

Undefined behavior

In this function, if none of the operators are matched, then the precedence variable will be returned, even though it was never assigned.

int precedence(char ch)
{
    int precedence;
    switch (ch) 
    {
    case '+':
    case '-':
        precedence = 0;
        break;
    case '*':
    case '/':
        precedence = 1;
        break;
    case '^':
        precedence = 2;
    }
    return precedence;
}

What will be the value then? 0? Most probably it's indeterminate. It would be better to explicitly define a default value.

Also this function can be simplified as @SuperBiasedMan showed for the other switch block:

int precedence(char ch)
{
    switch (ch) 
    {
    case '+':
    case '-':
        return 0;
    case '*':
    case '/':
        return 1;
    case '^':
        return 2;
    }
    return INT_MAX;
}

To get INT_MAX, you'd need to add #include <limits.h>.

One statement per line

Avoid multiple statements per line like these:

printf("Enter the Expression: "); scanf("%[^\n]", expression);
// ...

double numbers[5]; int nsi = 0;
char operators[5]; int osi = 0;
char numbuf[16]; int nbi = 0;
char ch; int  i = 0;

Code is easiest to read from top to bottom. I suggest to rewrite this to have one statement per line.

added 1267 characters in body
Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

This function is odd:

char checknumber(char ch) 
{
    if ((ch >= '0' && ch <= '9') || ch == '.') return 1; else return 0;
}

To make a function boolean, this post explains some good options.

Even if you don't change the return type, the implementation can be written simpler:

return ((ch >= '0' && ch <= '9') || ch == '.');

I would also reorder the expressions in the range, as the logic is more clear when the values of the terms are organized in increasing order:

return '0' <= ch && ch <= '9' || ch == '.';

I removed the unnecessary parentheses, but if they help you understand the expression you may leave them in.

Undefined behavior

In this function, if none of the operators are matched, then the precedence variable will be returned, even though it was never assigned.

int precedence(char ch)
{
    int precedence;
    switch (ch) 
    {
    case '+':
    case '-':
        precedence = 0;
        break;
    case '*':
    case '/':
        precedence = 1;
        break;
    case '^':
        precedence = 2;
    }
    return precedence;
}

What will be the value then? 0? Most probably it's indeterminate. It would be better to explicitly define a default value.

Also this function can be simplified as @SuperBiasedMan showed for the other switch block:

int precedence(char ch)
{
    switch (ch) 
    {
    case '+':
    case '-':
        return 0;
    case '*':
    case '/':
        return 1;
    case '^':
        return 2;
    }
    return INT_MAX;
}

To get INT_MAX, you'd need to add #include <limits.h>.

One statement per line

Avoid multiple statements per line like these:

printf("Enter the Expression: "); scanf("%[^\n]", expression);
// ...

double numbers[5]; int nsi = 0;
char operators[5]; int osi = 0;
char numbuf[16]; int nbi = 0;
char ch; int  i = 0;

Code is easiest to read from top to bottom. I suggest to rewrite this to have one statement per line.

This function is odd:

char checknumber(char ch) 
{
    if ((ch >= '0' && ch <= '9') || ch == '.') return 1; else return 0;
}

To make a function boolean, this post explains some good options.

Even if you don't change the return type, the implementation can be written simpler:

return ((ch >= '0' && ch <= '9') || ch == '.');

I would also reorder the expressions in the range, as the logic is more clear when the values of the terms are organized in increasing order:

return '0' <= ch && ch <= '9' || ch == '.';

I removed the unnecessary parentheses, but if they help you understand the expression you may leave them in.

This function is odd:

char checknumber(char ch) 
{
    if ((ch >= '0' && ch <= '9') || ch == '.') return 1; else return 0;
}

To make a function boolean, this post explains some good options.

Even if you don't change the return type, the implementation can be written simpler:

return ((ch >= '0' && ch <= '9') || ch == '.');

I would also reorder the expressions in the range, as the logic is more clear when the values of the terms are organized in increasing order:

return '0' <= ch && ch <= '9' || ch == '.';

I removed the unnecessary parentheses, but if they help you understand the expression you may leave them in.

Undefined behavior

In this function, if none of the operators are matched, then the precedence variable will be returned, even though it was never assigned.

int precedence(char ch)
{
    int precedence;
    switch (ch) 
    {
    case '+':
    case '-':
        precedence = 0;
        break;
    case '*':
    case '/':
        precedence = 1;
        break;
    case '^':
        precedence = 2;
    }
    return precedence;
}

What will be the value then? 0? Most probably it's indeterminate. It would be better to explicitly define a default value.

Also this function can be simplified as @SuperBiasedMan showed for the other switch block:

int precedence(char ch)
{
    switch (ch) 
    {
    case '+':
    case '-':
        return 0;
    case '*':
    case '/':
        return 1;
    case '^':
        return 2;
    }
    return INT_MAX;
}

To get INT_MAX, you'd need to add #include <limits.h>.

One statement per line

Avoid multiple statements per line like these:

printf("Enter the Expression: "); scanf("%[^\n]", expression);
// ...

double numbers[5]; int nsi = 0;
char operators[5]; int osi = 0;
char numbuf[16]; int nbi = 0;
char ch; int  i = 0;

Code is easiest to read from top to bottom. I suggest to rewrite this to have one statement per line.

Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396
Loading