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.

I have written some code for one of my assignments. However, I feel that I am repeating myself slightly in a few places. I have that niggling feeling that there is a better way to do things.

Here is the code in full:

#include <stdio.h>
#include <string.h>
#define ARR_SIZE 256

int main(void)
{
    //Declare char arrays
    char arcInputString5[ARR_SIZE];
    char arcInputString10[ARR_SIZE];
    char arcInputString15[ARR_SIZE];
    char arcInputString20[ARR_SIZE];
    int clean1, clean2, clean3, clean4, nCount;
    char buffer[ARR_SIZE];

    printf("\nPlease Input String 1 - Max Length 5: ");
    //gets(arcInputString5);
    fgets(arcInputString5, ARR_SIZE, stdin);

    for(clean1 = 0; clean1 < strlen(arcInputString5); clean1++)
    {
        if(arcInputString5[clean1] == '\n' || arcInputString5[clean1] == '\r')
        {
            arcInputString5[clean1] = '\0';
            break;
        }
    }

    printf("\nPlease Input String 2 - Max Length 10: ");
    //gets(arcInputString10);
    fgets(arcInputString10, ARR_SIZE, stdin);

    for(clean1 = 0; clean1 < strlen(arcInputString10); clean1++)
    {
        if(arcInputString10[clean1] == '\n' || arcInputString10[clean1] == '\r')
        {
            arcInputString10[clean1] = '\0';
            break;
        }
    }

    printf("\nPlease Input String 3 - Max Length 15: ");
    //gets(arcInputString15);
    fgets(arcInputString15, ARR_SIZE, stdin);

    for(clean1 = 0; clean1 < strlen(arcInputString15); clean1++)
    {
        if(arcInputString15[clean1] == '\n' || arcInputString15[clean1] == '\r')
        {
            arcInputString15[clean1] = '\0';
            break;
        }
    }

    printf("\nPlease Input String 4 - Max Length 20: ");
    //gets(arcInputString20);
    fgets(arcInputString20, ARR_SIZE, stdin);

    for(clean1 = 0; clean1 < strlen(arcInputString20); clean1++)
    {
        if(arcInputString20[clean1] == '\n' || arcInputString20[clean1] == '\r')
        {
            arcInputString20[clean1] = '\0';
            break;
        }
    }

    printf("\nThankyou For Your Inputs - They Are Shown Back To You Below\n\n");

    puts(arcInputString5);
    puts(arcInputString10);
    puts(arcInputString15);
    puts(arcInputString20);

    printf("\nThe String Lengths For Each Input Are Listed Below. \nIt is also indicated below if you have typed too many characters in");

    printf("\n%d", strlen(arcInputString5));
    printf("\n%d", strlen(arcInputString10));
    printf("\n%d", strlen(arcInputString15));
    printf("\n%d", strlen(arcInputString20));

    if(strlen(arcInputString5) > 5)
    {
        printf("\n\nString Length: %d - Exceeded Allowed Characters", strlen(arcInputString5));
    }
    else
    {
        printf("\n\nString Length: %d - NOT Exceeded Allowed Characters", strlen(arcInputString5));
    }

    if(strlen(arcInputString10) > 10)
    {
        printf("\n\nString Length: %d - Exceeded Allowed Characters", strlen(arcInputString10));
    }
    else
    {
        printf("\n\nString Length: %d - NOT Exceeded Allowed Characters", strlen(arcInputString10));
    }

    if(strlen(arcInputString15) > 15)
    {
        printf("\n\nString Length: %d - Exceeded Allowed Characters", strlen(arcInputString15));
    }
    else
    {
        printf("\n\nString Length: %d - NOT Exceeded Allowed Characters", strlen(arcInputString15));
    }

    if(strlen(arcInputString20) > 20)
    {
        printf("\n\nString Length: %d - Exceeded Allowed Characters", strlen(arcInputString20));
    }
    else
    {
        printf("\n\nString Length: %d - NOT Exceeded Allowed Characters", strlen(arcInputString20));
    }


    //printf("\n\nBelow are the strings Concatenated: \n");
    sprintf(buffer, "\n\nBelow are the strings Concatenated: \n\n>%s<>%s><%s<>%s<", arcInputString5, arcInputString10, arcInputString15, arcInputString20);
    printf("%s", buffer);
    //puts(buffer)
}

I feel that the 4 sections of this code:

for(clean1 = 0; clean1 < strlen(arcInputString20); clean1++)
{
    if(arcInputString20[clean1] == '\n' || arcInputString20[clean1] == '\r')
    {
        arcInputString20[clean1] = '\0';
        break;
    }
}

And this code:

if(strlen(arcInputString5) > 5)
{
    printf("\n\nString Length: %d - Exceeded Allowed Characters", strlen(arcInputString5));
}
else
{
    printf("\n\nString Length: %d - NOT Exceeded Allowed Characters", strlen(arcInputString5));
}

