Place of errors in a project (Cohesion, Single-Responsibility Principle)
Why are errors so special? And what's your API?
And yes, SRP doesn't stand for "does one and only one thing", SRP stands for "has only 1 reason for change".
I used to be a single-file-per-declaration guy… And in a case when I had a more than once class or structure per single entry of API, I tend to define multiple files for each possible declaration:
~/project/ImageReader/
ImageReaderError.swift
ImageReader.swift
Image.swift
...
And I do not see too many problems with that. I just combine logic into a single folder and keep the files together. So, once I need to change the API, I change some of those files. There is a way to improve it, but it's a bit later… The other case I frequently see is structuring the project by grouping declarations by their type. For example,
~/project/errors/
ImageReaderError.swift
CameraError.swift
...
~/project/
ImageReader.swift
Camera.swift
...
This is quite similar what one can see in the new web framework for cool kids people use where they combine controllers, errors, models separately. What's the matter with that? Low cohesion… I need to introduce a single change in some functionality the code lays in absolutely random places. Having large projects structured this way makes it hard for new people to discover the possible scenarios, and make API documentation harder.
When the code for image reading is in the same place, I can see what is going on and why, where having errors and/or models in a separate, predefined structure makes it hard to find. Of course, after a while of working on a project anybody can get used to it and stop seeing problem in that. And when new people come to project, listen! And listen hard! If they find it hard, it is hard, but you do not notice (or choose to ignore?).
Now, let's consider a better option (from my point, of course): I have the only file: ~project/ImageReader.swift which is self-contained and has necessary documentation (I'd like to avoid it as much as I can, or minimize):
struct ImageReader {
init(from directory: String) { ... }
struct Error: `Swift`.Error {
var message: String
}
func read(filename: String) throws -> Image { ... }
}
In my opinion, it is clear from the definition what kind of error the image reader can produce. Of course, it doesn't say if it can produce anything else and we probably can document it, but I would expect it to produce nothing but ImageReader.Error until something else is stated.
Now the logic which changes together stays in the same place. One might ask: Why you have only error message and do not have a bunch of error codes in the error definition? Well, nobody ever asked me to do so. When the API (the ImageReader) is project internal code and is not a part of some library, I do not expose any error which can be handled outside, until anybody needs to handle them.
Having separate code for each possible scenario is leading to the over-engineering. I know you think it should be perfect, I've been there. But it actually shouldn't. We are better to be ready for a change rather than defining perfect API which is never going to see the light. Additionally, we need to know how the API is going to be used (oh no, this is not perfect). Yes, it is not and it should not be. In the most of cases, we define an API and we (the project developers) are users of that API.
One of the possible examples might be: The image reader is used only for reading application asserts (resources) delivered inside of an application. In such a case, the error should never (theoretically) happen and we can't even test for it. But, I would still provide reasonable readable error with as much context as possible in case when it happens . Just because I do not wanna ask users to collect their logs and ask a bunch of questions in order to reproduce the error. I wanna be able to see the error message, add test, fix the bug, and provide value. I also wanna be friendly to my colleagues and provide them as much details as possible when they make a mistake, or type, or forget to add a file to the resources.
An alternative would be to have this:
struct ImageReader {
init(from directory: String) { ... }
// See `ImageReaderError.swift` for error details.
func read(filename: String) throws -> Image { ... }
}
Now, the code is less cohesive and I need to look in a separate file to understand the possible errors (or read the code of course). Modern IDEs can make it relatively easy task, but would it be more readable? Would it be easier to spot a type, mistake, some inconsistency when the errors are defined in a separate file? Would it make code-review easier/faster? Of course, I can always split the screen and open errors in a separate file and make my life somehow better. And in some cases, that what I have to do.
The errors are the part of the API and should be documented as well. Having errors makes it easier to review, refactor the code, spot mistakes and simply to self-document the code. Someone might see value in having all errors in a single place, but somehow such cases happen very rarely…
Extra note: When the class grows and you'd like to break it down into multiple files, but support the same API you always have multiple options:
Keep everything as it is and let the module grow
Break it down into multiple sub-classes (maybe even private): with their own errors
Break down only implementation, but keep external API the same and re-use the same error
The last one would require you to extract error into a separate file and make your internal implementations depend on it:
~/project/ImageReader/...
Error.swift
ImageReader.swift
InternalPNGReader.swift
InternalJPEGReader.swift
InternalOpenFile.swift
Each of Internal depends on the Error.swift and ImageReader.swift depends eon each of Internal*. This is of course a trade of and there is more than one way to solve that. And you're the one who knows the most!