2
\$\begingroup\$

Here is a shortest possible syntax for replacing whitespaces with null and throwing an exception where it is not allowed:

public class Name
{
    string _first;
    public string First
    {
        get => _first;
        set => _first = (Optional)value;
    }

    string _middle;
    public string Middle
    {
        get => _middle;
        set => _middle = (Required)value;
    }
}

Where:

public class Optional
{
    public static explicit operator Optional(string text) => new Optional(text);
    public static implicit operator string(Optional optional) => optional.Text;
    Optional(string text) => Text = IsNullOrWhiteSpace(text) ? null : text;
    string Text { get; }
}

And:

public class Required
{
    public static explicit operator Required(string text) => new Required((Optional)text);
    public static implicit operator string(Required required) => required.Text;
    Required(string text) => Text = text ?? throw new FormatException();
    string Text { get; }
}

P.S. Value modification on property assignment is not recommended, but could be very handy sometimes :)

\$\endgroup\$
  • 3
    \$\begingroup\$ I hope you won't take this the wrong way, but the whole "reasoning" part doesn't bring anything to the post and should be removed. If you want to explain your reasoning, try to stick to the point, StackExchange isn't a blog. \$\endgroup\$ – IEatBagels Sep 23 at 19:47
  • 2
    \$\begingroup\$ I wanted to edit my comment but can't : Explaining your reasoning is fine (great, even), but there's just too much "political" fluff around it. \$\endgroup\$ – IEatBagels Sep 23 at 19:54
  • 1
    \$\begingroup\$ This question is being discussed in meta \$\endgroup\$ – IEatBagels Sep 24 at 12:50
  • 1
    \$\begingroup\$ The "Question" area is for the question, not for responding to answers. \$\endgroup\$ – Peter Taylor Sep 26 at 10:29
6
\$\begingroup\$

Usability

I would have expected Optional to transform null and whitespace only content into String.Empty, not the around way around.

 Optional(string text) => Text = IsNullOrWhiteSpace(text) ? string.Empty : text;

And Required to throw on null or whitespace only content.

 Required(string text) => Text = !IsNullOrWhiteSpace(text) ? text : throw new FormatException();

This way, the pattern focuses on actual string content, rather than on a technical null vs empty debate.

\$\endgroup\$
9
\$\begingroup\$

Not sure that adding a 7-line validator class per validation rule (plus typecasting abuse) is any shorter/better than a set of more straightforward composable validating one-liners, something like:

public static class ValidatingTransformers
{
    public static T ThrowsIfNull<T>(this T s, string msg = "") =>
        s != null ? s : throw new ArgumentException(msg);
    public static string ThrowsIfEmpty(this string s, string msg = "") =>
        !string.IsNullOrWhiteSpace(s) ? s : throw new ArgumentException(msg);

    public static string EmptyIfNull(this string s) => s ?? "";
    public static string NullIfEmpty(this string s) => !string.IsNullOrWhiteSpace(s) ? s : null;

    ... et cetera
}

Some other thoughts:

  1. Replacing empty/whitespace strings with null does not look like a great idea to me. Because, well, the null and the NullReferenceException. An empty string looks like a safer alternative.
  2. I'd avoid property setters and mutating a state, even if setters protect invariants. Much simpler, shorter, and safer alternative:
public class Name
{
    public string First { get; }
    public string Middle { get; }
    public string Last { get; }

    public Name(string first, string middle, string last)
    {
        First  = first.NullIfEmpty();
        Middle = middle.ThrowsIfNull("middle name must not be null");
        Last   = last.ThrowsIfEmpty("last name must not be empty");
    }
}

Shorter code, one single place for all validations, never ever get an object in an inconsistent state, no issues with concurrent mutations.