Could be shortened somehow, but I am unsure how to do this. They may not be able to, but that's why I'm posting here!

Any hints/tips are appreciated.

share|improve this question
5  
Hi Dr.Pepper. On CodeReview we rollback edits if they invalidate answers that have already been given... just a head's up. –  rolfl Jan 22 at 3:33
    
Ah ok mate, thanks for the heads up :) –  Dr.Pepper Jan 22 at 14:35
1  
"mate" is ok, "primate" is more appropriate in @rolfl's case :) –  Mat's Mug Jan 22 at 15:45
add comment

2 Answers 2

You currently have separate variables for each input. You could instead have a two-dimensional array:

char inputs[4][ARR_SIZE];

You have variables clean1, clean2, …, nCount, but only use clean1. Since they're only used as index variables in non-nested loops, you only need one.

Then you can refactor the reading parts to have a for loop that reads each input in turn. You can do something similar when printing the strings out.

For checking their lengths, you can also do something similar, but you might want to have an array of maximum lengths to check against, e.g.:

static const int max_lengths[] = { 5, 10, 15, 20 };
share|improve this answer
    
The clean2 and ncount etc were left in there from some testing. Thanks for noticing! took them out and updated OP. Thanks for your input, ill have a proper read over it and do some testing now. –  Dr.Pepper Jan 22 at 3:32
add comment

Whenever you have a self-contained chunk of code that serves an identifiable purpose, it's a good idea to package it in a function. That's true even if you aren't repeating yourself four times. The fact that all the code appears in quadruplicate makes it even more urgent to define some functions to make your code reusable.

To decide how to break up your code, the key questions to ask are:

  1. What is the purpose of this chunk of code? Can you give it a name? If you have a hard time naming the operation, maybe you need to decompose it further into smaller operations.
  2. What could vary? Those will be your parameters. What are the operation's outputs?

Here's how I recommend decomposing your main() function:

/**
 * Prints the prompt, then writes a '\0'-terminated string of user input to buf. 
 */
void input(const char *prompt, char *buf, int bufsize) {
    /* TODO: Exercise for you */
}

/**
 * Truncates str at the first '\n' or '\r' character.
 */
void chomp(char *str) {
    str[strcspn(str, "\n\r")] = '\0';
}

/**
 * Prints either "String Length: N - Exceeded Allowed Characters\n"
 *            OR "String Length: N - NOT Exceeded Allowed Characters\n"
 */
void report_whether_length_exceeded(const char *str, int maxlen) {
    /* TODO: Exercise for you */
}

int main() {
    char str5[ARR_SIZE], str10[ARR_SIZE], str15[ARR_SIZE], str20[ARR_SIZE];
    char buffer[ARR_SIZE];

    input("Please Input String 1 - Max Length 5:", str5, sizeof(str5));
    input("Please Input String 2 - Max Length 10:", str10, sizeof(str10));
    input("Please Input String 3 - Max Length 15:", str15, sizeof(str15));
    input("Please Input String 4 - Max Length 20:", str20, sizeof(str20));

    chomp(str5);
    chomp(str10);
    chomp(str15);
    chomp(str20);

    printf("Thank you for your inputs - they are:\n\n"
           "%s\n%s\n%s\n%s\n",
           str5, str10, str15, str20);

    printf("Their string lengths are:\n\n"
           "%d\n%d\n%d\n%d\n",
           strlen(str5), strlen(str10), strlen(str15), strlen(str20));

    report_whether_length_exceeded(str5, 5); 
    report_whether_length_exceeded(str10, 10);
    report_whether_length_exceeded(str15, 15);
    report_whether_length_exceeded(str20, 20);

    /* I assume you want to append all the strings to a buffer for fun.  Otherwise,
       you could achieve the same effect more easily just with printf().
       It would be a good habit to use snprintf() instead of sprintf() if
       it's available on your system. */
    sprintf(buffer, ">%s<>%s><%s<>%s<", str5, str10, str15, str20);
    printf("\nHere are the strings concatenated:\n\n%s\n", buffer);
}

Once you accomplish that, you could go one step further: instead of storing four strings in separate char arrays, you could hold them all in a single array of strings (that is, a two-dimensional char array). However, I consider it more important to understand how to decompose your code into functions, so I'll leave it at that.

A final note: you'll give yourself fewer headaches if you consistently end each printout with a trailing newline. Sometimes putting it at the end of the previous line, and sometimes putting it at the beginning of the next line makes your code harder to work with.

share|improve this answer
    
Thanks for the help mate, ill sit down and have a look at your answer properly later this evening –  Dr.Pepper Jan 22 at 14:37
1  
I think chomp() should be part of input() and input() should return the string length (since it has to compute it to trim the \n and \r. Also report_whether_length_exceeded() is an odd function - a simple boolean length_exceeded() and a printf would make more sense to me. Also the final unbounded sprintf is asking for trouble and seems unnecessary (just use printf to concatenate). –  William Morris Jan 23 at 22:06
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.