Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > Request for source code review

Reply
Thread Tools

Request for source code review

 
 
James Kuyper
Guest
Posts: n/a
 
      08-30-2012
On 08/30/2012 01:04 AM, Ian Collins wrote:
> On 08/30/12 01:55 PM, Keith Thompson wrote:

....
>> This isn't directly a C issue, but your Makefile doesn't reflect any
>> dependencies on the *.h files. For example, if you edit gauss.h and
>> type "make", it won't rebuild anything.

>
> A decent make should provide a means for automating this. Otherwise
> where do you stop recursing up the include paths?


I've used a "decent" make that was part of a configuration management
system called ClearCase, which fully automated the tracking of
dependencies, but by rather drastic means. Whenever clearmake executed
the build script for a given target, it would monitor the processes that
were executed, keeping track of ALL files that were opened during the
build. If they were versioned files managed by ClearCase, it identified
which version of the file was currently visible; for other files it just
looked at the file date. The next time that the same target was built,
it would check to see whether any of the files that had been opened the
last time had been changed, and if so, it assumed that the build script
had to be re-executed.

One of the key things that made this work was that the make facility was
tightly integrated with a special device driver that managed access to
disk space; only the use of files stored on disk volumes managed by that
driver could be detected.

After paying the rather hefty license fees for such sophisticated
software, guess how it was used: whenever we delivered new code, CM
would build it using the option that forced a complete re-build,
regardless of dependencies.
--
James Kuyper
 
Reply With Quote
 
 
 
 
Malcolm McLean
Guest
Posts: n/a
 
      08-30-2012
בתאריך יום רביעי, 29 באוגוסט 2012 20:19:06 UTC+1, מאת Udyant Wig:
> I was following Steve Summit's C programming notes at
>
> Being a novice, I request the readers of this newsgroup to look over the
> source to suggest improvements regarding readability, layout, style,
> correctness, idioms, etc. I would appreciate it very much.
>
>

It's a lot better than most newbie code. But you need to think a bit
more about what makes a good function.

d() is a good function, but a bad nmae. It rolls a ide and returns the
result. It's intuitive, it's resuable, it represents a natural module.

However some of the printing functions seem to exist purely to get code out
of main(). When main() becomes so long that it's unreadable then it's
necessary to break it down. But when the program is quite simple, it's better
not to hide things in functions.


This function for example is really just a wrapper for a printing loop

/* Print the number of occurrences of the various sums. */


void printsums (int sums [], int limit) {

int i;


for (i = NDICE; i < limit; i++)
printf ("%2d: %d\n", i, sums [i]);

}


then it's got another problem with it, which is that it's taking one
limit as an argument and the other as a compile time constant. So you're
creating confusion. Don't be afraid to pass a compile time constant as
a parameter. Often that makes functions more understandable and more
general.


histsums() however is more substantial, and drawing a histogram is a natural
unit of work. So the case for this being in a function is a lot stronger.
You could also write a top-level fucntion printoutput(), if you want a tidy
main().

 
Reply With Quote
 
 
 
 
Ben Bacarisse
Guest
Posts: n/a
 
      08-30-2012
Udyant Wig <(E-Mail Removed)> writes:

> Keith Thompson <(E-Mail Removed)> writes:
> | This isn't directly a C issue, but your Makefile doesn't reflect any
> | dependencies on the *.h files. For example, if you edit gauss.h and
> | type "make", it won't rebuild anything.
> I will have to learn to do that, then.
>
> | And you might as well use "-std=c99", or even "-std=c11" if you have a
> | new enough version of clang.
> I have been focusing on ANSI C90, the language as described in K&R 2nd
> ed. Sadly, there won't be new editions of K&R to expound the later C
> standards.
>
> I understand that some of the useful new features of C99 include:
> * variable length arrays
> * a Boolean type
> * C++ comments
> * `for' loop variable declaration
> * struct initialization


They are called "compound literals". "Old" C has struct initialisation,
so that's not a good term for it. What C99 adds is the ability to
"denote" an unnamed struct object:

plot((struct point){3, 4});

> * the long long type
> * complex numbers
>
> Does there exist learning material for C99 as there does for C90?
> Or would I be better off learning C99 as a `patch' for C90?


