LoadFile -- should it throw or return null?

Personally I would throw and exception in this situation.

I like knowing why something failed. StreamReader in C# has multiple reasons for a read faliure.

• OutOfMemoryException
• DirectoryNotFoundException
• DriveNotFoundException
• EndOfStreamException
• FileLoadException
• FileNotFoundException
• PathTooLongException
• PipeException

While some are interchangable and some could be handled by your wrapper, some can and should be shown to the user. In general I try to let the user know why my process failed and I assume you would want to know if you were using someone else's code.

Also I am somehow confused what you mean by failing gracefully, both your options consist in a graceful falure, a non graceful falling would mean your program just breaks. A graceful falure also usually contains an exception or a descriptive log stating the reason for the falure, not a "null" return.

Which I don't see how it could be helpful at all, it would require you to call the "FileExists" function and verify that "LoadFile" doesn't return null. The fact that the file exists in no way gives me the guarantee that the reading of the file reading was successful.

Personally I'd use exception rather than returning null. The reason is, I think returning null means the operation is success but it does not produce any output. Throwing exception usually means there's something wrong with that operation. As a plus, you can specify what's wrong with that operation by putting some information in that exception class.

However, throwing exception has performance impact and the error can be hideous if you're not careful enough. You might accidentally wrapping the function call with try...catch that do nothing when the exception is thrown (this is clearly a code smell though.).

What is the correct / expected result? LoadFile implies it has to succeed, thus you should throw exception. If you ever encounter a situation where you are fine with failure you could create TryLoadFile which returns null or bool (with out variable) to indicate success or failure.

This is heavily opinion based and also consistency based. Therefore I'd reply that either is fine - but...

If NULL in your code indicates that operation failed, and you don't use exceptions - return NULL. While some may disagree, I'd say that consistency is important. When using NULL be aware that you will ALWAYS need to handle it correctly when the function returns NULL.

If exceptions in your code indicate and error, throw and exception - because again you would break the consistency in the project.

In C# I generally tend to prefer to use exceptions for file operation errors.

...

In short: Either can be fine, but be sure to not mix NULL and exceptions to do the same thing in single project.

That depends on where this code lives, although I am not sure I would go for exceptions either way, but rather a variant.

If I am writing something low-level such as an engine, all possible contextual information is important to have upon failure. It is very important that the foundation be solid and all errors handled gracefully, so you will want to know exactly why every error occurs.
However I would generally never enable exceptions in video games, so the function would have to have some way of returning an enum explaining the failure.

High-level code should be easier to use. It would normally be fine here to just use a true/false type of check by default as long as there is still a way to get extended error information if requested. As an example:

/* Convert to C#. */
SomeObj * LoadFile( const wchar_t * _pcPat, uint32_t * _pui32Error = nullptr ); // Can provide _pui32Error if and only if you want extended error information.

But why is the function returning a pointer to anything? Why not make the file an object, such as CFile? Then you would have:

CFile::Load()
CFile::IsOpen() const
CFile::OpenFlags() const // Flags used to open the file.
CFile::FailReason() const // Or optional flags in Load() or whatever; somehow you can get extended error information.
CFile::Path() cons // Path to the file we opened.
etc.


L. Spiro

Very nice answers, thank you! You replies helped me clear this up, I'll be using exceptions.

The actual code I'm writing is a high-level wrapper for asset loading from multiple sources (directories, ZIP files, etc). "LoadFile" might have been misleading, because it (ideally) returns an asset, I just didn't want to complicate the question. Incorrect types (loading MP3 as texture) throw exceptions of course.

18 hours ago, Karzan said:

what you mean by failing gracefully

That's part of the question I guess I wasn't sure myself; it's something like a "simple" error (file does not exist) versus a "complicated" error (corrupt file). The replies helped me clear this up, in retrospect, it was an irrelevant distinction.

18 hours ago, Zaoshi Kaba said:

What is the correct / expected result?

An asset, which can be a number of things. Most importantly, quite a few things can go wrong with it: file does not exists, incorrect type, corrupt file, etc. The function name does imply it has to succeed, because I already have a "FileExist" function too.

16 hours ago, Vilem Otte said:

heavily [...] consistency based

This is very true. And if I throw for an invalid file type, I should throw for a non-existing file too.

Thanks again guys

Most C# developers would probably expect an exception to be thrown. Not only is this an exceptional case, but it's also how the standard library handles it. So when in Rome... stick with established best practices

Also, consider the fact that games usually load files that are necessary for proper functionality and expected to exist (assets, etc.), based on the assumption that the game was properly installed. There isn't much the software could do to recover and continue from missing assets, and as a developer I wouldn't want my game to gimp along with missing data anyway!

Exceptions are way overused these days in my opinion. Everybody throws exceptions as if they were candy, I evens aw a ThisIsAnExpectedFailureException so no. C# handles it this way so you can do it the same way but don't throw exceptions everywhere.

If we were in an C++ level, I would totally avoid exceptions and instead build my functions in a way that the return type indicates if something did go wrong or have at least some error/ state proeprties on my classes (my FileStream classes could be cast to bool to check if file is ready for read/write)

Is your missing file handling code available at the calling site? Or does it live much further up the stack?

If the former, return null, if the latter, throw. That is the only purpose of exceptions - to quickly unwind the stack many levels to a pre-registered branch target.

However, loading a file should be an asynchronous operation. LoadFile should return a handle that can be polled for completion (either success/fail) or, should take a delegate to be called upon completion. So, neither of your options ?