-2

Very strange problem. I've been testing my little command line program which reads a file and parses through it and it is working fine with my test data but when I went after the real thing it couldn't find the file.

The original file was a text file put together by text edit and the actual file was saved from Microsoft Word in MS-Dos format. When I tried to read the MS Word file, it couldn't find it. I did not get an error but did get a nil string back from the file loading method. I then renamed my test file to the same name and it got the original test data. Huh? At worst, I figured I'd see some sort of odd looking data loaded into my string... not nil.

Here is a stylized portion of code snippet. Please ignore the 'catch and release' code around the Datafile NSString... I realize I don't need to do it in that way and that is not the point of the question.

datafilename is set to 'config1.txt'. 

(NSString*) OpenEntryFile: (NSString*) pathname withdatafilename: (NSString*) datafilename {    

NSStringEncoding encoding;
NSError* error = nil;
NSString* inputdatafile;
NSString* response;
NSString *homeDir    = NSHomeDirectory();

NSString *fullPath = [homeDir stringByAppendingPathComponent:datafilename];

filepointer = 0;
[Datafile release];
inputdatafile = [NSString stringWithContentsOfFile: fullPath usedEncoding:&encoding error:&error];

Datafile = [inputdatafile copy];

response = [NSMutableString stringWithString: @"OK"];

if (error) {response = [NSMutableString stringWithString: @"ERROR"];};

if ([Datafile length] < 60) {response = [NSMutableString stringWithString: @"SHORT"];};

return response;
}
Eimantas
  • 48,927
  • 17
  • 132
  • 168
joseph ruth
  • 219
  • 2
  • 12

1 Answers1

28

This code has a number of issues;

  • Datafile should be dataFile

  • the if(error) is wrong; only by checking the return value can you know if an error was generated.

  • there is no need to use NSMutableString for your responses. Just use the constant string directly.

  • there is no need to copy the data as read with stringWithContentsOfFile:; just retain the resulting string (if you get one).

  • if you are getting nil for inputdatafile, then an error would have been generated. I.e. the returned string cannot be nil without error containing a description of the problem.

i.e. this will always output either a string or an error:

if (inputdatafile)
    NSLog(@"%@", inputdatafile);
else
    NSLog(@"error %@", error);

From a comment:

 if ([error code]) {response = [NSMutableString stringWithString: @"ERROR"];}

This is completely wrong.

The rules are that you must check the return value prior to checking the error. Always and without exception.

To do otherwise will yield crashes and other erroneous behavior.

Quinn Taylor
  • 44,553
  • 16
  • 113
  • 131