I'd do it that way. If you find K&R's style suits you, there is
really no better way to start and C99's extras are small.

VLAs are the most involved, I think, and they have been made optional in
C11. That's a crying shame to my mind, but that's how things have
panned out. No one who's implemented VLAs will remove the support, but
it makes them a second-class feature of the language.

--
Ben.
 
Reply With Quote
 
Udyant Wig
Guest
Posts: n/a
 
      08-30-2012
Jorgen Grahn <(E-Mail Removed)> writes:
| Off-topic hint: if you call the flags CFLAGS instead of FLAGS, you can
| skip the object file targets. Gnu Make has builtin rules for such
| things. See the manual.
That worked.

I will have to study the manual at some point.

| /Jorgen
 
Reply With Quote
 
Udyant Wig
Guest
Posts: n/a
 
      08-30-2012
lovecreatesbeauty <(E-Mail Removed)> writes:
|> I will have to learn to do that, then.
|>
|
| try my little Auto Makefile
|
| https://github.com/jhlicc/Visual-Stu...aster/Makefile
That looks interesting. It should prove useful.

It says on line 3:

# .c and Makefile in src, .h can be in include, no other directories.

Do the headers need to be in src or beside it?


 
Reply With Quote
 
Udyant Wig
Guest
Posts: n/a
 
      08-30-2012
Malcolm McLean <(E-Mail Removed)> writes:
| It's a lot better than most newbie code. But you need to think a bit
| more about what makes a good function.
|
| d() is a good function, but a bad nmae. It rolls a ide and returns the
| result. It's intuitive, it's resuable, it represents a natural module.
I can think of:
* die
* roll_die
* n_sided_die

| However some of the printing functions seem to exist purely to get code out
| of main(). When main() becomes so long that it's unreadable then it's
| necessary to break it down. But when the program is quite simple, it's better
| not to hide things in functions.
|
| This function for example is really just a wrapper for a printing loop
|
|
| /* Print the number of occurrences of the various sums. */
|
| void printsums (int sums [], int limit) {
|
| int i;
|
| for (i = NDICE; i < limit; i++)
| printf ("%2d: %d\n", i, sums [i]);
| }
|
|
| then it's got another problem with it, which is that it's taking one
| limit as an argument and the other as a compile time constant. So you're
| creating confusion. Don't be afraid to pass a compile time constant as
| a parameter. Often that makes functions more understandable and more
| general.
I have used this convention in two other functions (histogram &
histsums). I should redesign them too.

| histsums() however is more substantial, and drawing a histogram is a natural
| unit of work. So the case for this being in a function is a lot stronger.
| You could also write a top-level fucntion printoutput(), if you want a tidy
| main().
My aim was to make clear the flow of the program in main. Anything
that did some specific work, I moved to its own function.

But, I get your point.

Incidentally, when faced with an unfamiliar codebase, where do you
begin reading?
 
Reply With Quote
 
Ben Bacarisse
Guest
Posts: n/a
 
      08-30-2012
Udyant Wig <(E-Mail Removed)> writes:

> Malcolm McLean <(E-Mail Removed)> writes:
> | It's a lot better than most newbie code. But you need to think a bit
> | more about what makes a good function.
> |
> | d() is a good function, but a bad nmae. It rolls a ide and returns the
> | result. It's intuitive, it's resuable, it represents a natural module.
> I can think of:
> * die
> * roll_die
> * n_sided_die


It's not really anything about dice. It picks a number in [1,n] with
uniform probability. I'd probably go with something like uniform_int_in
and then make the lower bound a parameter too:

int uniform_int_in(int min, int max);

