Source Checking Program - Critique?

Hello C Board,

This is my first post so I'll start with briefly introducing myself. My name is Bart, I'm 17 years old and live in the Netherlands.
I got into programming 4 days ago and started learning C with "The C Programming Language" by Kernighan and Ritchie. I've plowed through the first chapter and did the final exercise.

The assignment is to write a program that checks code that is put in for rudimentary syntax errors like missing parentheses, braces and brackets. I've tried my best but assume that my program could be improved on both functionality and code.

Code:

---

#include <stdio.h>
#define MAXINPUT 32766
/* This program checks the input on rudimentary syntax errors.

-Thoughts before coding-

1. Collect the input and store it in array of character type.
2. Check the input character by character.
3. Count number of opening and closing curly brackets ('{}') and see whether they match up or not.
4. If they are not equal, return to the user that he has forgotten a curly bracket.
5. Check the number of opening and closing open brackets ('()') and see whether they match up or not.
6. If they are not equal, return to the user that he has probably forgotten an open backet.
7. Check the number of opening and closing square brackets ('[]') and see whether they match up or not.
8. If they are not equal, return to the user that he has probably forgotten a square backet.
9. Check for single and double quotes and see whether the result of adding those up is an even number.
10. If this is not the case, return to the user that he has probably forgotten a single or double quote.
11. Check whether the amount of times '*' and '/' are used directly after each other is even.
12. If this is not the case, return to the user that he has probably forgotten to close a comment block
*/

int b_getinput(char b_input[], int maxinput); // Declaring fuction
main()
{
    int len; // Holds length of input
    int j; // Used for the loop to check array
    int ccurlo; // Holds amount of times opening curly brackets are used
    int ccurlc; // Holds amount of times closing curly brackets are used
    int copeno; // Holds amount of times opening open brackets are used
    int copenc; // Holds amount of times closing open brackets are used
    int csquareo; // Holds amount of times opening square brackets are used
    int csquarec; // Holds amount of times closing square brackets are used
    int dquote; // Holds amount of times double quotes are used
    int squote; // Holds amount of times single quotes are used
    int bcom; // Holds amount of times '*' and '/' are used subsequently

    char input[MAXINPUT] = {0}; // Holds current input

    j = ccurlc = ccurlo = copenc = copeno = csquarec = csquareo = dquote = squote = bcom = 0; // Set all integers to 0

    len = b_getinput(input, MAXINPUT); // Get value for len

    if (len > 0) // If there is input
        printf("\n The code you put in was:\n\n%s\nPOSSIBLE ERRORS: \n\n", input); // Print the code that was put in

    for (j = 0; j <= len; j++) // Loop for as long as j <= len
        {
            if (input[j] == '{') {
                ++ccurlo;
            }
            else if (input[j] == '}') {
                ++ccurlc;
            }
            else if (input[j] == '(') {
                ++copeno;
            }
            else if (input[j] == ')') {
                ++copenc;
            }
            else if (input[j] == '[') {
                ++csquareo;
            }
            else if (input[j] == ']') {
                ++csquarec;
            }
            else if (input[j] == '"') {
                ++dquote;
            }
            else if (input[j] == '\'') {
                ++squote;
            }
            else if (input[j] == '*') {
                if (input[j+1] == '/') {
                    ++bcom;
                }
            }

        }
    if (ccurlc != ccurlo) {
        printf ("The amount of opening curled brackets is not equal to the amount of closing curled brackets, check to see if you have forgotten to put '{' or '}' somewehere. \n\n");
    }
    if (copenc != copeno) {
        printf ("The amount of opening open brackets is not equal to the amount of closing open brackets, check to see if you have forgotten to put '(' or ')' somewehere. \n\n");
    }
    if (csquarec != csquareo) {
        printf ("The amount of opening square brackets is not equal to the amount of closing square brackets, check to see if you have forgotten to put '[' or ']' somewehere. \n\n");
    }
    if ((dquote % 2)!= 0) {
        printf ("The amount of double quotes you used is not an even number, check to see if you have forgotten to place a double quote somewhere.\n\n");
    }
    if ((squote % 2)!= 0) {
        printf ("The amount of single quotes you used is not an even number, check to see if you have forgotten to place a single quote somewhere.\n\n");
    }
    if ((bcom % 2)!= 0) {
        printf ("The amount of times you used */ is not an even number, check if you forgot to close a block of comments.\n\n");
    }
    return 0;
}

