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 created this simple code: for size an array with 2 values (n and m) verify if n and m are >= 1. m = width and n = height. The code works but I'm sure it isn't optimized.

The code :

printf("Number of ligns.");
scanf("%d",n);
printf("Number of columns.");
scanf("%d",m);


if(*n >= 1)
{
    if(*m >= 1)
    {
        printf("Number of ligns : %x, Number of columns : %x \n", *n, *m);
    }
    else
    {
        while(*m < 1)
        {
            printf("Error, number of colums/ligns < 1. \n");
            scanf("%d",m);
        }
        printf("Number of ligns : %x, Number of columns : %x \n", *n, *m);
    }
}
else
{
    while(*n < 1)
    {
        printf("Error, number of colums/ligns < 1. \n");
        scanf("%d",n);
    }
            if(*m >= 1)
    {
        printf("Number of ligns : %x, Number of columns : %x \n", *n, *m);
    }
    else
    {
        while(*m < 1)
        {
            printf("Error, number of colums/ligns < 1. \n");
            scanf("%d",m);
        }
        printf("Number of ligns : %x, Number of columns : %x \n", *n, *m);
    }
}
share|improve this question
    
The big question is not whether it is optimized. The big question is "Does it matter"? Do you find it too slow? How fast would you like it to be? –  luiscubal Feb 6 '13 at 19:51
    
@luiscubal I think this is a learning exercise - and if so, its more about learning how to do it the right way than optimizing for performance. At first glance, I see they are violating the DRY principle. –  jsanc623 Feb 6 '13 at 19:56
    
Also, this doesn't seem to be the whole function. –  luiscubal Feb 6 '13 at 19:59

1 Answer 1

up vote 4 down vote accepted

You should provide a compiling code. I'll assume that n and m are int, but you should have added their declaration.

  • You should use explicit names for variables. n is a line count, so call it line_count. m is column_count.
  • You should check the return values of scanf to be sure you handle gracefully EOF and bad input.
  • You print "Error, number of colums/ligns < 1. \n" every time, so the user can't know if it's the lines or the columns or both that are < 1.
  • The english word is "lines", not "ligns".
  • Your checks are too complicated and redundants. The entire if/else block can be replaced by this:

    while(*line_count < 1)
    {
        printf("Error, number of lines < 1. \n");
        scanf("%d", line_count);
    }
    
    
    while(*column_count < 1)
    {
        printf("Error, number of colums < 1. \n");
        scanf("%d", column_count);
    }
    
    printf("Number of lines : %x, Number of columns : %x \n", *line_count, *column_count);
    
share|improve this answer
    
Note that n and m don't seem to be the line/col count variables, they are pointers to the count variables. –  luiscubal Feb 6 '13 at 20:07
    
Indeed. But I am often reluctant to put some "ptr" prefix or suffix in the names. I had an overdose of hungarian notation several years ago, I try to avoid anything that looks like it. –  Cyrille Ka Feb 6 '13 at 20:17

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.