> | However some of the printing functions seem to exist purely to get code out
> | of main(). When main() becomes so long that it's unreadable then it's
> | necessary to break it down. But when the program is quite simple, it's better
> | not to hide things in functions.
> |
> | This function for example is really just a wrapper for a printing loop
> |
> |
> | /* Print the number of occurrences of the various sums. */
> |
> | void printsums (int sums [], int limit) {
> |
> | int i;
> |
> | for (i = NDICE; i < limit; i++)
> | printf ("%2d: %d\n", i, sums [i]);
> | }
> |
> |
> | then it's got another problem with it, which is that it's taking one
> | limit as an argument and the other as a compile time constant. So you're
> | creating confusion. Don't be afraid to pass a compile time constant as
> | a parameter. Often that makes functions more understandable and more
> | general.
> I have used this convention in two other functions (histogram &
> histsums). I should redesign them too.
>
> | histsums() however is more substantial, and drawing a histogram is a natural
> | unit of work. So the case for this being in a function is a lot stronger.
> | You could also write a top-level fucntion printoutput(), if you want a tidy
> | main().
> My aim was to make clear the flow of the program in main. Anything
> that did some specific work, I moved to its own function.


Personally, I prefer too see too many functions rather than too few.
Over time, laziness (if you at all like me) will cause you to get
a better balance. It's much harder to get into the habit of hiving off
code into functions than it is to get out of it.

> But, I get your point.
>
> Incidentally, when faced with an unfamiliar codebase, where do you
> begin reading?


If you are lucky, the READ_ME file or the huge explanatory comment at
the top of main.c! Failing that, I start with main() to see if I can
get the fell of what's happening, but I will probably jump around other
modules too just to get familiar with what is where.

--
Ben.
 
Reply With Quote
 
Ian Collins
Guest
Posts: n/a
 
      08-31-2012
On 08/30/12 11:43 PM, James Kuyper wrote:
> On 08/30/2012 01:04 AM, Ian Collins wrote:
>> On 08/30/12 01:55 PM, Keith Thompson wrote:

> ....
>>> This isn't directly a C issue, but your Makefile doesn't reflect any
>>> dependencies on the *.h files. For example, if you edit gauss.h and
>>> type "make", it won't rebuild anything.

>>
>> A decent make should provide a means for automating this. Otherwise
>> where do you stop recursing up the include paths?

>
> I've used a "decent" make that was part of a configuration management
> system called ClearCase, which fully automated the tracking of
> dependencies, but by rather drastic means. Whenever clearmake executed
> the build script for a given target, it would monitor the processes that
> were executed, keeping track of ALL files that were opened during the
> build. If they were versioned files managed by ClearCase, it identified
> which version of the file was currently visible; for other files it just
> looked at the file date. The next time that the same target was built,
> it would check to see whether any of the files that had been opened the
> last time had been changed, and if so, it assumed that the build script
> had to be re-executed.
>
> One of the key things that made this work was that the make facility was
> tightly integrated with a special device driver that managed access to
> disk space; only the use of files stored on disk volumes managed by that
> driver could be detected.


I've had hours of fun with clearmake...

Sun's (d)make uses the output of the preprocessor phase of compiles to
collate dependency information. Rather more elegant and a lot cheaper!

--
Ian Collins
 
Reply With Quote
 
Udyant Wig
Guest
Posts: n/a
 
      08-31-2012
Firstly, I thank all reader
 
Reply With Quote
 
Udyant Wig
Guest
Posts: n/a
 
      08-31-2012
I thank all readers who responded to my request.

I have revised the source taking into account many of the remarks made.
Follows the new listing.

/* utils.h begins */

#include <stdlib.h>

extern int uniform_int_in (int min, int max);

/* utils.h ends */



/* utils.c begins */

#include "utils.h"

/* Pick a number in [1, n] with uniform probability. */
int
uniform_int_in (int min, int max) {
return min + (rand () / (RAND_MAX / max + 1));
}