\$\endgroup\$
  • \$\begingroup\$ +1, yes - I used extension methods before. The problem is - invoking a method on the null reference makes my feeling scream a way more than a type cast. Would it still be a typecasting abuse for you if relevant model names be used, like (Text)str or (TextOrNull)str for the same purposes? \$\endgroup\$ – Dmitry Nogin Sep 23 at 3:55
  • \$\begingroup\$ Type cast exception will look reasonable then for (Text) as no white spaces are allowed for the type. \$\endgroup\$ – Dmitry Nogin Sep 23 at 3:59
  • 1
    \$\begingroup\$ Typecasting like _first = (Optional)value;` suggests that _first has a type of Optional, otherwise why would anyone do the cast? It is not obvious that the only reason for the cast is validation via some intermediate type. But even if we accept this semantic, it still needs a lot of boilerplate code comparing to static functions. \$\endgroup\$ – Bronx Sep 23 at 4:21
8
\$\begingroup\$

Edit: the tone of this might seem a bit negative, so I'll add that this was a fun one to review in hopes of lightening the mood ;)

Miscellaneous thoughts:

  • This is horrifying! Why did you do this?!

  • You can just about get away with this for properties, but for method calls with required/optional fields, a FormatException will be inadequate (an ArgumentException telling the caller which argument went wrong is important), which rather limits the scope of this technique.

  • Because the thing is type specific, you could give it a sensible name (e.g. OptionalString), make both conversions implicit, and use it as the field type. This would required even less syntax, and would be somewhat less opaque to the maintainer who has to live with this.

  • These need documentation: it's not obvious that an Optional should convert "" to null: this is enough to put me off it completely.

  • You can get this 'low-char-count syntax by using a static using and providing these as static methods to a static class. The advantages would include:

    • It's not horrifying
    • It requires less code, because you don't have to build a whole type and its conversions for each sort of string
    • You can't misuse the types in ways you didn't intend, and they don't appear everywhere
    • You can throw the static methods at a delegate if you have need
    • It's not horrifying
    • People who don't hate themselves can use the fully qualified name
    • It won't incur an allocation every time you assign a value (though using structs would address this already)
    • You have greater freedom with the type of checks you can perform (e.g. multiple parameters)
    • It will be easier to document, because the API will be single method, not a type+stuff
    • It's not horrifying
  • It's good that the conversion to the types is an explicit one, as this limits the scope for things going wrong with the intended usage.

  • It would be nice if the Text members were explicitly private, so there was no question as to your intentions.
  • I personally do not like the expression-bodied syntax for constructors, but that's probably just me.
\$\endgroup\$
  • 2
    \$\begingroup\$ Throwing in a regular constructor is totally valid -- this is how you signal that you cannot create a valid instance using given constructor parameters. Violation of CA1065 is throwing in a static constructor only. \$\endgroup\$ – Bronx Sep 22 at 22:42
  • \$\begingroup\$ @Bronx wow, thanks for calling that out. I spent some time wondering how to phrase that bit because I couldn't think of a good reason other than to make the tooling happen, so I'm glad to remove it ;) \$\endgroup\$ – VisualMelon Sep 22 at 22:49
  • \$\begingroup\$ @VisualMelon please see the reasoning update above :) \$\endgroup\$ – Dmitry Nogin Sep 23 at 0:34
  • \$\begingroup\$ @DmitryNogin I hope this answer didn't come across too seriously ;) It's an interesting idea, but I don't buy the argument that we keep things simple and predictable for the sake of inexperienced programmers: we keep them simple and predictable for the sake of tired and hungry programmers who don't want to spent 20 seconds understanding these conversion classes (and forget what they were actually trying to debug) when everyone natively speaks the language of functions. \$\endgroup\$ – VisualMelon Sep 23 at 8:33
  • \$\begingroup\$ @DmitryNogin I completely agree that Optional and Required can be types in their own right, but then should be exposed in the public API, rather than used for conversions which are hidden inside properties. I'm not sure what the bit about not mentioning C# experience is about... but I guess I'll be unemployable then ;) \$\endgroup\$ – VisualMelon Sep 23 at 8:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.