424,985 Members | 1,863 Online Need help? Post your question and get tips & solutions from a community of 424,985 IT Pros & Developers. It's quick & easy.

critique my program!

 P: n/a Hello C-landers, can you find any improvement or critique to this program? #include #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=99; array=0; array=55; array=18; array=2; array=67; array=0; array=9; e(); printar(); e(); pline(); e(); while (rounds <= (LIM+1)) { 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; } 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]); } void pline(void) { printf("------------------------------------------------------------------------------"); } Aug 11 '07 #1
10 Replies

 P: n/a On Aug 10, 10:40 pm, vonbres...@gmail.com wrote: Hello C-landers, can you find any improvement or critique to this program? #include #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=99; array=0; array=55; array=18; array=2; array=67; array=0; array=9; Right off the bat, you exceed the array bounds, by assigning something to the 8th element (array). 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 /* 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[i]; 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 #include /* 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; size_t arr_size = sizeof array / sizeof array; 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 Aug 11 '07 #2

 P: n/a On Fri, 10 Aug 2007 22:40:46 -0700, vonbreslau wrote: Hello C-landers, can you find any improvement or critique to this program? #include #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=99; array=0; array=55; array=18; array=2; array=67; array=0; array=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[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(); } 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[i]); } void pline(void) { printf("------------------------------------------------------------------------------"); } -- Army1987 (Replace "NOSPAM" with "email") No-one ever won a game by resigning. -- S. Tartakower Aug 11 '07 #3

 P: n/a Army1987 wrote: On Fri, 10 Aug 2007 22:40:46 -0700, vonbreslau wrote: >Hello C-landers, can you find any improvement or critique to thisprogram?#include #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); voidprintar(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! -- Ian Collins. Aug 11 '07 #4

 P: n/a #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=99; array=0; array=55; array=18; array=2; array=67; array=0; array=9; e(); printar(); e(); pline(); e(); while (rounds <= (LIM+1)) { 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; } 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]); } void pline(void) { printf("------------------------------------------------------------------------------"); } Aug 11 '07 #5

 P: n/a "osmium" Hello C-landers, can you find any improvement or critique to thisprogram?#include #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=99; array=0; array=55; array=18; array=2; array=67; array=0; array=9; e(); printar(); e(); pline(); e(); while (rounds <= (LIM+1)) { 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; } 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]);}void pline(void){printf("------------------------------------------------------------------------------");} -- Aug 11 '07 #6

 P: n/a 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. Aug 11 '07 #7

 P: n/a On Sat, 11 Aug 2007 11:44:05 -0700, vonbreslau wrote: 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; declares an array of 7 elements, numbered from array to array . 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. -- Army1987 (Replace "NOSPAM" with "email") No-one ever won a game by resigning. -- S. Tartakower Aug 11 '07 #8

 P: n/a vo********@gmail.com wrote: Hello C-landers, can you find any improvement or critique to this program? #include #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=99; array=0; array=55; array=18; array=2; array=67; array=0; array=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 Aug 11 '07 #9

 P: n/a "Ian Collins"

 P: n/a Serve Lau wrote: "Ian Collins" One good thing about initialising an array this way is the compiler willbe 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; arr = 0; arr = 1; arr = 2; There has to be a trade off between optional checking and compile speed, that's why we have tools like lint. -- Ian Collins. Aug 12 '07 #11 