views:

399

answers:

8

Suppose I've the following classes in my project:

  • class Is // validation class
  • class Math // number manipulation class

Now, if I want to validate a given number for primality where would be the logical place to insert my Prime() method? I can think of the following options:

  • Is_Math::Prime()
  • Math_Is::Prime()

I hate these ambiguities, the slow down my thinking process and often induce me in errors. Some more examples:

  • Is::Image() or Image::Is() ?
  • Is_Image::PNG() or Image_Is::PNG() ?
  • Is_i18n_US::ZipCode() or i18n_Is_US::ZipCode() or i18n_US_Is::ZipCode() ?

In the Image example the first choice makes more sense to me while in the i18n example I prefer the last one. Not having a standard makes me feel like the whole code base is messy.

Is there a holy grail solution for organizing classes? Maybe a different paradigm?

+10  A: 

For the Math example, I'd put the actual functionality of checking if a number is prime in the Math class. In your Is class you would put a method that would be called when a validation needs to occur. You would then use Math::Prime() from there.

With Image, that's a type check. You probably don't need to make a method for it unless you are making sure valid image data has been uploaded.

With the PNG method, same with Math. Put the actual PNG data checker algorithm in Image and make your validator method in Is call it.

The zip code example should be in your Is class only since it operates on a string primitive and probably will just use a regexp (read: it won't be a complex method, unlike your PNG checker which probably will be).

Marc W
+1 - very clear explanation, that should help him with future issues.
James Black
Actually it's basically the same of what I'm doing, but in the inverse way to it got me even more confused.
Alix Axel
I'm not sure I understand what you mean. Did my answer help you or confuse you?
Marc W
It actually got me more confused, since what you don't suggest any standard and your way of doing things (keeping complex methods in one place and more simple methods in other place) is just the inverse of what I do.
Alix Axel
It's not so much separating the complex from the simple methods as it is maintaing a separation of concerns. Why should a validator class know the intricacies of how to validate PNG image data? That should be a job for the `Image` object itself. The methods in `Is` only serve as an entry point for your validation service and should make use of the methods implemented in the object it validates to perform the actual validation.
Marc W
And what are your thoughts regarding Satanicpuppy answer bellow?
Alix Axel
I think he said basically the same thing I said but more generally. It's also a good answer. His answer seems to expand upon the examples I gave.
Marc W
A: 

Is there a holy grail solution for organizing classes? Maybe a different paradigm?

No, that is a basic flaw of class based oop. It's subjective.

Functional programming (Not to be confused with procedural programming) has less problems with this matter, mostly because the primary building blocks are much smaller. Classless oop also deals better, being a hybrid of oop and functional programming of sorts.

troelskn
I wonder what classless oop is... Maybe you care so show us an example?
Alix Axel
Javascript, Lua, various Scheme implementations - to name a few - provide a prototype based object system. In Ruby, classes are runtime entities that can be manipulated. These are examples of what I'd call a classless object system.
troelskn
+3  A: 

If you want to respect the SRP (http://en.wikipedia.org/wiki/Single_responsibility_principle), do the little exercice:

Select your class and try to describe what it does/can do. If you have an "AND" in your description, you must move the method to an other class.

See page 36: http://misko.hevery.com/attachments/Guide-Writing%20Testable%20Code.pdf

Other Law (there are many more) that will help you organize your classes: Law of Demeter (http://en.wikipedia.org/wiki/Law_of_Demeter).

To learn a lot and to help you make the right choice, I advice you Misko's blog (A google evangelist): http://misko.hevery.com

Hope this helps.

Toto
I guess I already do that... "the class Image performs several image manipulations AND several image validations", the thing I'm struggling with is where do I store the validations? In a "Image_Validation extends Image" class or in a "Validation_Image extends Validation" one?
Alix Axel
Never forget: you should only extend Abstract classes. Otherwise, you should use a "class injection" (See the Misko's bolg). Your question, translated, is about class Dependency (Again: Misko's blog is a reference). To be honest, it is impossible to answer your question without seeing your whole code, your constraints. Etc. If all your classes can pass the "NO AND" test and if you respect the Dependency Injection best practicies, your classes should be easy to refactor/reorganize in case of requierments changes. => The "dependency" architecture you have chosen is good. :)
Toto
+3  A: 

Everything about handling validation in itself would fit in your Is-classes:

  • Did it pass?
  • Which parts did not pass?
  • Should the validation errors be logged somewhere?

Zend_Validate in Zend Framework provides such an approach, maybe you can get some inspiration from it. Since this approach would have you implementing the same interface in all validation-classes, you could easily

  • use the same syntax for validation, independantly of which data is validated
  • easily recognize which validation rules you have available by checking for all classes named Is_Prime, Is_Image instead of checking for Math_Is, Image_Is all over the place.

Edit:
Why not use a syntax like this:

class Math {
    public function isPrime() {
        $validation_rule = new Is_Prime();
        return (bool) $validation_rule->validates($this->getValue());
    }
}

And thereby also allow

class Problem {
    public function solveProblem(Math $math) {
        $validation_rule = new Is_Prime();
        if($validation_rule->validates($math->getValue())) {
            return $this->handlePrime($math);
        } else {
            return $this->handleNonPrime($math);
        }
    }
}
chelmertz
+1  A: 

I think there is no "The Right Answer" to the problem you stated. Some people will put Prime in Is, and some in Math. There is ambiguity. Otherwise you wouldn't be asking this question.

Now, you have to resolve the ambiguity somehow. You can think about some rules and conventions, that would say which class/method goes where. But this may be fragile, as the rules are not always obvious, and they may become very complicated, and at that point they're no longer helpful.

I'd suggest that you design the classes so that it's obvious by looking at the names where some method should go.

Don't name your validation package Is. It's so general name that almost everything goes there. IsFile, IsImage, IsLocked, IsAvailable, IsFull - doesn't sound good, ok? There is no cohesion with that design.

It's probably better to make the validation component filter data at subsystems boundary (where you have to enforce security and business rules), nothing else.

After making that decision, your example becomes obvious. Prime belongs in Math. Is::Image is probably too general. I'd prefer Image::IsValid, because you'll probably also have other methods operating on an image (more cohesion). Otherwise "Is" becomes a bag for everything, as I said at the beginning.

phjr
I like your answer but I actually have something that goes like this: Is::Image($arg), Is::Number($arg) and then some other methods organized in the following way Is::Image::PNG($arg) and Is::Number::Float($arg) and Is::Number::Integer($arg) for the prime method I was thinking Is::Number::Integer::Prime($arg) I think there is cohesion here, am I wrong?
Alix Axel
@eyze I hope you agree that "Is" is a bag for everything here. If validation is *everything* you're doing, then it's ok. But then you shouldn't have any doubts about Math (there's no place for it). If validation isn't everything you're doing, then I'd suggest you to find a better architecture. Could you give an example how do you plan to use Is::Number::Integer::Prime?
phjr
+2  A: 

I don't think it's ambiguous at all. "Is" should be first in every one of those examples, and I'll tell you why: "Is" is the superset of validation operations in which Is::Math is a member.

In the case of Is::Math, what are you doing? Are you doing math operations? Or are you validating mathematical entities? The latter, obviously, otherwise it'd just be "Math".

Which of those two operations has the greater scope? Is? Or Math? Is, obviously, because Is is conceptually applicable to many non-Math entities, whereas Math is Math specific. (Likewise in the case of Math::Factor, it wouldn't be Factor::Math, because Math is the superset in which Factor belongs.)

The whole purpose of this type of OOPing is to group things in a manner that makes sense. Validation functions, even when they apply to wildly different types of entities (Prime numbers vs. PNG images) have more similarities to each other than they do to the things they are comparing. They will return the same types of data, they are called in the same kind of situations.

Satanicpuppy
Damn good answer, you nailed exactly my thinking process!
Alix Axel
+1  A: 

I don't think "is" belongs in class names at all. I think that's for methods.

abstract class Validator {}

class Math_Validator extends Validator
{
  public static function isPrime( $number )
  {
    // whatever
  }
}

class I18N_US_Validator extends Validator
{
  public static function isZipCode( $input )
  {
    // whatever
  }
}

class Image_Validator extends Validator
{
  public static function isPng( $path )
  {
    // whatever
  }
}

Math_Validator::isPrime( 1 );
I18N_US_Validator::isZipCode( '90210' );
Image_Validator::isPng( '/path/to/image.png' );
Peter Bailey
Why not class I18N_US_Validator extends I18N_US?
Alix Axel
Why? Well, I think Satanicpuppy said it best, "Validation functions, even when they apply to wildly different types of entities (Prime numbers vs. PNG images) have more similarities to each other than they do to the things they are comparing. They will return the same types of data, they are called in the same kind of situations."
Peter Bailey
A: 

Classes can be considered to be fancy types that do things, like validating themselves.

abstract class ValidatingType 
{
  protected $val;
  public function __construct($val)
  {
     if(!self::isValid($val))
     {  // complain, perhaps by throwing exception
        throw new Exception("No, you can't do that!");
     }
     $this->val = $val;

  }
  abstract static protected function isValid($val);
}

We extend ValidatingType to create a validating type. That obliges us to create an isValid method.

class ValidatingNumber extends ValidatingType
{
   ...
   static protected function isValid($val)
   {
      return is_numeric($val);
   }
}

class ValidatingPrimeNumber extends ValidatingNumber
{
   /*
    * If your PHP doesn't have late-binding statics, then don't make the abstract 
    * or overridden methods isValid() static.
    */
   static protected function isValid($val)
   {
      return parent::isValid($val) 
             or self::isPrime($val); // defined separately
   }
}

class ValidatingImage extends ValidatingType
{
   ...
   static protected function isValid($val)
   {
      // figure it out, return boolean
   }
}

One advantage of this approach is that you can continue to create new validating types, and you don't get a ballooning Is class.

There are more elegant variations on this approach. This is a simple variation. The syntax may require cleaning up.

Ewan Todd