b_getinput(char s[], int lim)
{
    int c, i;

    for (i = 0; i < lim-1 && (c=getchar())!=EOF; ++i) // Create loop to run for as long as the amount of characters put in does not exceed the limit and EOF is not put in.
        s[i] = c; // Assign the value of c to place i in the array.
    if (c == '\0') {
        s[i] = c;
        ++i;
    }
    s[i] = '\0';
    return i;
}

---

It'd be very much appreciated if someone could take a look at the code and help me improve it.

Thanks in advance,
Bart

main() needs a type

I haven't compiled this to check it for errors...

But I will say that if you've got this working, even falteringly, after just 4 days of study, you're doing just plain fine!

Well done.

Of course, your code will improve with experience.

One thing you could do is roll things like ccurlo and ccurlc into a single variable, like this:

Code:

---

int curl;
if (input[j] == '{') curl++;
if (input[j] == '}') curl--;

/*other code here*/
if( !curl ) printf( "Uh oh!\n" );

---

The logic being, "curl" will only be zero if the number of opening and closing curlies are equal.

Quote:

---

Originally Posted by Epy

main() needs a type

---

Oh ok, why is that if I may ask? I noticed it in almost all the C code I saw on the internet but it hasn't been mentioned once in the book if I'm not mistaking

Quote:

---

Originally Posted by CommonTater

I haven't compiled this to check it for errors...

But I will say that if you've got this working, even falteringly, after just 4 days of study, you're doing just plain fine!

Well done.

Of course, your code will improve with experience.

---

Thanks a lot, I've compiled it and it's working without any errors here.

Quote:

---

Originally Posted by TheBigH

One thing you could do is roll things like ccurlo and ccurlc into a single variable, like this:

Code:

---

int curl;
if (input[j] == '{') curl++;
if (input[j] == '}') curl--;

/*other code here*/
if( !curl ) printf( "Uh oh!\n" );

---

The logic being, "curl" will only be zero if the number of opening and closing curlies are equal.

---

Great idea! Hadn't thought about that yet.

Quote:

---

Originally Posted by Bart de Koning

Oh ok, why is that if I may ask? I noticed it in almost all the C code I saw on the internet but it hasn't been mentioned once in the book if I'm not mistaking

---

main() is required by most Operating Systems to return an integer errorlevel to the OS. This value is used in parent programs and scripting (batch files) as a means of directing branching in the program. If you do not return an integer, the OS gets garbage which can result in very strange behaviour in parent programs and scripts. So, the minimum program skeleton for a C program is...

Code:

---

int main (void)
  {
    // your code here

    return 0;
}

---

The problem is that while your code may work correctly, it is incorrectly interfaced to the operating system.

BigH's suggestion is a good one except:
1) you should never combine statements on a single line. It makes code harder to read. Good indenting is almost as important as good code.
2) His final if() statement will return the incorrect result... Success means curl == 0... but if it does come out 0, he would print an error message... Try it like this...

Code:

---

int curl = 0;

if (input[j] == '{')
  curl++;
if (input[j] == '}')
  curl--;

/*other code here*/
if( curl != 0 )
  printf( "Uh oh!\n" );

---

Yeah... you should be THAT picky... it will help you start off with good habits that will stay with you as your code becomes increasingly complex.

(With apologies to BigH for tearing apart his code :D )

Quote:

---

Originally Posted by Bart de Koning

Oh ok, why is that if I may ask? I noticed it in almost all the C code I saw on the internet but it hasn't been mentioned once in the book if I'm not mistaking

---

The return type of main() is int, but the default return type for a function without one is int, so that is actually okay. Just don't use void or anything else. However, most people would consider this lazy (you do the same thing with b_getinnput) as there is no good reason to not just state the return type. That way, mistakes are explicit instead of implicit.

Code:

---

j = ccurlc = ccurlo = copenc = copeno = csquarec = csquareo = dquote = squote = bcom = 0; // Set all integers to 0

---

