vo********@gmail.com wrote:
Hello C-landers, can you find any improvement or critique to this
program?
#include <stdio.h>
#define ZER 0
#define LIM 7
One person already recommended against the definition for ZER, but not
LIM. I agree. The difference is that LIM, presumably an abbreviation
for limit, can be seen as a parameter. You may later want to change
that value. ZER, an abbreviation for zero, should never change and has
no meaning beyond the constant zero. If your code had an adjustable
lower limit, which happened to be 0, but might be changed to something
else, then you could write
#define LOWER_LIMIT 0 /* lowest sort index */
I don't think it applies to your current program, though, since the
lower limit is not expected to change.
/* Sort numbers according to its increasing size */
void e(void);
void printar(void);
void pline(void);
int array[LIM];
int main()
{
int i, temporal, rounds;
One of my habits I use to reduce errors is to add a comment for each
declaration, most importantly variables and data types. If the variable
usage is blindingly obvious, such as i here, I will probably write /*
temp */. But what about temporal and rounds? What do they really hold?
My suggestion is /* temporary for swap */ and /* number of swap passes
completed */. This helps because a clear definition reduces confusion
over how it should be used.
As an example of reducing confusion, look at LIM. Is it the number of
elements in the array or is it the highest index of an array element?
Part of your code assumes one definition while other parts assume
another. By being explicit in the name and comment for the definition,
you help to prevent those types of problems:
#define N_VALUES 8 /* number of values to sort */
rounds = ZER;
array[0]=99; array[1]=0; array[2]=55; array[3]=18;
array[4]=2; array[5]=67; array[6]=0; array[7]=9;
e(); printar(); e(); pline(); e();
while (rounds <= (LIM+1)) {
How did you choose your limit for the while loop? Is it always
sufficient? Is it more than needed?
for ( i = ZER ;i < LIM; ++i) {
if (array[i] array[i+1]) {
printf("%2d %2d ",array[i], array[i+1]);
temporal = array[i+1];
array[i+1] = array[i];
array[i] = temporal;
printar(); e();
}
}
++rounds;
}
There are many ways to write a loop for a fixed number of iterations, as
you do with the while loop. My favorite is the for loop, since it has
the counter initialization, termination test, and increment together,
making it easier for me to read. The while loop is more appropriate,
IMO, when the index variable does not get incremented in a regular
manner at the end/beginning of the loop.
pline(); e(); printar(); e(); e();
return 0;
}
void e(void)
{
printf("\n");
}
void printar(void)
{
int i;
for (i = ZER; i <= LIM; ++i)
printf("\t%2d ",array[i]);
Here you use both a tab and spaces to separate elements. It would
probably be better with one or the other, not both. Also, I would
probably have printar output a terminating new line, unless you expect
to use it some time without a following new line. That is a minor
issue, though.
}
void pline(void)
{
printf("------------------------------------------------------------------------------");
}
Have fun with C!
--
Thad
