Arlodotexe / OwlCore.Storage

The most flexible file system abstraction, ever. Built in partnership with the UWP Community.
15 stars 4 forks source link

Use of exceptions #24

Closed HEIC-to-JPEG-Dev closed 1 year ago

HEIC-to-JPEG-Dev commented 1 year ago

Issue: Exceptions are used prolifically within the code base; as a simple example, the constructor for SystemFile.

Reason Exceptions should not be used where a known outcome is a possibility; they should be restricted to catastophic events and unexpected outcomes.

There is a massive performance penalty for using exceptions and additional code required for the developer. In file loops, this would bring an app to a halt.

Example for example: SystemFile x; try { x = new("C:\path") } catch (exception ex) { do something else} compared to: var x = new SystemFile("C:\path");

Proposal Replace exceptions with return values from methods that are more meaningful, for example: null, error code, or valid empty object.

Arlodotexe commented 1 year ago

We took cues from what System.IO and Windows.Storage when deciding when to use exceptions.

If calling File.Exists here creates an overhead problem we can measure, we can see about removing it. Otherwise, it's one-time parameter validation. See https://github.com/Arlodotexe/OwlCore.Storage/issues/26#issuecomment-1492316984

Generally, we use exceptions to indicate undesirable states caused by invalid data. We also use exceptions to indicate when a file/folder does not exist on methods where the intent of the method isn't "check if it exists" (same as .NET).

Can you offer up some examples on how you would do this, what this would look like to you?

HEIC-to-JPEG-Dev commented 1 year ago

In my libraries, I return null if something is invalid; I also have a common result error system to handle addiitonal data.

Arlodotexe commented 1 year ago

It's a recommended best practice to throw instead of returning an error code.

HEIC-to-JPEG-Dev commented 1 year ago

I think my confusion here was the assumption that the library would work with files/folders that change/don't exist.

From my understanding, the library only works with files that exist and do not change state. In that scenario, raising an exception would be correct as a file not existing is not an expected state.