5
\$\begingroup\$

I am new to C. And I just want to know if my code is horrible or not. I tried to make program to delete all comments in C program. It's from "The C Programming Language Second Edition" book, Exercise 1-23.

Here is my solution :

#include <stdio.h>
#define MAXCHAR 1000
int c = 0;
int d = 0;
int newFileText[MAXCHAR];
int longDisable = 0;
int shortDisable = 0;
int i = 0;

//Function declarations
void printArray(int a[], int b);

void myputchar(int a[], int c);

int main(void) {
    while ((c = getchar()) != EOF) {
        if (i > MAXCHAR)i = 0;
        if (c == '/') {
            d = getchar();
            if (d == '/') shortDisable = 1;
            else if (d == '*') longDisable = 1;
        } else if (c == '*') {
            d = getchar();
            if (d == '/') {
                c = getchar();
                if (!(c > 0)) {
                    break;
                }
                longDisable = 0;
            }
        } else if (c == '\n') {
            shortDisable = 0;
        }
        if (shortDisable || longDisable);
        else {
            myputchar(newFileText, c);
            i++;
        }
        /*printf("\nShort disable: %d", shortDisable);
        printf("\nLong disable: %d", longDisable);*/
    }
    printArray(newFileText, i);
    return 0;
}

void printArray(int theArray[], int lengthSoFar) {
    for (int i = 0; i < lengthSoFar; i++) {
        printf("%c", theArray[i]);
    }
}

void myputchar(int a[], int c) {
    int i = 0;
    while (a[i] != '\0') {
        i++;
    }
    a[i] = c;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Take a look at this other code review which tried to solve the same exercise. Maybe you can learn something from that question and it's answers, too. \$\endgroup\$ – I'll add comments tomorrow Sep 29 '16 at 18:12
  • \$\begingroup\$ Its far too complicated than it should be i think.There is more simpler answer.Answered by @200_success \$\endgroup\$ – Muhamed Cicak Sep 29 '16 at 18:35
  • \$\begingroup\$ But ivan's solution accounts for string literals correctly, so there is something to be learned from it. \$\endgroup\$ – 200_success Sep 29 '16 at 18:37
  • \$\begingroup\$ I'm just too beginner to C, to understand his code.I had a look.But nothing seems understandable for me :) \$\endgroup\$ – Muhamed Cicak Sep 29 '16 at 18:38
  • \$\begingroup\$ Well ^that being said, you have to start learning new things. It's useful. Enjoy your trip in C-world ! \$\endgroup\$ – Grajdeanu Alex. Sep 30 '16 at 4:18
5
\$\begingroup\$

Bugs

When processing an expression like 10/2 or 10*2, the 2 gets discarded from the output.

It looks like you attempted to implement a circular buffer for the output, but it actually fails miserably if the output exceeds 1000 bytes. One problem is performance: myputchar() performs a linear scan of the buffer to find the end of the string, where it can write the next character. A second problem is buffer overflow: myputchar() makes no attempt to ensure that the location where it stores the next character lies within the buffer. The third problem is that when you do if (i > MAXCHAR)i = 0, you haven't made any attempt to print the buffer contents before trying to reuse the buffer.

C grammar

In C, a /* or // that appears within a "string literal" should be treated literally. It should not be interpreted as a start-of-comment marker.

Furthermore, within a //-style comment, any /* or */ needs to be disregarded. Similarly, within a /* */-style comment, any // should be disregarded. That is, the correct output for the following C99 program…

#include <stdio.h>
int main() {
    puts("First " /* asdf // */ "example"
    );
    puts("Second " // /*
         "example"
         // */
    );
    puts("Third " /* "/" */ "example");
}

… should be

First example
Second example
Third example

(I also got this wrong in Rev 1 of this answer. I've corrected the bug, but this illustrates a point that C requires a stateful parser.)

Organization

The variables c, d, newFileText, longDisable, shortDisable, and i are global; they should all be local to the main function.

It is customary to put the main function last, so that you don't need to declare the functions it uses.

Input / output

You read one character at a time with getchar(), but you attempt to print the output in chunks of up to 1000 characters. Writing in chunks could be more efficient than writing a character at a time, but it is certainly more difficult to implement correctly. However, after you go through the effort of accumulating the chunks, you then proceed to print the buffer one character at a time using printf("%c", theArray[i]), which negates any performance advantage you could have gained.

Therefore, you would have been better off printing the output one character at a time to begin with, without any attempt at buffering. To do so, you could use putchar(…), which is simpler than printf("%c", …).

If you did want to efficiently write a chunk of bytes at a time, you could use the write() function instead of the loop in printArray().

Suggested solution

This solution addresses the more serious logic bugs, without attempting to handle string literals correctly.

#include <stdio.h>

int main(void) {
    int c, d;
    int longDisable = 0, shortDisable = 0;

    while ((c = getchar()) != EOF) {
        switch (c) {
          case '/':
          case '*':
            d = getchar();
            if (!longDisable && c == '/' && d == '/') {
                shortDisable = 1;
            } else if (!shortDisable && c == '/' && d == '*') {
                longDisable = 1;
            } else if (!shortDisable && c == '*' && d == '/') {
                longDisable = 0;
            } else if (!(shortDisable || longDisable)) {
                putchar(c);
                if (d != EOF) putchar(d);
            }
            break;

          case '\n':
            shortDisable = 0;
            /* fall through... */
          default:
            if (!(shortDisable || longDisable)) {
                putchar(c);
            }
        }
    }
}
|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for the lesson :) And some people told me that (char c=getchar())!=EOF is bad to do.They said i should do int c instead of char c. \$\endgroup\$ – Muhamed Cicak Sep 29 '16 at 18:22
  • 1
    \$\begingroup\$ Actually, you're right. Technically, it should be int c, d. \$\endgroup\$ – 200_success Sep 29 '16 at 18:23
  • \$\begingroup\$ And there is one part that i dont understand very well.I am not really familiar with switch statement so i might be wrong.You wrote if (c=='/' && d=='/').Isnt c anyways equal to '*' there ?? Because its in case '*' ?? \$\endgroup\$ – Muhamed Cicak Sep 29 '16 at 18:29
  • \$\begingroup\$ No. The way switch works, if c is either '/' or '*', the code starting with d = getchar() will be executed. \$\endgroup\$ – 200_success Sep 29 '16 at 18:34
  • \$\begingroup\$ Oh,okay i understand now.Thank you for helping me out.This code looks much better than mine.Its far too simpler.Nice lesson :) \$\endgroup\$ – Muhamed Cicak Sep 29 '16 at 18:36

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.