I know I'm doing validation wrong. Please persuade me to stop :)

Question

First let me explain how I currently handle validation, say, for an IPv4 address:

public struct IPv4Address {
    private string value;

    private IPv4Address(string value) {
        this.value = value;
    }

    private static IPv4Address CheckSyntax(string value) {
        // If everything's fine...
        return new IPv4Address(value);

        // If something's wrong with the syntax...
        throw new ApplicationException("message");
    }

    public static implicit operator IPv4Address(string value) {
        return CheckSyntax(value);
    }

    public static implicit operator string(IPv4Address address) {
        return address.value;
    }
}

I have a bunch of structs like this one.

They often have additional private members to handle things but not methods are exposed publicly.

Here is a sample usage:

IPv4Address a;
IPv4Address b = "1.2.3.4";
a = b;
b = "5.6.7.8";

string address = a;

// c contains "1.2.3.4"
IPv4Address c = address;

// See if it's valid
try {
    IPv4Address d = "111.222.333.444";
}
catch (ApplicationException e) {
    // Handle the exception...
}

I can feel that there's something very disturbing with this, hence I'm pondering about switching to a static class with methods like IsIPv4Address and so on.

Now, here's what I think is wrong with the approach above:

• New team members will have to wrap their heads around this

• It might impede integration with 3rd party code

• Exceptions are expensive

• Never seen anything like this and I am a conservative type at heart :)

And then what I like about it:

• Very close to having a lot of specialized primitives since you have value types.

• In practice they can be often used like primitive types would, for example it isn't a problem passing the struct above to a method that accepts a string.

• And, my favourite, you can pass these structs between objects and be sure that they contain a syntactically valid value. This also avoids having to always check for correctness, which can be expensive if done unnecessarily and even forgotten.

I can't find a fatal flaw to the approach above (just a beginner here), what do you think?

Edit: as you can infer from the first line this is only an example, I'm not asking for a way to validate an IP address.

Answer 1

I also very much like the concept of mimicking the primitive patterns in the framework, so I would follow that through and lean toward IpV4Address.Parse("1.2.3.4") (along with TryParse) rather than implicit cast.

Answer 2

Seems like a whole lot of complexity with no benefit. Just because you can doesn't mean you should.

Instead of a CheckSyntax(string value) that returns a IP (This method is poorly worded btw) I would just have something like a

bool IsIP(string)

Then you could put this in a utility class, or a base class, or a separate abstraction someplace.

Answer 3

First of all, you should read posts about implicit casting, and when to use it (and why it's bad to use it in your scenario), you can start here.
If you need to have checking methods, they should be public static, instead of such strange constructs, beside this, having such methods allows you to choose if you want to throw exceptions (like .Parse() methods do), or signal by returning some value that should be checked (like .TryParse() methods).
Beside this, having static methods for creating valid objects doesn't mean you can't use value type (struct) instead of a class, if you really want to. Also, remember that structs have implicit empty constructor, that you can't "hide", so even your construct can be used like this:

IPv4Address a = new IPv4Address();

which will give you invalid struct (value will be null).

Answer 4

From the MSDN topic on implicit conversions

The pre-defined implicit conversions always succeed and never cause exceptions to be thrown. Properly designed user-defined implicit conversions should exhibit these characteristics as well

Microsoft itself has not always followed this recommendation. The following use of the implicit operator of System.Xml.Linq.XName throws an XmlException:

XName xname = ":";

Maybe the designers of System.Xml.Linq got away with it by not documenting the exception :-)