critique my program!

[vonbreslau]

Hello C-landers, can you find any improvement or critique to this
program?

#include <stdio.h>
#define ZER 0
#define LIM 7
/* 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;
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)) {
for ( i = ZER ;i < LIM; ++i) {

if (array > array[i+1]) {
printf("%2d > %2d ",array, array[i+1]);

temporal = array[i+1];
array[i+1] = array;
array = temporal;
printar(); e();
}
}
++rounds;
}
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);
}
void pline(void)
{

printf("------------------------------------------------------------------------------");
}

 

[user923005]

Hello C-landers, can you find any improvement or critique to this
program?

#include <stdio.h>
#define ZER 0
#define LIM 7
/* 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;
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;

Right off the bat, you exceed the array bounds, by assigning something
to the 8th element (array[7]).
Also, the functions for printing a line and printing a newline are not
as good as just doing it inline.

Because of the way you have constructed your program the sort is not
reusable.

I would suggest something more along these lines:

#include <stdlib.h>
/* The definition for etype should come form an include file or
command line #define or the like. */
typedef int etype;

void quadratic_sort_bi(etype array[], const size_t lo,
const size_t hi)
{
size_t i,
j,
h,
l;
for (i = lo + 1; i <= hi; i++) {
const etype tmp = array;
for (l = lo - 1, h = i; h - l > 1 {
j = (h + l) / 2;
if (tmp < array[j])
h = j;
else
l = j;
}
for (j = i; j > h; j--)
array[j] = array[j - 1];
array[h] = tmp;
}
}

#ifdef UNIT_TEST

#include <stdio.h>
#include <time.h>

/* Because of promotion to double, the dumper should work for many
integral and floating point types */
void dump_arr(etype array[], size_t size)
{
size_t index;
for (index = 0; index < size; index++)
printf("array[%u]=%f\n", (unsigned) index, (double)
array[index]);
}

void init_arr(etype array[], size_t size)
{
size_t index;
for (index = 0; index < size; index++)
array[index] = (etype) rand();
}
int main(void)
{
int array[8];
size_t arr_size = sizeof array / sizeof array[0];
srand((unsigned)time(NULL));

init_arr(array, arr_size);
puts("before sort:");
dump_arr(array, arr_size);
quadratic_sort_bi(array, 0, arr_size - 1);
puts("after sort:");
dump_arr(array, arr_size);

return 0;
}

#endif

 

[Army1987]

Hello C-landers, can you find any improvement or critique to this
program?

#include <stdio.h>
#define ZER 0

Is there any reason for using three keystrokes instead than one
whenever you want to write 0, considering than 1) the compiler
proper (i.e. the one which parses preprocessed code) will not even
be aware of that, 2) this doesn't improves readability in no way
whatsoever, and 3) its value will never change?

#define LIM 7
/* Sort numbers according to its increasing size */ void e(void); void
printar(void);
void pline(void);

int array[LIM];

int array[LIM] = {99, 0, 55, 18, 2, 67, 0, 9}

int main()
{
int i, temporal, rounds;
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;

You can also initialize it when you declare it, see above.

e(); printar(); e(); pline(); e();

while (rounds <= (LIM+1)) {
for ( i = ZER ;i < LIM; ++i) {

if (array > array[i+1]) {
printf("%2d > %2d ",array, array[i+1]);

temporal = array[i+1];
array[i+1] = array;
array = temporal;
printar(); e();
}

Look up selection sort, is much faster than bubblesort and it is
about as simple as it.

}
++rounds;
}
pline(); e(); printar(); e(); e();
return 0;
}
void e(void)
{
printf("\n");
}
void printar(void)

You might want to pass the address and size of the array as
parameters.

{
int i;
for (i = ZER; i <= LIM; ++i)
printf("\t%2d ",array);
}
void pline(void)
{

printf("------------------------------------------------------------------------------");
}

 

[Ian Collins]

Hello C-landers, can you find any improvement or critique to this
program?

#include <stdio.h>
#define ZER 0

Is there any reason for using three keystrokes instead than one
whenever you want to write 0, considering than 1) the compiler
proper (i.e. the one which parses preprocessed code) will not even
be aware of that, 2) this doesn't improves readability in no way
whatsoever, and 3) its value will never change?

#define LIM 7
/* Sort numbers according to its increasing size */ void e(void); void
printar(void);
void pline(void);

int array[LIM];

int array[LIM] = {99, 0, 55, 18, 2, 67, 0, 9}

You left off the semicolon and initialised one to many items.

