Nicola Pero wrote:
Ok - it looks silly to discuss too much about this anyway (with all the
pending stuff) but I can't avoid another one on this :-)
I don't understand why you are assuming that +stringWithContentsOfFile
returning nil is an error. The caller will assess that. In the case of
NSSavePanel, for example, it's a perfectly valid condition.
<pedantic>Technically a nil return here indicates a failure ... an
error. I'ts up to the caller to decide how to deal with that, not to
decide whether it's a failure or not.</pedantic>
<pedantic>An 'error', for the programmer using the base library, is when
the base library generates an exception. Anything else is not an
error. A nil return here indicates that the file doesn't exist, is
not readable
or can't be read. This is considered a valid result, otherwise the API
would have required an exception - with the name and reason of the
error -
to be raised.</pedantic>
Sorry ... but the above is simply not the case. Exceptions are not the
only way of denoting
an error condition and the standard mechanism of indicating an error
during object
construction is to return nil. As a general rule, constructors are
supposed to avoid
raising exceptions, and should document cases where they do since
raising an exception
is not the normal behavior for them.
But I agree that it's up to the caller to decide how to deal with a nil
result, which is why the library should not be logging but letting the
caller decide what to do. :-)
Yes - it's a very simple (but not very powerful) API ... the idea I
suppose is that if you want richer control, then you have to use a more
complex API.
+stringWithContentsOfFile: is a simple high-level wrapper around the
low-level API, which provides you with a simple and straightforward API
for reading a file if it's there, and ignore it if it's not or can't be
read. It's perfect for cases like the NSSavePanel, which need precisely
that. The fact that it returns nil is to be considered a *feature* of
the
library, and using it is *not* a programming error.
Returning a nil is a failure indicator ... sure you can consider it a
'feature', but
that's not really the point - the log is a warning - to indicate
something
that is likely to be a programing error. By 'likely' I mean that it's
a programming
error often enought to be worth logging ... if it was 'always' a
programming error
we would either be writing an NSLog() (no choice about it), or raising
an NSException.
An NSLog() would be used where we know it's an error but expect the
program to
be able to continue without intervention anyway, thoough we might also
use it
where we are fairly certain it's a error.
I've already said that elsewhere, but perhaps restating will help.
It's common practice
to log possible problems both as an aid to development and for
diagnostic purposes later.
Under Unix, NeXTstep (and now MacOS-X) we commonly log things to
stderr. In NeXT/MacOS/GNUstep gui applications, the naive user never
sees these logs ...
so they don't bother the user ...
but they do provide useful information for programmers. The reason for
not
logging *loads* of stuff is for performance, and so that more serious
problems
don't get lost amidst all the very routine stuff.
So the 'hierarchy' of logging is like this -
NSDebugLog() ... interesting circumstances, possibly errors, but not
likely to be of
use logging under normal conditions. Needs to be turned on to occur.
NSWarnLog() ... likely to be an error, almost always worth logging.
Needs to be turned off
if it's not to occur.
NSLog() ... probable error, always considered worth logging. can't be
turned off
NSException ... definite error requiring some intervention if the
program is to continue
running. Results in log and abort otherwise.
Error handling is a different issue, and is handled by two mechanisms
... return values
from functions and methods (IMO best used to denote errors which
should be dealt
with immediately in the code), and exceptions (best used to pass error
information
back up the stack to where the program can deal with them in a
different manner).
Good explanation - but then why the following code -
tmp = NSZoneMalloc(zone, fileLength);
if (tmp == 0)
{
NSWarnFLog(@"Malloc failed for file (%s) of length %d - %s",
thePath, fileLength, GSLastErrorStr(errno));
CloseHandle(fh);
return NO;
}
is this an error that the programmer has done when calling the method ?
No, that's just a mistake :-)
I think a memory allocation failure causes the program to abort
anyway, and
the log there would never get executed.
The
explanation of why you are using NSWarnLog is not very convincing.
You are checking the result of each operation you do, and producing a
NSWarnLog is any operation fails. But it's not the programmer's fault if
memory can't be allocated, or if a file was deleted by another process
just after the method was called (but before the file was opened), or
its
permissions changed in the same way, or if the file is too big or if an
fseek on the file fails (or if the NFS went down just after the method
started executing).
That's just because some of them should actually be NSlog() or
NSException
cases because they are more severe problems ... and I went through
quickly to
*improve* the situation, rather than spending time trying to perfect it.
provided
This stuff is not in the programmer's control, and not the programmers's
fault, and a different programming style would not necessarily remove
the
cause of the errors - so it should not be a NSWarnLog. Why are you
against it being a NSDebugLog ?
Because warn logging should be on by default ... and only (possibly)
turned off for
a 'production' system where you don't want end users to see any of
this if they start
poking around places where they are not expected.
Why do you want to turn *off* useful information?
By the way, it seems you want to always have the calling code separately
check if the the file is there (and that it's not a dir), then check if
the file is readable by you, and only then read it using
+stringWithContentsOfFile:.
Well in the NSSavePanel I don't see how that would help or be a better
programming style ... it would require us to write a lot more code in
NSSavePanel to do exactly the same things - read the file if it's there
and can be read as a file, and ignore the matter in all other cases.
It would also be less efficient, since it would check twice that the
file
is readable (basically part of the +stringWithContentsOfFile: would need
to be duplicated in the caller, which *is* bad programming style and
cause
of bugs).
It lists the files in a directory, then removes certain names from the
list.
If the .hidden file is not in the list, it does not need to try
reading it.
Even if it couldn't reasonably work that way for some reason, I'd
advocate using -
if ([mgr isReadableFileAtPath: hidden] == YES)
// Read hidden file and do whatever it is we do.
Certainly there is no cause to duplicate any stringWithContentsOfFile:
code though.
But in any case, I'm not saying you should definitely not use
'features' of the API
in ways for which they might not have been intended ... just that you
should be
aware of warnings/logs if you do.
There are millions of reasons why a read from a file can fail without
any
programming error - no more memory, NFS server down, NFS network
problems,
file removed, permissions changed, cdrom/floppy unmounted, filesystem
error, harddisk failure, floppy failure, cdrom damaged ... any of these
can happen at any time; and conditions can very well change between the
moment that the programmer checks that the file is readable and the
moment
that he reads it. Not so uncommon.
Not *SO* uncommon ... but still all those conditions form a tiny
minority of
cases. The vast majority of cases in practice are where the programmer
has
provided the name of a file which does not exist or is not accessible for
chmod/chown reasons. And of that vast majority, a substantial minority
are cases where the wrong file name has been given ... genuine
programming
errors by many standard. It's therefore helpful to have a log for
these, and it's
a good thing for programmers to try to avoid them.
I can see that some people might want a NSDebugLog of what's
happening to
help in debugging weird problems. But it has nothing to do with
programming errors or programming practice or teaching people how to
improve/fix their code.
As I said ... ten years ago I'd probably have said that conflating the
two operations
of checking for a file and reading a file made sense for
convenience... since then
I've realised that I make more mistakes than I thought, and many
people make more
mistakes than me - So I now think that checking first usually makes
sense, and anything
that raises peoples awareness of this is probably a good thing.
and we ought to let the programmer know so they can improve/fix
their code.
The NSSavePanel code is correct, is using the documented API, exactly as
documented, and efficiently and elegantly - I see no reason to change
it.
I'm not going to dispute that ... much :-)
The code could be made more efficient by not reading the .hidden file
if it does not
exist ... I think whether you consider this more or less elegant is a
matter of taste.