Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Does this code look okay?

#include <stdio.h>
#include <ctype.h>

int case_letters();

int main(void)

{    
    case_letters();
    return 0;
}

int case_letters()

{
    int ch;
    int numOfUpper = 0;
    int numOfLower = 0;

    printf("please enter a some characters, and ctrl + d to see result\n");

    while ((ch = getchar()) != EOF)
    {
        if (isdigit(ch))
        {
            printf("please enter a valid character\n");
            continue;
        }


        else if (isupper(ch))
        {
            numOfUpper++;
        }

        else if (islower(ch))
        {
            numOfLower++;
        }
    }

    return  printf("There are %d Uppercase letters, and %d Lowercase letters\n", numOfUpper, numOfLower);
}
share|improve this question
1  
There are more characters that upper()/lower() and digit(). See chemie.fu-berlin.de/chemnet/use/info/libc/libc_4.html –  Loki Astari Feb 1 '13 at 19:10
add comment

2 Answers 2

up vote 2 down vote accepted

This does not seem to handle if the user enters a special character. ex: * & % etc...

share|improve this answer
add comment
  • It looks unusual to have newlines between the function statement and body since they're associated with each other. This just adds extra lines and makes the code a little harder to read.

  • If case_letters() is just supposed to print something at the end and not return a value, it should return void (no value), not int. main() isn't doing anything with this function after calling it.

  • Unformatted outputs should use puts() instead of printf().

  • As @Rhs has mentioned, your program doesn't handle special characters. Based on this from @Loki Astari, there are additional types of characters and with accompanying testing functions.

  • According to this answer, CTRLD only works on Mac OS and Linux. For Windows, it's CTRLZ. All of this should be stated in the output in case the user is running this on Windows.

  • Consider making this more modular for increased maintainability:

    1. keep letter counters for final output (in main())
    2. collect each character from user
    3. determine if the character is a letter
    4. increment either counter if a letter is entered

    Here's what I came up with:

    #include <ctype.h>
    #include <stdbool.h> // add this library
    #include <stdio.h>
    
    bool valid_char(char ch)
    {
        if (isdigit(ch)) return false;
    
        // include additional non-letter checks...
    
        // if everything checks out:
        return true;
    }
    
    void update_counters(char ch, int* numOfUpper, int* numOfLower)
    {
        if (isupper(ch))
        {
            (*numOfUpper)++;
        }
        else if (islower(ch))
        {
            (*numOfLower)++;
        }
    }
    
    void get_letters(int* numOfUpper, int* numOfLower)
    {
        puts("Please enter some characters\n");
        puts("Press CTRL+D on Mac OS/Linux or CTRL+Z on Windows to see the results\n");
        char ch;
    
        while ((ch = getchar()) != EOF)
        {
            while (!valid_char(ch))
            {
                puts("Character must be a letter\n");
                ch = getchar();
            }
    
            update_counters(ch, numOfUpper, numOfLower);
        }
    }
    
    int main(void)
    {
        int numOfUpper = 0;
        int numOfLower = 0;
    
        int* upperPtr = &numOfUpper;
        int* lowerPtr = &numOfLower;
    
        get_letters(upperPtr, lowerPtr);
    
        printf("There are %d uppercase letters and %d lowercase letters\n", numOfUpper, numOfLower);
    }
    
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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