2
\$\begingroup\$

K&R 1.8 exercise:

Write a program to count blanks,tabs, and newlines.

Is this a viable solution to the problem and is it properly formatted? I have never programmed in C before, so I just wanted to see if this is proper c programming as it were.

#include <stdio.h>

/* count blanks,tabs and new lines*/

int main(void)
{
    /* initializes blank,tab,newline and c
    sets them all to 0*/
    int blank,tab,newline;
    int c;
    blank = 0;
    tab = 0;
    newline = 0;

    while((c = getchar()) != EOF)
        if (c =='\n')
            ++newline;
        if (c == ' ')
            ++blank;
        if (c == '\t')
            ++tab;
        printf("Blanks: %d\nTabs: %d\nLines: %d\n", blank, tab, newline);
        return 0;
}
\$\endgroup\$
7
\$\begingroup\$

That is a great start. It's almost perfect logically.

First thing that caught my eye however is the lack of indentation. It could just be that something got lost in the transition between your code and your post here, I don't know. But you should indent whenever starting a new block. You indented the if bodies, that's good, but you missed the function body of main(), the while block. Had you done this, you would have been able to identify the following problem in your code.

The body of the while loop consists only of the first if statement that is counting newlines. You are effectively counting only newlines and not counting the blanks or tabs. This is how you have your code written but let's include some correct indentation:

while((c = getchar()) != EOF)
    if (c == '\n')
        ++newline;
if (c == ' ')
    ++blank;
if (c == '\t')
    ++tab;

Do you see the problem now?

There's a couple of ways to fix this but let me start with what you should have done always. If you ever have a block that would span multiple lines, you should always wrap the block in curly braces. That way there's no confusion what goes where. It's more forgivable to not include them if it's just a single line (just like you have in your individual if statements) but all three of them belongs to the while body so they should be in braces.

The other thing to about this code is that those three if statements are mutually exclusive. Only one of them could ever be hit for a given character, c. So you shouldn't have created a series of if statements like you did here, but instead a set of if/else blocks. That way the following if statements don't get executed if the preceding one was satisfied.
This could have fixed the logical error for the while loop too but it's still important to include the braces regardless so there's no confusion.

That block should have looked more like this:

while((c = getchar()) != EOF)
{
    if (c == '\n')
        ++newline;
    else if (c == ' ')
        ++blank;
    else if (c == '\t')
        ++tab;
}

Now there's some minor style issues in your code. You can follow these if you wish, it's just personal preference. But it could help other people read your code easier with little chance for confusion.

Your variable declarations should go on separate lines and the initializations of the variable should occur in the declaration. I'd also pluralize the variables since they are used as counters. In your head, if you read out the values of the variables what makes more sense? "I have 5 tab," or "I have 5 tabs? The comment was off a bit, we're not initializing c here, just declaring it.

Do this:

/* initializes blanks, tabs, and newlines and sets them all to 0 */
/* declares c to hold the read-in characters */
int c;
int blanks = 0;
int tabs = 0;
int newlines = 0;

And to a lesser degree, a call to printf() with parameters should be printed a single line at a time. The newline characters sprinkled in the format string makes it less clear how many lines this will print out.

Do this:

printf("Blanks: %d\n", blanks);
printf("Tabs: %d\n", tabs);
printf("Lines: %d\n", newlines);

So applying all of the above, your code would look like this:

#include <stdio.h>

 /* count blanks, tabs and newlines */

int main(void)
{
    /* initializes blanks, tabs, and newlines and sets them all to 0 */
    /* declares c to hold the read-in characters */
    int c;
    int blanks = 0;
    int tabs = 0;
    int newlines = 0;

    while((c = getchar()) != EOF)
    {
        if (c =='\n')
            ++newlines;
        else if (c == ' ')
            ++blanks;
        else if (c == '\t')
            ++tabs;
    }

    printf("Blanks: %d\n", blanks);
    printf("Tabs: %d\n", tabs);
    printf("Lines: %d\n", newlines);

    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ A few remarks: (1) Some argue that even single-line blocks should be surrounded with curly-braces; (2) It can be argued that for mutually exclusive conditions on a single integer variable switch is a better choice than if-else-if-else-if-else-..., and (3) Splitting what is basically a single print into 3 distinct printfs can cause issues regarding flushing etc. and is unnecessary to address the issue you raised. A better alternative might be printf("Blanks: %d\n" <nl> "Tabs: %d\n" <nl> "Lines: %d\n", <nl> blank, tab, newline);, where <nl> represents newline in the source file. \$\endgroup\$ – conio Dec 2 '12 at 0:56

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.