I think most people would consider this another poor practice as it is easier to read and maintain if you just initialize them individually in the declaration. Contra the impression of many newbies, good code is not about exploiting syntax shortcuts that make no difference to performance but seem cool because you can cram stuff into one line.

Code:

---

b_getinput(char s[], int lim)
{
    int c, i;
 
    for (i = 0; i < lim-1 && (c=getchar())!=EOF; ++i) // Create loop to run for as long as the amount of characters put in does not exceed the limit and EOF is not put in.

---

Only read one byte at a time if you are also processing one byte at a time and there is a reason to do so (eg, you are not accumulating it). It's inefficient and pointless otherwise. How about:

Code:

---

b_getinput(char s[], int lim) {
    int rv = fread(s, lim-1, 1, stdin);
    s[rv] = '\0';
    return rv;
}

---

BTW, why is this function called "b_getinput"? You can call it whatever you want, of course, but functions and variables that start with 'bX' or 'b_' are usually considered boolean, and this function is not.

Code:

---

    if (c == '\0') {
        s[i] = c;
        ++i;
    }

---

Source code is text and hence never contains '\0'. But if it did, this is a bizarre move: s[i] was already set to c, then you add one to i. What's the point?

I presume you are aware of this pitfall:

Code:

---

#include <stdio.h>

int main(void) {
        /* check it out ;) comment */
        char x = ''';

        puts("{{{ gotcha ]]]");

        return 0;
}

---

Subjective comment of the day.

Quote:

---

Originally Posted by CommonTater

1) you should never combine statements on a single line. It makes code harder to read. Good indenting is almost as important as good code.

---

For example, I thoroughly disagree with the use of the word "never" in that sentence.


Code where there are an excess of close brackets compared to open brackets at any point is not valid. e.g.

Code:

---

printf)))"WTF this isn't right!"(((;

---

You need to do more than increment on open bracket and decrement on close. You need to also ensure that the running total never goes negative.

Have you considered what happens if you have a string that contains brackets in it. e.g:

Code:

---

printf("ZOMG, ( this string has ( brackets in it !");

---

Once you're considered that and have a solution, have you considered that a string might contain escaped double-quotes in it? e.g.

Code:

---

printf("ZOMG, ) this string \" has ) brackets and \"double quotes\" in it !!");

---

Got that sorted? Good, now consider escaped backslashes followed by the end of string. e.g.

Code:

---

printf("Oh great, )\" now we're really screwed \\");

---

Oh, and you have the same issue in characters or comments e.g.

Code:

---

x = ')'; // The opposite of a '('

---

You also need to consider similar issues for brackets and quotes appearing inside a comment.
Finally the sequence, /*/ can indicate either the start or end of a multi-line comment!

The good news is that although these examples look scary, it's not too hard to make it work correctly for all of them and for everything you might run into in real code.
We can help!

Thank you guys so much for all the replies with constructive criticism! It is very much appreciated.

I have made a few changes to the code, following your guidelines.


1. Characters used in functions or comments are now not taken into account anymore.

2. Characters used while assigning values to variables are now not taken into account anymore either.

3. Instead of using 2 variables for each type of brace, the program now uses only 1 and adds up when it's an opening form and subtracts when it's a closing form. If the end result is not 0 a warning is printed to the user.

4. I have changed main() to int main(void)

5. I have changed b_getinput to a_getinput to avoid confusion with booleans.

6. I have changed the long line setting all integers to zero to one line per integer.


One thing I haven't changed though is the getline function suggested by MK27. The reason for this is that it uses a few things I have yet to come across in the book. My knowledge of C is still very limited and I don't want to use functions that I do not understand in an attempt to avoid learning bad habits.


The new code

Code:

---

#include <stdio.h>
#define MAXINPUT 32766
/* This program checks the input on rudimentary syntax errors.

-Thoughts before coding-

1. Collect the input and store it in array of character type.
2. Check the input character by character.
3. Count number of opening and closing curly brackets ('{}') and see whether they match up or not.
4. If they are not equal, return to the user that he has forgotten a curly bracket.
5. Check the number of opening and closing open brackets ('()') and see whether they match up or not.
6. If they are not equal, return to the user that he has probably forgotten an open backet.
7. Check the number of opening and closing square brackets ('[]') and see whether they match up or not.
8. If they are not equal, return to the user that he has probably forgotten a square backet.
9. Check for single and double quotes and see whether the result of adding those up is an even number.
10. If this is not the case, return to the user that he has probably forgotten a single or double quote.
11. Check whether the amount of times '*' and '/' are used directly after each other is even.
12. If this is not the case, return to the user that he has probably forgotten to close a comment block
*/

int a_getinput(char a_input[], int maxinput); // Declaring fuction
int main(void)
{
    int len; // Holds length of input
    int j; // Used for the loop to check array
    int curl; // Holds amount of times curled brackets are used
    int open; // Holds amount of times open brackets are used
    int square; // Holds amount of times square brackets are used
    int dquote; // Holds amount of times double quotes are used
    int squote; // Holds amount of times single quotes are used
    int bcom; // Holds amount of times '*' and '/' are used subsequently
    int in; // Holds a value to see whether the input should be counted or not.

    char input[MAXINPUT] = {0}; // Holds current input

    // Setting all integers to 0
    j = 0;
    curl = 0;
    open = 0;
    square = 0;
    dquote = 0;
    squote = 0;
    bcom = 0;
    in = 0;

    len = a_getinput(input, MAXINPUT); // Get value for len

    if (len > 0) // If there is input
        printf("\n The code you put in was:\n\n%s\nPOSSIBLE ERRORS: \n\n", input); // Print the code that was put in


    for (j = 0; j <= len; j++) {
    if (in == 0) { // If the value of in = 0 the code is not in (between): Quotation marks,  a comment block, a single comment line nor does it come after "="
            if (input[j] == '*') { // If /* is put in, add 1 to bcom and set in = 1. As the code put in hereafter until */ should not be checked.
                if (input[j-1] == '/') {
                    ++bcom;
                    in = 1;
                }
            }
            else if (input[j] == '/') { // If a single line comment is specified set in = 2.
                if (input[j+1] == '/'){
                    in = 2;
                }
            }
            else if (input[j] == '\"') { // If the coder has put in \" set in = 3.
                ++dquote;
                    in = 3;
            }
            else if (input[j] == '=') { // If the coder has put in a single "=" set in = 4. if == has been put in, in = 0.
                if (input[j+1] != '=')
                    in = 4;
            }
            else if (input[j] == '\'') { // If in isn't 1, 2, 3 or 4 and thus the single quote is written in a place that it has a function it adds 1 to squote
                ++squote;
            }
            else if (input[j] == '{') {
                ++curl;
            }
            else if (input[j] == '}') {
                --curl;
            }
            else if (input[j] == '(') {
                ++open;
            }
            else if (input[j] == ')') {
                --open;
            }
            else if (input[j] == '[') {
                ++square;
            }
            else if (input[j] == ']') {
                --square;
            }
        }
        if (in == 1) { // If a comment /* has been put in the program does not do anything with quotes, braces, brackets, parentheses etc. that are specified until */ is discovered.
            if (input[j] == '*') {
                if (input[j+1] == '/') {
                    --bcom;
                    in = 0; // Set in = 0 as the comment block has been closed and anything written after that has a function in the program.
                }
            }
        }
        else if (in == 2) {
            if (input[j] == '\n') { // The program does not do anything with quotes, braces, brackets, parentheses etc. that are specified until a new line is discovered.
                in = 0;
            }
        }
        else if (in == 3) { // The program does not check code between quotation marks until a quotation mark followed by a closing open bracket is discovered. If we take printf for example, there is no way we can place ") without getting out of printf.
            if (input [j] == '"') {
                if (input [j+1] == ')') {
                    in = 0;
                    ++dquote;
                }
            }
        }
        else if (in == 4) { // The program does not check code after a single '=' sign until a semicolon or newline is discovered.
            if (input[j] == '=' || input[j] == ';' || input[j] == '\n') {
                    in = 0;
            }
        }
    }
    if (curl != 0) {
        printf ("The amount of opening curled brackets minus the amount of closing curled brackets is not 0, check to see if you have forgotten to put '{' or '}' somewehere. \n\n");
    }
    if (open != 0) {
        printf ("The amount of opening open brackets minus to the amount of closing open brackets is not 0, check to see if you have forgotten to put '(' or ')' somewehere. \n\n");
    }
    if (square != 0) {
        printf ("The amount of opening square brackets minus the amount of closing square brackets is not 0, check to see if you have forgotten to put '[' or ']' somewehere. \n\n");
    }
    if ((dquote % 2)!= 0) {
        printf ("The amount of double quotes you used is not an even number, check to see if you have forgotten to place a double quote somewhere.\n\n");
    }
    if ((squote % 2)!= 0) {
        printf ("The amount of single quotes you used is not an even number, check to see if you have forgotten to place a single quote somewhere.\n\n");
    }
    if (bcom != 0) {
        printf ("The amount of times you used /* minus the amount of times you used */ is not 0, check if you forgot to close or open a block of comments.\n\n");
    }
    return 0;
}

a_getinput(char s[], int lim)
{
    int c, i;

    for (i = 0; i < lim-1 && (c=getchar())!=EOF; ++i) // Create loop to run for as long as the amount of characters put in does not exceed the limit and EOF is not put in.
        s[i] = c; // Assign the value of c to place i in the array.
        ++i;
    s[i] = '\0';
    return i;
}

---

Again, any constructive criticism is very much appreciated!

Sorry for the double post but I can't change the previous one.

I've added a new feature to my program that was suggested by iMalc.
If a closing open, square or curled bracket is put in the program checks whether the value of their corresponding variables goes negative. When this is the case it sets the value of another variable to 1. The program then prints to the user that he or she has placed a closing bracket before an opening one.

This is the new source code

Code:

---

#include <stdio.h>
#define MAXINPUT 32766
/* This program checks the input on rudimentary syntax errors.

-Thoughts before coding-

1. Collect the input and store it in array of character type.
2. Check the input character by character.
3. Count number of opening and closing curly brackets ('{}') and see whether they match up or not.
4. If they are not equal, return to the user that he has forgotten a curly bracket.
5. Check the number of opening and closing open brackets ('()') and see whether they match up or not.
6. If they are not equal, return to the user that he has probably forgotten an open backet.
7. Check the number of opening and closing square brackets ('[]') and see whether they match up or not.
8. If they are not equal, return to the user that he has probably forgotten a square backet.
9. Check for single and double quotes and see whether the result of adding those up is an even number.
10. If this is not the case, return to the user that he has probably forgotten a single or double quote.
11. Check whether the amount of times '*' and '/' are used directly after each other is even.
12. If this is not the case, return to the user that he has probably forgotten to close a comment block
*/

int a_getinput(char a_input[], int maxinput); // Declaring fuction
int main(void)
{
    int len; // Holds length of input
    int j; // Used for the loop to check array
    int curl; // Holds amount of times curled brackets are used
    int open; // Holds amount of times open brackets are used
    int square; // Holds amount of times square brackets are used
    int dquote; // Holds amount of times double quotes are used
    int squote; // Holds amount of times single quotes are used
    int bcom; // Holds amount of times '*' and '/' are used subsequently
    int in; // Holds a value to see whether the input should be counted or not.
    int cneg; // Holds a value to see whether a closing curled bracket has been placed in front of an open one.
    int oneg; // Holds a value to see whether a closing open bracket has been placed in front of an open one.
    int sneg; // Holds a value to see whether a closing square bracket has been placed in front of an open one.
    char input[MAXINPUT] = {0}; // Holds current input

    // Setting all integers to 0
    j = 0;
    curl = 0;
    open = 0;
    square = 0;
    dquote = 0;
    squote = 0;
    bcom = 0;
    in = 0;
    cneg = 0;
    oneg = 0;
    sneg = 0;

    len = a_getinput(input, MAXINPUT); // Get value for len

    if (len > 0) // If there is input
        printf("\n The code you put in was:\n\n%s\nPOSSIBLE ERRORS: \n\n", input); // Print the code that was put in


    for (j = 0; j <= len; j++) {
    if (in == 0) { // If the value of in = 0 the code is not in  (between): Quotation marks,  a comment block, a single comment line nor  does it come after "="
            if (input[j] == '*') { // If /* is put in, add 1 to bcom and  set in = 1. As the code put in hereafter until */ should not be  checked.
                if (input[j-1] == '/') {
                    ++bcom;
                    in = 1;
                }
            }
            else if (input[j] == '/') { // If a single line comment is specified set in = 2.
                if (input[j+1] == '/'){
                    in = 2;
                }
            }
            else if (input[j] == '\"') { // If the coder has put in \" set in = 3.
                ++dquote;
                    in = 3;
            }
            else if (input[j] == '=') { // If the coder has put in a single "=" set in = 4. if == has been put in, in = 0.
                if (input[j+1] != '=')
                    in = 4;
            }
            else if (input[j] == '\'') { // If in isn't 1, 2, 3 or 4 and  thus the single quote is written in a place that it has a function it  adds 1 to squote
                ++squote;
            }
            else if (input[j] == '{') {
                ++curl;
            }
            else if (input[j] == '}') {
                --curl;
                if (curl == -1) {
                    cneg = 1;
                }
            }
            else if (input[j] == '(') {
                ++open;
            }
            else if (input[j] == ')') {
                --open;
                if (open == -1) {
                    oneg = 1;
                }
            }
            else if (input[j] == '[') {
                ++square;
            }
            else if (input[j] == ']') {
                --square;
                if (square == -1) {
                    sneg = 1;
                }
            }
        }
        if (in == 1) { // If a comment /* has been put in the program  does not do anything with quotes, braces, brackets, parentheses etc.  that are specified until */ is discovered.
            if (input[j] == '*') {
                if (input[j+1] == '/') {
                    --bcom;
                    in = 0; // Set in = 0 as the comment block has been  closed and anything written after that has a function in the program.
                }
            }
        }
        else if (in == 2) {
            if (input[j] == '\n') { // The program does not do anything  with quotes, braces, brackets, parentheses etc. that are specified until  a new line is discovered.
                in = 0;
            }
        }
        else if (in == 3) { // The program does not check code between  quotation marks until a quotation mark followed by a closing open  bracket is discovered. If we take printf for example, there is no way we  can place ") without getting out of printf.
            if (input [j] == '"') {
                if (input [j+1] == ')') {
                    in = 0;
                    ++dquote;
                }
            }
        }
        else if (in == 4) { // The program does not check code after a  single '=' sign until a semicolon or newline is discovered.
            if (input[j] == '=' || input[j] == ';' || input[j] == '\n') {
                    in = 0;
            }
        }
    }
    if (curl != 0) {
        printf ("The amount of opening curled brackets minus the amount  of closing curled brackets is not 0, check to see if you have forgotten  to put '{' or '}' somewehere. \n\n");
    }
    if (open != 0) {
        printf ("The amount of opening open brackets minus to the amount  of closing open brackets is not 0, check to see if you have forgotten  to put '(' or ')' somewehere. \n\n");
    }
    if (square != 0) {
        printf ("The amount of opening square brackets minus the amount  of closing square brackets is not 0, check to see if you have forgotten  to put '[' or ']' somewehere. \n\n");
    }
    if ((dquote % 2)!= 0) {
        printf ("The amount of double quotes you used is not an even  number, check to see if you have forgotten to place a double quote  somewhere.\n\n");
    }
    if ((squote % 2)!= 0) {
        printf ("The amount of single quotes you used is not an even  number, check to see if you have forgotten to place a single quote  somewhere.\n\n");
    }
    if (bcom != 0) {
        printf ("The amount of times you used /* minus the amount of  times you used */ is not 0, check if you forgot to close or open a block  of comments.\n\n");
    }
    if (cneg == 1){
        printf ("You have placed a closing curled bracket before an opening one.\n\n");
    }
    if (oneg == 1){
        printf ("You have placed a closing open bracket before an opening one.\n\n");
    }
    if (sneg == 1){
        printf ("You have placed a closing square bracket before an opening one.\n\n");
    }
    return 0;
}

a_getinput(char s[], int lim)
{
    int c, i;

    for (i = 0; i < lim-1 && (c=getchar())!=EOF; ++i) //  Create loop to run for as long as the amount of characters put in does  not exceed the limit and EOF is not put in.
        s[i] = c; // Assign the value of c to place i in the array.
        ++i;
    s[i] = '\0';
    return i;
}

---

Any critique is still very much appreciated,

Bart