How would you improve this simple class to be more loosely coupled?

Question

As you can see below, in the constructor I'm instantiating a validation object so I can validate a user's email in a set method. Is this architecture best practice or flawed? Can I avoid making my User class directly dependent on my Validation class?

Class User {
  Private Email

//constructor
User() {
  Validation = new Validation
}

SetEmail(NewValue) {
  if (Validation.isEmail(NewValue)) {
    Email = NewValue
  }
}

And a related question: When a set method receives an invalid value, what is the proper response? I see 2 options

1. Don't set the value and return false
2. Set the value anyway, but set an error property for the object. (so if User.Error is set I know something went wrong)

I suspect #1 is best practice because you can then assure the value of any object property is always valid. Correct?

Fuss

Answer 1

The proposals so far all seem to be way overkill, especially with all the IOC and AOP stuff.

1. The User class needs an email address, so create an EmailAddress class and have the User class accept one via a property and/or its constructor. That validation can be as simple as whether the input EmailAddress reference is null or not.

2. The EmailAddress class can be a simple but generally reusable implementation (consider basing it on the RFC document). It should be immutable and should throw an exception from its constructor on invalid input.

3. Ideally, the EmailAddress class should be composed of an EmailUserId class (based on the RFC?) and a InternetDomain class (based on the RFC?), since an email address is a composite data structure. Again, each of those classes should manage immutable instances and should throw an exception on construction with invalid input.

"Validation" strikes me as not a "thing" but rather a generic "action". Therefore, it lends itself to being a method rather than a class. In this case, I tend to implement the validation in each of these classes as a private static method (valid(input)) that is invoked from the constructor, in languages like Java or C#. Often, it becomes useful to expose that functionality publicly in the form of a question (isValid(input)).

EDIT:

Are you suggesting that every distinct data type I need to validate should have it's own class?

That is one solid way of addressing the issue, commonly known as a Value Type (thanks for the reminder, Frank). The result will be a few (dozen or two) well-defined, reusable classes like perhaps EmailAddress, PhoneNumber, PersonName, etc. The presented alternative is likely to result in a "god class" with a mixture of functionality that is not reusable, not easy to test, and difficult to maintain.

There are other ways to partition the solution, but my suggestion does have the advantage of being mature, well understood, and consistent with a large set of solid design principles. I would certainly recommend trying it before attempting to invent your own.

Vinificator

That is to say EmailAddresses are Value Types. Excellent solution Rob. – Daredeviltry

Are you suggesting that every distinct data type I need to validate should have it's own class? So I'd need a name class, address class, city class, zip class, etc? I currently have a validation class with many methods (email, phone, etc). You suggest splitting each method into it's own class? – Fuss

Yes I believe that is what he is saying. It might seem wrong to make a class for something like zip code but it's just part of breaking things up to represent a single concept. – Vanillic

Answer 2

I would:

1. Break the coupling to the concrete Validation object via dependency injection: define an abstract (pure virtual) Validation class, make a concrete validation class derive from it, and pass in ("inject") a reference to the abstract Validation class in the User class's constructor.

   For an excellent discussion of how and why to do this in C++, see Robert Martin's 1996 article on the subject from The C++ Report.

2. Rather than returning false or silently setting some property, raise an exception. That's what they are there for.

Exo

I'm not sure I understand. Are you saying that User should extend and abstract Validation class? – Fructuous

No--separate the concrete Validation class into an interface (an abstract class) and an implementation (a concrete class), and make the User class depend only on the interface, not the implementation, of Validation. I don't do this for every class (a la J2EE), but decoupling is the #1 use case. – Exo

Okay, I understand now. I do that all the time, I just didn't know the name for it. – Fructuous