2
\$\begingroup\$

I recently posted Spaces To Tabs Converter but it had lots of bugs, so I rewrote the code. I wanted to ask, is it good code? I didn't find any bugs.

#include <stdio.h>
#define TABVALUE 4
#define ARRAYSIZE 3

void emptyArray(int *a);

int main(void) {
    int c, d, s;
    int savedChars[ARRAYSIZE];
    c = s = d = 0;
    while ((c = getchar()) != EOF) {
        if (c == ' ') {
            s = 0;
            s++;
            for (int j = 0; j < TABVALUE - 1; j++) {
                d = getchar();
                if (d != EOF) {
                    savedChars[j] = d;
                    savedChars[j + 1] = '\0';
                    if (d == ' ') {
                        s++;
                    } else {
                        break;
                    }
                }
            }
            if (s == TABVALUE) {
                emptyArray(savedChars);
                putchar('\t');
                s = 0;
            } else {
                putchar(c);
                for (int i = 0; i < 3; i++) {
                    if (savedChars[i] != '\0') {
                        putchar(savedChars[i]);
                    }
                }
            }
        } else {
            if (c != EOF) putchar(c);
        }
    }
    return 0;
}

void emptyArray(int *a) {
    for (int i = 0; i < ARRAYSIZE; i++) {
        a[i] = '\0';
    }
}
\$\endgroup\$
  • \$\begingroup\$ Can you explain why variables only used within main are not local to main? \$\endgroup\$ – Eric Lippert Oct 3 '16 at 16:39
  • \$\begingroup\$ You are right :) I will make them local. \$\endgroup\$ – Muhamed Cicak Oct 3 '16 at 16:48
  • \$\begingroup\$ But i guess,#define VARIABLE_NAME must be global (or should be) ?? ARRAYSIZE must be anyways because i use it in emptyArray.But i am not sure about TABVALUE \$\endgroup\$ – Muhamed Cicak Oct 3 '16 at 16:50
  • \$\begingroup\$ Is there some reason why it needs to be defined with the preprocessor instead of, say, a constant local? \$\endgroup\$ – Eric Lippert Oct 3 '16 at 17:12
  • \$\begingroup\$ What do you mean ?? I am not sure i understanded.Are you talking about ARRAYSIZE ? \$\endgroup\$ – Muhamed Cicak Oct 3 '16 at 17:50
3
\$\begingroup\$

A chunk of memory that is used to temporarily queue up content before it is eventually flushed is typically called a buffer. A typical variable name would be buffer or buf.

You defined constants TABVALUE and ARRAYSIZE. It looks like ARRAYSIZE is dependent on TABVALUE, so you shouldn't hard-code both 4 and 3. You also shouldn't hard-code 3 in the for loop.

The program outline is basically

while ((c = getchar()) != EOF) {
    if (c == ' ') {
        /* Huge chunk of buffer manipulation code here... */
    } else {
        if (c != EOF) putchar(c);
    }
}

You've already ascertained that c is not EOF in the while loop, so if (c != EOF) is pointless. I would also recommend inverting the test so that there isn't a huge chunk of code before the else. This outline is equivalent to your logic:

while ((c = getchar()) != EOF) {
    if (c != ' ') {
        putchar(c);
    } else {
        /* Huge chunk of buffer manipulation code here... */
    }
}

The first space encountered gets stored as c, but subsequent spaces get written to the savedChars buffer. The code would be simpler if you just wrote the first space to the buffer too.

I'm not convinced that you need a savedChars buffer, though. The buffer will only contain a string of spaces, possibly followed by a non-space character at the end. You should be able to do the job using just the s count and the final character d.

\$\endgroup\$
  • \$\begingroup\$ Nice Lesson :) I am not sure why is if(c!=' '){putchar (c)} else{// all of the code} better than if(c==' '){//all of the code} else{putchar(c)} \$\endgroup\$ – Muhamed Cicak Oct 4 '16 at 16:06
  • \$\begingroup\$ And how could i do this without savedChars buffer ? Can you write an example ? \$\endgroup\$ – Muhamed Cicak Oct 4 '16 at 16:07
0
\$\begingroup\$

I am not really sure what the purpose of this is

        s = 0;
        s++;

inside the While loop, especially since you have already set that variable to 0 outside of the loop

c = s = d = 0;

but the main thing that I am thinking is that you want s to have a value of 1 before you do anything inside of that if statement. Instead of giving s such a large scope, let's create it inside of that if statement and let it go away when we leave that if statement, so get rid of the declaration outside of the while loop and rewrite the code like this

int main(void) {
    int c, d;
    int savedChars[ARRAYSIZE];
    c = d = 0;
    while ((c = getchar()) != EOF) {
        if (c == ' ') {
            int s = 1;
            //...

and then I look further down into the next if statement inside of the while loop and see that you set s = 0; but you don't use s after that, by this I mean that the next time we see s in the flow of the code, you are actually setting it back to 1 again. there isn't any need for this declaration. Get rid of it.

now the code looks like this for your main method

int main(void) {
    int c, d;
    int savedChars[ARRAYSIZE];
    c = d = 0;
    while ((c = getchar()) != EOF) {
        if (c == ' ') {
            int s = 1;
            for (int j = 0; j < TABVALUE - 1; j++) {
                d = getchar();
                if (d != EOF) {
                    savedChars[j] = d;
                    savedChars[j + 1] = '\0';
                    if (d == ' ') {
                        s++;
                    } else {
                        break;
                    }
                }
            }
            if (s == TABVALUE) {
                emptyArray(savedChars);
                putchar('\t');
            } else {
                putchar(c);
                for (int i = 0; i < 3; i++) {
                    if (savedChars[i] != '\0') {
                        putchar(savedChars[i]);
                    }
                }
            }
        } else {
            if (c != EOF) putchar(c);
        }
    }
    return 0;
}

I really think that you should take this blob of code and put it into its own method and then call it from Main instead of putting it all into Main.

maybe like this

int main(void)
{
    int savedChars[ARRAYSIZE];
    CodeToRun(savedChars);
}

void CodeToRun(int[] savedChars)
{
    int c = 0;
    int d = 0;
    while ((c = getchar()) != EOF) {
        if (c == ' ') {
            int s = 1;
            for (int j = 0; j < TABVALUE - 1; j++) {
                d = getchar();
                if (d != EOF) {
                    savedChars[j] = d;
                    savedChars[j + 1] = '\0';
                    if (d == ' ') {
                        s++;
                    } else {
                        break;
                    }
                }
            }
            if (s == TABVALUE) {
                emptyArray(savedChars);
                putchar('\t');
            } else {
                putchar(c);
                for (int i = 0; i < 3; i++) {
                    if (savedChars[i] != '\0') {
                        putchar(savedChars[i]);
                    }
                }
            }
        } else {
            if (c != EOF) putchar(c);
        }
    }
}

I would obviously change the name of the method to something more meaningful, I would also suggest that you name your variables a little better as well.

\$\endgroup\$
  • \$\begingroup\$ is my syntax wrong? \$\endgroup\$ – Malachi Oct 3 '16 at 19:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.