[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: NSSavePanel complains about .hidden

From: Richard Frith-Macdonald
Subject: Re: NSSavePanel complains about .hidden
Date: Mon, 07 Oct 2002 11:49:06 +0100
User-agent: Mozilla/5.0 (X11; U; Linux ppc; en-US; rv:1.0.0) Gecko/20020622 Debian/1.0.0-0.woody.1

Richard Frith-Macdonald wrote:

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));
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.


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.

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

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.

I know warn logs are selectable at base library compile time ... I'd have no problem
with (I even like the idea) of having a flag to deselect them at runtime.
eg. --GNU-Debug=NoWarn to turn off warnings.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]