HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

feedback for exercise 4-11 in K&R

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
feedbackexercisefor

Problem

I know that there is one more topic about the exercise 4-11, but the difference is that I solved this exercise and i just need some feedback on my solution. So, I'll explain how it works on an output like "3 4 +\n", for example. The static variable c is initialized with the value ' ' so the condition of the first if statement evaluates to true, the while loop within the if statement will run until c reaches the value '3'. Because it is a number, the function will return the NUMBER signal and the variable c will get the value ' ' - the value that breaks the loop within the 3rd if statement.

Now we are, again, on the first if statement and, again, c has the value ' '. The condition is evaluated to true and the loop within the if statement will run until c reaches the value '4'.
Again, because '4' is a "number" the function will return the NUMBER signal and c will get the value '+' - the value that breaks the loop within the 3rd if statement.

This time c is equal to '+' and the condition of the first if statement is evaluated to false, so the value '+' will be returned. I saved the value of the variable c in a temporary variable tmp, and initialize tmp with the value of c before changing it to ' '.

Without this movement I'll get an infinite loop.

The program is made of 5 parts:

main.c

```
#include
#include
#include "calc.h"
#define MAXOP 1001

int main() {
char entry;
char s[MAXOP];
double op2;

while((entry = getop(s)) != EOF) {
switch(entry) {
case NUMBER:
push(atof(s));
break;
case '+':
push(pop() +pop());
break;
case '*':
push(pop() * pop());
break;
case '-':
op2 = pop();
push(pop() - op2);
break;
case '/':
op2 = pop();
if(op2) {
push(pop() / op2);
}

Solution

Comments on getop() only:

Your initial loop repeats the exit condition twice. It (and subsequent code)
also contains double assignments, which are unnecessary and generally better
avoided:

if((s[0] = c) == ' ' || c == '\t') {
    while((s[0] = c = getch()) == ' ' || c == '\t')
        ;
}


Here is a simpler version:

while (c == ' ' || c == '\t') {
    c = getch();
}
s[0] = c;


Your loop to read a number is repeated and repetition is normally undesirable.
The loop could be extracted into a function and called twice:

static int get_number(char *s)
{
    int c = getch();
    for (; isdigit(c); c = getch()) {
        *s++ = c;
    }
    *s = '\0';
    return c;
}


On end of file, this returns EOF.

Note that you should probably handle invalid input such as "1.2.3"

In main

char entry;
...
while((entry = getop(s)) != EOF) {


entry should really be an int

Code Snippets

if((s[0] = c) == ' ' || c == '\t') {
    while((s[0] = c = getch()) == ' ' || c == '\t')
        ;
}
while (c == ' ' || c == '\t') {
    c = getch();
}
s[0] = c;
static int get_number(char *s)
{
    int c = getch();
    for (; isdigit(c); c = getch()) {
        *s++ = c;
    }
    *s = '\0';
    return c;
}
char entry;
...
while((entry = getop(s)) != EOF) {

Context

StackExchange Code Review Q#39081, answer score: 3

Revisions (0)

No revisions yet.