/* utils.c ends */



/* gauss.h begins */

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

#include "utils.h"


#define NDICE 5 /* The number of dice to be rolled. */
#define SIDES 6 /* Each die has these many sides. */
#define MAXSUM (SIDES * NDICE) /* The maximum possible sum of NDICE
dice.
*/
#define LIMIT ((MAXSUM) + 1) /* This constant is used:
0) to declare the array of sums
1) as a loop limit
*/
#define ROLLS 100000 /* The number of throws of the dice */
#define SCALE 100 /* Used to fit the histogram into the
screen width. A lower value
implies longer bars.
*/

extern int roll_die (int n);
extern int sumdice (int ndice);
extern void fillsums (int sums [], int maxrolls);
extern void printsums (int sums [], int begin_limit, int end_limit);
extern void histogram (int sum, int n, int scale);
extern void histsums (int sums [], int begin_limit, int end_limit);

/* gauss.h ends */



/* gauss.c begins */

#include "gauss.h"

/* Given n, throw an n-sided die. */
int
roll_die (int n) {
return uniform_int_in (1, n);
}


/* Roll the given number of dice and return their sum. */
int
sumdice (int ndice) {
int i;
int sum = 0;

for (i = 0; i < ndice; i++)
sum += roll_die (SIDES);

return sum;
}

/* For the specified number of dice throws: calculate the sum of the
given number of dice, and increment the array slot whose index is
the sum.
*/
void
fillsums (int sums [], int maxrolls) {
int i;

for (i = 0; i < maxrolls; i++)
++sums [sumdice (NDICE)];
}

/* Print the number of occurrences of the various sums. */
void
printsums (int sums [], int begin_limit, int end_limit) {
int i;

for (i = begin_limit; i < end_limit; i++)
printf ("%2d: %d\n", i, sums [i]);
}

/* Print a horizontal bar histogram for a given sum. */
void
histogram (int sum, int n, int scale) {
int i;

printf ("%2d: ", sum);
for (i = 0; i < n / scale; i++)
putchar ('=');
putchar ('\n');
}

/* Print a histogram of the occurrences. */
void
histsums (int sums [], int begin_limit, int end_limit) {
int i;

if ((NDICE > 0) && (SIDES > 1)) {
for (i = begin_limit; i < end_limit; i++)
histogram (i, sums [i], SCALE);
}
else {
if (NDICE < 1) {
fputs ("NDICE set to insane value\n", stderr);
fputs ("So, no histogram.\n", stderr);
}
if (SIDES < 2) {
fputs ("SIDES set to insane value\n", stderr);
fputs ("So, no histogram.\n", stderr);
}
}
}

/* gauss.c ends */



/* main.c begins */

#include "gauss.h"

int
main (void) {
int sums [LIMIT] = {0}; /* We use only indices
NDICE to LIMIT - 1
for indexing the sums of
the given of dice. We waste
sizeof (int) * NDICE
bytes.
*/

srand (time (NULL)); /* Seed the RNG using the
current time.
*/
fillsums (sums, ROLLS); /* Count the sums. */
printsums (sums, NDICE, LIMIT); /* Print how many of each sum
counted.
*/
putchar ('\n');
histsums (sums, NDICE, LIMIT); /* Print a histogram of
the same.
*/

exit (EXIT_SUCCESS); /* Done. So, exit. */
}

/* main.c ends */
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
What is code review? (Java code review) www Java 51 05-15-2007 01:10 PM
Code Review Request pitoniakm@msn.com Java 2 06-30-2005 02:26 PM
Request for Code Review Ben Hanson C++ 19 07-03-2004 11:27 PM
Request for code review P Kenter C++ 4 06-02-2004 05:26 PM
Re: Accessing Request.InputStream / Request.BinaryRead *as the request is occuring*: How??? Brian Birtle ASP .Net 2 10-16-2003 02:11 PM



Advertisments