StringExtensions.AddPrefix and StringExtensions.IsNullOrEmpty allow null pseudo-this argument
Closed this issue · 5 comments
Extension methods are designed to look like ordinary instance methods, and as such it is normal for them to throw an exception if their this argument is null. The various extensions in TraversalExtensions do this for example.
All the System.Linq.Enumerable extension methods perform a similar check. (They throw an ArgumentNullException whereas TraversalExtensions throws a NullReferenceException. The latter is more consistent with pretending that extension methods are ordinary methods, but ArgumentNullException seems to be the idiom used in CoreFx in practice.)
The AddPrefix and IsNullOrEmpty methods explicitly allow a null this. (In fact, that's the whole point of IsNullOrEmpty. It just defers to string.IsNullOrEmpty but you can invoke it as though it were a member of a string instance.) This feels like an abuse of the mechanism.
I wholly agree.
I also dislike the Format extension.
I think FormatWith predates string interpolation... so these extensions need to be reviewed as to whether they are still useful...
This is a great opportunity to delete everything that is poor / bad practice and replace usages as we go along
Yes, I was coming to FormatWith...
I think what's really missing here is any sort of clear project-wide definition of what all this is for.
I note that nothing in Endjin.IP uses any of the following:
FileStreamExtensionsHttpStatusCodeExtensionsIntegerExtensionsListExtensionsUriExtensions
And that overstates how much of this is in use, because within the other classes there are numerous unused methods.
I'm wondering if, rather than copying all the existing types from Endjin.Extensions.System into here, we should start from nothing and add only what's required by other Corvus or Menes projects that depend on this. (In fact, as far as I could tell, nothing else in Corvus depends on this anyway. Currently nothing in Menes appears to either, but I know that some of our existing OpenAPI code does depend on Endjin.Extensions.System, which I'm guessing is the reason this is in here?)
That way we'd be cherry picking features we know we're actually using. And it would also mean we'd have an opportunity to review just a handful of features to begin with (and modify them if we think better design options exist), rather than trying to do the whole lot at once.
👍