One good thing about initialising an array this way is the compiler will
be able to spot the error!

 

[osmium]

Hello C-landers, can you find any improvement or critique to this
program?

#include <stdio.h>
#define ZER 0
#define LIM 7
/* Sort numbers according to its increasing size */
void e(void);
void printar(void);
void pline(void);

int array[LIM];
int main()

array is what is called a global variable. This is bad practice, and while
not harmful here, there is nothing gained by making it global. Move the
array definition to below main. Rule of thumb: don't use global variables
unless there is a darn good reason.

I quit looking after seeing that.

{
int i, temporal, rounds;
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)) {
for ( i = ZER ;i < LIM; ++i) {

if (array > array[i+1]) {
printf("%2d > %2d ",array, array[i+1]);

temporal = array[i+1];
array[i+1] = array;
array = temporal;
printar(); e();
}
}
++rounds;
}
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);
}
void pline(void)
{

printf("------------------------------------------------------------------------------");
}

 

[Richard]

Hello C-landers, can you find any improvement or critique to this
program?

#include <stdio.h>
#define ZER 0
#define LIM 7
/* Sort numbers according to its increasing size */
void e(void);
void printar(void);
void pline(void);

int array[LIM];
int main()

array is what is called a global variable. This is bad practice, and while
not harmful here, there is nothing gained by making it global. Move the
array definition to below main. Rule of thumb: don't use global
variables

Not below main at all. Into main is what you meant. A big difference.

And had he done that then he must change the function declarations to
take a reference to the array. For the sake of this test code it hardly
seems worth it .... Clearly if this is NOT "test and learn" code then
globals are not the way.

unless there is a darn good reason.

There are many reasons. This might not be one of them....

I quit looking after seeing that.

Possibly you are in the wrong NG? This is a help newsgroup. If you get
all prissy and spit the dummy because of someone breaking one of the
conventions that YOU favour then you really are probably in the wrong
place.

{
int i, temporal, rounds;
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)) {
for ( i = ZER ;i < LIM; ++i) {

if (array > array[i+1]) {
printf("%2d > %2d ",array, array[i+1]);

temporal = array[i+1];
array[i+1] = array;
array = temporal;
printar(); e();
}
}
++rounds;
}
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);
}
void pline(void)
{

printf("------------------------------------------------------------------------------");
}

--

 

[vonbreslau]

Thanks a lot all of you guys for your comments!

Everyone gave me something to think about and to see the possibilities
of this language with your examples. For instance I certainly must
investigate all this:

"you exceed the array bounds by assigning something to the 8th
element",
the alternate way to initialize arrays
array[] = {x, x2, x3},
the fact that putting the array outside main is considered a bad
practice and could be harmful ( yet my program didn't worked until I
put it outside, don't have a clue why!)
Later someone said that if the array was to be into main then there
should be function declaration to reference it.
I trust all these things would be revealed in the next chapters of my
K&R book.

 

[Army1987]

Thanks a lot all of you guys for your comments!

Everyone gave me something to think about and to see the possibilities
of this language with your examples. For instance I certainly must
investigate all this:

"you exceed the array bounds by assigning something to the 8th
element",

int array[7]; declares an array of 7 elements, numbered from
array[0] to array [6].

the alternate way to initialize arrays
array[] = {x, x2, x3},
the fact that putting the array outside main is considered a bad
practice and could be harmful ( yet my program didn't worked until I
put it outside, don't have a clue why!)

Because then the function printarray() can't see it (unless you
pass it a pointer to it).

Later someone said that if the array was to be into main then there
should be function declaration to reference it.
I trust all these things would be revealed in the next chapters of my
K&R book.

Yes. You might find the FAQ www.c-faq.com useful, in case you
should not understand something.

 

[Thad Smith]

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 > array[i+1]) {
printf("%2d > %2d ",array, array[i+1]);

temporal = array[i+1];
array[i+1] = array;
array = 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);

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!

 

[Serve Lau]

One good thing about initialising an array this way is the compiler will
be able to spot the error!

isnt it strange that not many compilers spot this already? lint would see
it, why dont compilers do this yet

int arr[2];

arr[0] = 0;
arr[1] = 1;
arr[2] = 2;

 

[Ian Collins]

One good thing about initialising an array this way is the compiler will
be able to spot the error!

isnt it strange that not many compilers spot this already? lint would see
it, why dont compilers do this yet

int arr[2];

arr[0] = 0;
arr[1] = 1;
arr[2] = 2;

There has to be a trade off between optional checking and compile speed,
that's why we have tools like lint.