Closed kuskmen closed 5 years ago
Hi,
Thank you for your interest in Optional.
I appreciate the effort, but I am afraid I will not merge this PR. The least I can do, however, is to provide a bit of reasoning and feedback.
I actually initially started out having something quite similar (in Optional.Collections, take a look here for an example), but decided to abandon it as I found little benefit in the extra layer of abstraction.
As you can see, there is small difference between the implementation I used and the one you have submitted - in my version, the argument name needed to be passed explicitly (also, this was before nameof(...)
), which is not the case in your implementation:
Guard.ArgumentNotNull(source);
This makes your version less verbose, but unfortunately it doesn't quite work. Let us take a closer look at the actual implementation:
public static void ArgumentNotNull(object argument)
{
if (argument == null)
{
throw new ArgumentNullException(nameof(argument));
}
}
The null check itself would work as expected, but the argument name would always be written out as argument
and not the actual argument at the relevant call site.
This is due to the way the nameof()
operator works - it basically writes out what is inside the parentheses verbatim (not completely true, but it is a decent first approximation). So nameof(something)
results in the string "something"
. If you think about, it makes quite good sense, as the object itself might have been passed through several method calls before reaching the nameof()
operator - and each time it is passed to a new method the argument name could be different, so there is no reasonable way to resolve which name should be used.
This means, that an actual implementation would still need to be called like:
Guard.ArgumentNotNull(source, nameof(source));
Which isn't really that much of an improvement over the direct null check, which is the main reason I decided to just keep things simple for now and check directly.
Finally, I wouldn't really want to add a separate DLL into the mix, just for this functionality. If I at some point decide that it would be more relevant, it should preferably just stay as an internal class inside the main project itself.
I hope it makes sense - otherwise feel free to ask any questions you might have :)
Also, you are of course very welcome to bring such changes up for discussion here before actually implementing them, if you want to be sure that your work is not in vain.
Best regards, /Nils
Hi,
thanks for the feedback. Now that you've said it, I do see that I made a mistake about nameof
operator (why didn't I thought about it earlier, doh!). However, this has nice fix as you said.
About the functionality itself, I noticed that more and more popular public libraries do this in their code and to me it makes sense to kinda throw exceptions centralized.Why? Because someday you might want to make your own api exception for specific case and changing that in the code might be cumbersome, this imho but I guess we are going to the point of YAGNI so its double edged knife, however I am still junior and most certainly I am missing something(again?). Bottom line it was not intended to make the code thousand times better (after all it just one extracted method, amazing), the bigger idea was the centralized throwing, but I see your point and it also makes sense to me.
This however, will not stop me from contributing to the project. I was searching for interesting open source project to participate in for so long and now that I found it I am so eager to help.
I don't mind doing work that might turn out to be useless afterwards, after all feedback like this keeps me moving, so don't feel sorry for me.
Best regards, kuskmen
Closing this for now, based on previous feedback. Feel free to open an issue for further discussion :)
This is intended to reduce the boilerplate code that the project is and will introduce in future.
It is simple class that will be used for adding so called guards (name borrowed from Haskell). Right now only null reference guard is implemented, but many may follow.
Guard functionality is placed in new project (Optional.Internals), but should not be intended for packaging hence the name "Internals".