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?

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.

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.