bbum
  • 162,346
  • 23
  • 271
  • 359
  • This is a code snippet in a larger piece of code so most of what you've pointed out isn't terribly important. However, the comment about the use of the error object was pretty much it. If there is no error, the error object shows value of 0. If there is an error, the error object shows with a value. I don't think I'm using it in the if statement correctly. Would it be cleaner to just look at the error object itself somehow to avoid the 'if nil then skip else do a bunch of stuff' awkwardness? – joseph ruth Apr 04 '11 at 03:24
  • 7
    While I agree that some are relatively minor, every single one of those problems I pointed out are very real and indicative of a lack of understanding of how the system works (or, at least, a lack of consistency with typical patterns). You cannot look directly at the error object. You must check the return value and then deal with the error *only if the return value was nil/NO*. – bbum Apr 04 '11 at 03:32
  • I replaced my if with this.... if ([error code]) {response = [NSMutableString stringWithString: @"ERROR"];}; as well as moved some stuff around and it worked better as expected. I mean why would you check something that has nothing to do with the error (the output of the method) to see if you have an error? Doesn't that just seem wrong to you? It's like checking the tailpipe for emissions to see if the engine is not running. – joseph ruth Apr 04 '11 at 03:34
  • Though I do thank you for your clue that my usage of the error object was totally messed up. – joseph ruth Apr 04 '11 at 03:41
  • 5
    @Joe Ruth: take the knowledge that has been given to you by bbum, seriously. He is not giving you a "clue" to a puzzle. He is sharing his knowledge and, in doing so, providing you a path to becoming a better Cocoa developer. – joshpaul Apr 04 '11 at 04:11
  • So why does my code work? If it won't, why won't it? If you got an answer, I'm all ears. The only answer I'm getting out of the manual is that Apple's code doesn't always return an error code when things go wrong but instead they take the lazy way out and just return a null object pointer. If that's how it works, that's how it works but it is pretty dopey. Don't you see that? NO other system works that way. – joseph ruth Apr 05 '11 at 04:08
  • 12
    Code that works may work merely because of symptoms. You read the docs wrong; the return value indicates success/failure. The error describes why. If you pass NULL for the error parameter, the system doesn't go to the expense of creating an error. If you *do* pass an error parameter, the system does *not* guarantee that it will preserve whatever value you assigned to it (your `error=nil` is pointless). It matters not what your opinion of it is, that is the way it works. You might find it "the lazy way out", but -- in that -- you are also mistaken. – bbum Apr 05 '11 at 07:44
  • 2
    I.e. your code, as written, works only by coincidence, not by the rules of the system. (And, btw, I'm curious why you are creating `NSMutableStrings` everywhere -- guaranteed to be less efficient than using constant strings?) – bbum Apr 05 '11 at 07:47
  • Again, when you don't have an answer, you just try to shout someone down. Sure mark of a scoundrel. Here is my take. The run time message processor only has three basic inputs, method name, parameters, and selectors. Nothing here about error structure or return code. So, if you pass bogus crap into the LINK EDITOR/LOADER which is what they it, there is no 'fixed' place to pass a return code back into. The error structure, if it even exists, is buried in amongst the parameter data and is never in the same place. So, the ONLY real recourse is to send back a nil objects. – joseph ruth Apr 06 '11 at 03:49
  • Maybe YOU'RE the one who should be a bit more curious. And I've been hearing that efficiency nonsense for about 35 years and again it is usually the sign of someone trying to pull a fast one. Generally, all this 'efficiency' talk makes absolutely no different in wall times unless you're talking about millions of records, which we are not. Besides, how do you know how I am using this data elsewhere in the program or in a different method? – joseph ruth Apr 06 '11 at 03:56
  • 12
    It doesn't matter how you are using the data. You are calling a system API and, if you don't follow the documented and designed patterns of that API, your code will only behave correctly by coincidence and, potentially, only behave as you expect until the next software update (or the one after that). This is a statement of fact, not a personal opinion. – bbum Apr 06 '11 at 05:00
  • 2
    @Joe Just wanted to note that @bbum's bit about efficiency has nothing to do with your `NSError` confusion, but rather with your confused use of `NSMutableString`. Since you're not actually appending or otherwise mutating your `NSMutableString` objects in any way, there's no point in using them! Cocoa mutability semantics have nothing to do with reassignment (which is what you are doing). Hence, `NSString` will work just fine without changing anything else. This isn't an example of someone pulling *the old **efficiency** ploy* on you. This is just common sense. – Jonathan Sterling Apr 06 '11 at 05:05
  • 4
    @Jonathan That, yes, absolutely true; `NSString` is stunningly efficient in comparison to mutable strings. And, yes, passing NULL to `NSError**` parameters can be a significant CPU/memory cycle savings in some esoteric, but useful, places. – bbum Apr 06 '11 at 05:14
  • 8
    I want to call special attention to something @bbum said earlier: “[T]he return value indicates success/failure. The error describes why.” To look at the error before you look at the method's result is to ask why it failed before you know it failed. Until you know it failed (method returned `NO`/`nil`), the answer to why it failed is not guaranteed to be meaningful. So, don't bother looking at the reason for failure until you have determined that the method failed. – Peter Hosey Apr 06 '11 at 05:30
  • 1
    Here is a link to an answer relating my experience with this exact problem – http://stackoverflow.com/questions/2069039/error-handling-with-nsurlconnection-sendsynchronousrequest/2511161#2511161 – ohhorob Apr 06 '11 at 05:38
  • 7
    @Joe Ruth: as a word of advice, I'd go do some googling about who @bbum is, who he works for, and what he works on. Then think about telling him that he should be more curious or speculating to him on how the runtime works. :) – Dave DeLong Apr 06 '11 at 18:25
  • So my explanation for the reason for this clumsy kludge gets no rebuttal? – joseph ruth Apr 06 '11 at 23:57
  • Oh, and let me make it clear that I am not doubting the necessity and rightness of your answer. Just if the architecture that MAKES it necessary and right. – joseph ruth Apr 07 '11 at 00:21
  • 6
    The return value is always the primary focus of the method (a design decision). The use of NSError removes all ambiguity; there is *never* a case where there is both no result and no error. This makes the coding & testing matrix that much simpler in that there is ONE and ONLY ONE execution path that yields a valid result or no result; the error is *entirely metadata*. And, yes, orthogonally, there have ben very real and measurable performance problems that were addressed by passing NULL for the error and only checking PASS/FAIL. – bbum Apr 07 '11 at 01:46
  • I have to disagree slightly with @bbum's comment "your `error=nil` is pointless," though the rest of his suggestions are spot-on. While this may hold true if you're dealing only with Apple frameworks, other code may attempt to copy this pattern, but not get it completely right. I propose it's safer to always initialize your error pointer to nil, at least for production code. That way your shipping app won't die in a fiery crash due to a simple `NSLog("error %@", error)` if the method you called failed to set an error. Yes, I've been burned by this exact bug using third-party frameworks. – wdn Apr 07 '11 at 15:45
  • 1
    There is *no guarantee* that the frameworks will preserve the contents of the `error` pointer in the success case. Thus, *any test of `error` without testing the return value may unavoidably crash*. I.e. from the caller's perspective, the value of `error` is undefined on return. From the callee's perspective, the `*error`'s value is undefined. – bbum Apr 07 '11 at 17:50
  • @bbum I don't disagree with you there. I'm just arguing, from a defensive programming point of view, that it doesn't hurt to initialize `error` to _nil_ in case the method returns an error value but fails to set the `error` object. I've run into this with third-party frameworks. You can argue that you should leave it uninitialized to catch these sorts of errors during development, much like the _nil_ in `dealloc` debate, but I'm talking about improving the quality of shipping code for your customers. – wdn Apr 07 '11 at 19:05
  • 2
    That is a mighty broken framework you are working with. Frankly, if it were my app, I'd do everything in my power to either fix or remove said framework. If the framework has screwed up something so basic as that, I would have to suspect the rest of it is useless, too. I get where you are coming from, but it seems like a futile battle in the face of such poor quality. – bbum Apr 07 '11 at 20:17
  • Well, it is what it is, as they say. Virtually every other computer framework in existence does not work this way. For example, you ask Oracle for the results of a SQL and you get a return code. This one does not work that way and that is that. I do have a question for you however. Why does a method returning a (void) return a basically random number and not a nil, which would seem to make more sense? – joseph ruth Apr 08 '11 at 02:10
  • 3
    Because a method returning `void` returns nothing at all; it is entirely undefined. If your compiler isn't complaining mightily when you try to look at that return value, you are doing it wrong (unless you are doing something highly out of the ordinary). Why anything be returned when nothing is more meaningful? Return values also force synchrony where none may be needed. – bbum Apr 08 '11 at 04:27
  • 4
    *This one does not work that way and that is that.* And for very good reasons. You may not agree with them, but dismissing them as a kludge or the result of a lack of careful consideration is a mistake. – bbum Apr 08 '11 at 04:30
  • 2
    @Joe Ruth "Virtually every other computer framework in existence does not work this way" So? Cocoa *does* work this way, and wanting it to work differently isn't going to change that. If you really want it to change, file a bug report and hope that the framework engineers at Apple decide to take your suggestion and overhaul all of the frameworks. (Hint: they won't) – Dave DeLong Apr 08 '11 at 04:57
  • 3
    @Joe Ruth "Virtually every other computer framework in existence does not work this way" At least the Windows API works this way: calling GetLastError() before making sure that the last procedure call actually failed may return basically anything. Some APIs set the last error, some don't. There's no guarantee that the value is sensible if the last call didn't actually fail. – jfortmann Apr 08 '11 at 08:29