On Wed, 20 Jan 2010 10:24:44 -0700
Daniel Dilts <[email protected]> wrote:
> I've started playing around with C and I'm wondering if anybody can
> tell me how to improve my code. This function just iterates over a
> directory. It is a really simple function, but the error handling
> takes up so much space that it is difficult to follow the flow of the
> code.
This is a fundamental problem with C. If you really want to handle
every error condition and do something specific for each one, you really
don't have any choice other than to just do it. Newer languages provide
constructs such as try/catch/throw for error handling that make the
code a little easier to read (if not easier to understand).
In general, C programmers reserve error handling for cases where they
can actually do something about the error. For example, it makes sense
to detect an error when you try to open the directory and it fails, but
if you have an error when closing the directory, what exactly are you
going to do about it? Tell the user "well, I was able to successfully
open and read the directory for you, but since I had an error when
trying to close it, I'm just going to report the error and not tell you
anything that I found in the directory?" Save your error handling for
when it makes a difference.
Oh yeah, telling the user "I couldn't read the directory" is one
thing... telling them "I couldn't read directory X" is better.
I also see you going through a lot of machinations to preserve the
value of errno (presumably so you can return it at the end of the
function). You probably want to do that before you call perror, since
perror might set errno, too. Are you planning to do something with the
returned error code in the calling routine? If not, why go through the
work to preserve it? You could just as easily return a generic error
from this function (-1, maybe?) since you've already reported the error
to the user via the perror function.
I did see some oddities in your code, though, unrelated to the error
handling... for example, you pass "pad" in, but then immediately throw
it away and assign NULL to it. Later, you allocate a new pad (whatever
that is... some windows construct maybe?), but you don't really do
anything with it. I suspect you want to be using "*pad" in the
function so you can return the value to the calling routine (you
probably should make sure that pad has a rational value first before
you try stuffing something into it, though... wouldn't want to get a
segmentation violation because pad was NULL and you tried to assign
to *pad).
Second, it looks like your first loop through the directory is just
trying to figure out how many items there are and how wide the longest
one is (presumably so you can pass some dimensions into newpad?). Your
second time through the directory, you're not doing anything... I'm
assuming you want to add elements to your pad in some way. I realize
that this code is just an incomplete example, but when you're asking for
help you might want to stick a comment in there saying "here's where I
will add the directory elements to the pad so I can show them to the
user."
And indent, please. I had to paste your code into my editor and make
it re-indent the buffer so I could tell what was going on. I don't
really care what your indentation style is as long as it isn't "don't
bother to indent." :-)
-jan-
--
Jan L. Peterson
http://www.peterson-tech.com/~jlp/
--------------------
BYU Unix Users Group
http://uug.byu.edu/
The opinions expressed in this message are the responsibility of their
author. They are not endorsed by BYU, the BYU CS Department or BYU-UUG.
___________________________________________________________________
List Info (unsubscribe here): http://uug.byu.edu/mailman/listinfo/uug-list
