1
\$\begingroup\$

I have created this simple code:

For the size of 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.

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – luiscubal Feb 6 '13 at 19:51
  • \$\begingroup\$ @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. \$\endgroup\$ – jsanc623 Feb 6 '13 at 19:56
  • \$\begingroup\$ Also, this doesn't seem to be the whole function. \$\endgroup\$ – luiscubal Feb 6 '13 at 19:59
5
\$\begingroup\$

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);
    
\$\endgroup\$
  • 1
    \$\begingroup\$ Note that n and m don't seem to be the line/col count variables, they are pointers to the count variables. \$\endgroup\$ – luiscubal Feb 6 '13 at 20:07
  • \$\begingroup\$ 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. \$\endgroup\$ – Cyrille Ka Feb 6 '13 at 20:17

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.