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.

My task was:

Write a function that takes three arguments: a character and two integers. The character is to be printed. The first integer specifies the number of times that the character is to be printed on a line, and the second integer specifies the number of lines that are to be printed. Write a program that makes use of this function.

And my code is: (i know i did not add different scenarios like if the first char is number etc, but please let me know if it looks ok)

#include <stdio.h>
void printing_char (char ch, int numberOfChars, int numberOfLines);

int main (void)
{
    char userChar;
    int lines,times;
    printf ("please enter a character, number of times in a line, and number of lines:\n");
    while ((scanf ("%c%d%d", &userChar, &times, &lines)) == 3)
    {
        printing_char (userChar, times, lines);
    }
    return 0;
}

void printing_char (char ch, int numberOfChars, int numberOfLines)
{
    int x;
    int y = 0;

    while (++y <= numberOfLines)
    {
            for (x = 0; x<numberOfChars; x++)
            {
                printf ("%c", ch);
            }
            printf ("\n");
    }
}
share|improve this question
    
printf("%c", ch); have you considered putchar(ch);? –  asveikau Feb 5 '13 at 5:09
add comment

2 Answers

up vote 2 down vote accepted

Only a couple of things:

  • int lines,times; - one declaration per line, please!

  • scanf("%c%d%d", &userChar, &times, &lines) - where does one number end and the next start?. You need to include some terminating characters, such as ,

    printf("please enter a character, number of times in a line, and number of lines, separated by a comma:\n");
    while ((scanf("%c,%d,%d", &userChar, &times, &lines)) == 3)
    
  • An expression such as while (++y <= numberOfLines) is confusing, what value does y have at each step? It'd be better to rewrite this as a for loop

    for ( y = 0; y < numberOfLines; y++ )
    
share|improve this answer
    
Thanks allot @GlennRogers –  Nir Feb 5 '13 at 0:13
add comment

In addition to comments by @GlennRogers, here's a bit of pedantry:

  • consider putting main last to avoid the need for a prototype for printing_char

  • printing_char should be static. Seems a bad name too.

  • for-loops can define the loop variable in the initial loop:

    for (int x = 0; x<numberOfChars; x++)
    
  • nested loops are generally best avoided. Your inner for-loop would be better as a function:

    static void print_n_times(int ch, int n)
    {
        for (int i=0; i<n; ++i) {
             putchar(ch);
        }
    }
    
  • adding a blank line after the function definition is very odd. Best not done.

  • numberOfChars and numberOfLines are verbose for my taste. I prefer n_chars, n_lines (or camel-case nChars, nLines). Also, as the variables in main have exactly the same meaning, it would be reasonable to call lines and times the same there, ie. n_chars and n_lines

  • you have unnecessary brackets around the scanf call

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.