How much OO is too much?

[DougBTX]

Trimmed up (and working) example:

Looking good

For the PHP4 people, you can get a file_put_contents from PHP_Compat.

Douglas

[Captain Proton]

ability to vary persistance (i.e. choose to store in db instead of a file)

Just curious, why is this always so important? Not for this guestbook, but in general I mean.

[sweatje]

Just curious, why is this always so important? Not for this guestbook, but in general I mean.

No definative answer from me, but I would guess that it is high on the "scalability" checklist for the share-nothing architecture. If you implementation is dependant on a local file on the server then you can't expand easily by adding more web servers.

Next on the list would be compatability with current infrastructure. This would be why you would want to support multiple varities of db for a project. Is seems weird, but it is easier for me to justify using an open source project here at work if I can run it on top of oracle instead of mysql.

[bonefry]

Just curious, why is this always so important? Not for this guestbook, but in general I mean.

As @sweatje said the shared nothing architecture has a major advantage. It is very scalable horizontaly in most of the situations, i.e. you can share the load on multiple servers. But in order to do that, if the persistance is file-system based, the files should be transported from one server to another, because sessions may sometimes be moved around, which is very very bad. With a DB all the worries are gone.

[Archbob]

Ok, we have an HTML form, thats not even PHP, thats just a plan html page:

if(isset($_POST['submit']))
{
   $name=$_POST['name'];
   $comment=$_POST['comment'];
   $name=strip_tags($name);
   $comment=$strip_tags($comment);
   $insertg="INSERT in guestbook (name, comment) values('$name','$comment')";
   mysql_query($insertg);
   print "Entry added.";
}

Now for displaying it:

$getentries=mysql_query("SELECT * from guestbook order by ID DESC");
while($getentries2=mysql_fetch_array($getentries))
{
	print "Name:$getentries2[name]<br>Comment:$getentries2[comment]<br><hr>";
}

You would have this at the top in the common.php or connect.php thats included:

if(!get_magic_quotes_gpc())
{
  $_GET = array_map('mysql_real_escape_string', $_GET); 
  $_POST = array_map('mysql_real_escape_string', $_POST); 
  $_COOKIE = array_map('mysql_real_escape_string', $_COOKIE);
}
else
{  
   $_GET = array_map('stripslashes', $_GET); 
   $_POST = array_map('stripslashes', $_POST); 
   $_COOKIE = array_map('stripslashes', $_COOKIE);
   $_GET = array_map('mysql_real_escape_string', $_GET); 
   $_POST = array_map('mysql_real_escape_string', $_POST); 
   $_COOKIE = array_map('mysql_real_escape_string', $_COOKIE);
}

[Captain Proton]

$getentries=mysql_query("SELECT * from guestbook order by ID DESC");
while($getentries2=mysql_fetch_array($getentries))
{
print "Name:$getentries2[name]<br>Comment:$getentries2[comment]<br><hr>";
}

Wow it's been a while since I've seen code like that

[Dr Livingston]

Yer, I had forgotten just how bad PHP could be, if used in the wrong hands

[Archbob]

Its just a small example. It works doesn't it?
I'm not writing a class just to echo or print 1 line.
Want to tell me whats so horribly wrong with that print code and how you would do it?

[DougBTX]

OOP version anyone?

OK:

class Guesbook {
    function get_content() {
        $content = @file_get_contents('guestbook.txt');
        if(strlen($c = htmlspecialchars(trim(@$_POST['comment'])))) {
            file_put_contents('guestbook.txt', "$content$c<hr />");
            header("Location: http://{$_SERVER['SERVER_NAME']}{$_SERVER['SCRIPT_NAME']}");
        }
        return $content;
    }
}

$guestbook = new Guesbook;
?>
<h1>Guesbook 0.0.3</h1>
<?php echo $guestbook->get_content() ?>
<form action="?" method="POST">
<textarea name="comment" rows="20" cols="20"></textarea><br />
<input type="submit" value="add comment to guestbook" />
</form>

Before we add new features, I think we should add the ability to show an error message saying "Sorry, you must enter a comment!" if $_POST['comment'] is empty.

Regards,
Douglas

[arborint]

Yer, I had forgotten just how bad PHP could be, if used in the wrong hands

I have to counter that one of the beauties of PHP is that you can write code just like that. One of the great things about the code posted is that it is readable and understandable by 99% of PHP programmers. This is opposed to code posted by, Dr Livingston for example, where the size of the audience who understands the code approaches one.

For larger scripts (Marcus notes the 100 line mark) scripts become much less maintainable, flexible and testable -- especially for teams.

[bonefry]

Its just a small example. It works doesn't it?
I'm not writing a class just to echo or print 1 line.
Want to tell me whats so horribly wrong with that print code and how you would do it?

They are just making fun . But anyway, it is wrong because you always have to have some sort of error handling, even in "working code". So you never should call mysql_query and mysql_fetch_array directly.

[Captain Proton]

Want to tell me whats so horribly wrong with that print code and how you would do it?

Well, the variable names tell me a lot. $getentries and $getentries2, to me that's an indicator that you don't really know what sort of thing the mysql_* functions return but you know that you need them and you had to give them a name.

It may sound stupid, but variable names (like function names and class names) are important. Meaningful names say that you yourself understand what the code does and it also helps other people who read the code to understand it.

In nitpicking mode I could also say something about the use of constants for array indices ($getentries2[name] instead of $getentries2['name']).

So, nothing to do with the lack of a class or not being OO enough, you see

[Archbob]

I always put or die(mysql_error()) in the query . But that was just a simple example, so I left it out.

I usually don't use the empty function to determine if a string is empty, I always do:

if(strlen($name)<1)
{
	print "String is empty.";
}

For some reason, sometimes even if a string is empty(nothing but spaces), the empty function still thinks there's something there.

Ok, $getentries and $getentires are easy for anyone to understand because basically anyone can understand whats contained in the array are the guestbook entries. I name variable names on what they will display, not what kind of data they are.

[DougBTX]

Might be clearer as:

if ($string == "") {
 // string empty...
}

if ($string != "") {
 // string not empty...
} 

string.empty? anyone?

I always put or die(mysql_error()) in the query .

Since when does die count as error handling? Use die when you don't have error handling and continuing could cause real errors, like data corruption. Otherwise you'll get ugly, meaningless error messages going out to live users of your script. Not great.

Regards,
Douglas

[McGruff]

Ok, we have an HTML form, thats not even PHP, thats just a plan html page

Validation?

[McGruff]

$getentries and $getentires are easy for anyone to understand because basically anyone can understand whats contained in the array are the guestbook entries.

So call them: "$guestbook_entries". Apart from that, it's definitely confusing using one name for the table and the same with a "2" stuck on the end for rows in the table.

[Archbob]

I said that was just a small example, usually I would check to see if there is something in those fields. mysql_error() is when I'm developing something to see where the error is. I do strip_tags, I use mysql_escape-string to get injections and all the good stuff. That was just a small mini-example, sheesh.

[been]

can you explain what you mean by "syntactic validation"?

Not the best of terms, apologies, by syntactic validation I mean stuff like checking if a string is of a certain length, regex matching, date validation etc... like the generic rules you were talking about earlier. Maybe "input validation" would be more appropriate terminology?

Can you explain the difference? From my perspective, those two things are really the same, because user input changes the state of objects. If that new state is invalid, I can spit errors back at the user.

They certainly seem to be related, but I don't think they are the same:
For example we could have input of a date of birth, we could validate that input to be a valid date (ie. February 36 would be an invalid date).
When validated, regardless of the context the date is going to be used in, it is considered a "valid" date. But, if we have that user's-age+18-rule, then that +18 rule would be a precondition of, let's say, a setAge($age) method. The "user must be older than 18" is a business rule, the "valid date" rule is not.
In fact, in this case it is even more a question of separating presentation (the date input field) from business logic (the user must be older than 18), depending on the design, "date of birth" might not even be a property of the User class.

In DbC, the +18 rule would be a precondition, something equivalent to

class User {
      // @pre: $age >= 18
      // @post: getAge() == $age
      function setAge($age) {
          $this->_age = $age;
      }
 } 

the minute you call code like $user->setAge(15), the application would halt with a big bad error. We could do some exception catching to be able to present the user with a meaningfull error message, but then we have to take care of that object in an invalid state.

To sum up, I think validation outside objects is necessary to satisfy the preconditions of the contracts and when they cannot be satisfied to present an application user meaningfull messages without having to put objects in invalid states in between.
I think validation inside the object is necessary to be able to crash early if a contract is breached. Inside is about software quality, outside is mainly about user experience and security.

Does that mean you have a User and a UserValidator? Or do you just have a generic Validator which you initialize when you need it like in Chris's example?

For the moment I have a validation system not unlike WACT's (at least as it was a while back, haven't been following up on Wact lately). Although the naming doesn't really sound right to me, for example's sake, yes, I'd have a User and a UserValidator. But what does that do, a UserValidator? Sounds like it validates users, but if I already have users, why should I need to validate them? So, in practice I might have a User, but I'd have a SignupValidator, EditUserValidator, etc...

Taking "the" example for polymophism:...

I got the feeling that the analogy is a bit skewed. "area" is a clear property of ClosedFigure or something similar, and it clearly belongs to that class. However, should one have a AreaCalculator hierarchy, there would be enough ways to keep the polymorphism, for example:
In a language like PHP we could adhere to a strict naming convention and dynamically instantiate objects based on prefixes.
Or, we could put a getCalculator() method in the ClosedFigure interface.
Or we could implement some hashtable lookup and work with a factory
...

[DougBTX]

Not the best of terms, apologies, by syntactic validation I mean stuff like checking if a string is of a certain length, regex matching, date validation etc... like the generic rules you were talking about earlier. Maybe "input validation" would be more appropriate terminology?

Some form of stricter type hinting?

I wonder, is there a difference between validating the length of a string and validating the magnitude of a number? You seem to be suggesting that the former should happen outside the object, but the latter should be done internally.

I'd have a SignupValidator, EditUserValidator, etc...

Which is more important to you, validation of the request or validation of the affects of the request?

It sounds like the former, where you have a validator for each form, hence one for each request. Like building a wall around your domain objects so that you know they will always be protected from your users.

I guess you hate the idea of me throwing $_REQUEST straight into an object, and just expecting it to work

the minute you call code like $user->setAge(15), the application would halt with a big bad error.

But that never happens. If the application halted on setAge, I might never get to setBirthday, so I wouldn't be able to tell them that February 36 doesn't exist.

Although the naming doesn't really sound right to me, for example's sake, yes, I'd have a User and a UserValidator. But what does that do, a UserValidator? Sounds like it validates users, but if I already have users, why should I need to validate them?

Lets say you have a signup form. When the user submits the form, it will have enough data in it to create a just-signed-up-User. Rather than trying to tie down the request, I'll let the user interact directly with the object. They won't have full access, they won't be able to set the user's role, or the internal user_id, but they will be able to set some things. Before I save the new user, I've got to validate it, that's where the internal UserValidator comes in. (I'm also not happy with the name, it wouldn't even need to be an object, just as SquareAreaCalculator wouldn't need to be an object.)

It doesn't really matter whether I'm looking at a Signup page, or an EditUser page, or anything else. A valid user is a valid user. It means I have the equivalent of a validator per Thing-I-want-to-persist, rather than a validator per request. The Thing knows if it can be saved or if it can't be saved, lets ask it.

In a language like PHP we could adhere to a strict naming convention and dynamically instantiate objects based on prefixes.
Or, we could put a getCalculator() method in the ClosedFigure interface.
Or we could implement some hashtable lookup and work with a factory
...

Aren't those just backdoors to polymorphism like BerislavLopac was trying to write using require?

I'm not denying the existance of alternatives to polymorphism, I'm just saying it is the best tool to use in this case. A strict naming convention and arrays will get you most of the way to reimplementing objects, I believe Drupal tries to pull off this trick. The getCalculator() method I covered, "so you might have $shape->area_calculator instead ... but then you only ever need to access it through $shape->area() so lets make $shape->area_calculator private ...", but you're not taking it as far as it will go. Unless you need a calculator which you can apply to something other than the original Shape object, but you'd need to show me where you need that to justify the extra code you need to handle that case. Hash tables, I assume using the class name as the key? You're back to function prefixes and passing $self variables around I'm afraid.

The analogy might look a bit skewed if you are more interested in validating requests rather than the data you are putting in your database, but as I'm trying to argue, you don't need to validate the requests themselves.

With dynamic typing, you don't need to worry when someone tries to set their age to "John", there will be no "Cannot assign String 'John' to integer field" exceptions, until you try and put in in your database. You'll pick the error up when you validate the object, and you find that "John" is in no way bigger than 18.

Regards,
Douglas

[Luke Redpath]

That was just a small mini-example, sheesh.

Joining in this late, but anyway...you have to make your mind up. Is this "20 lines of code" just a "small mini-example" or the proof-of-concept that you can write a good bit of procedural code to produce a guestbook? You made the claim, people called you on it, you can hardly complain if you write crap code and get criticism

[been]

I wonder, is there a difference between validating the length of a string and validating the magnitude of a number? You seem to be suggesting that the former should happen outside the object, but the latter should be done internally.

Oops, was I that bad at expressing myself?
No, I don't see that much difference in checking a string's length and the magnitude of a number.
I do however feel that there's a difference between checking the magnitude of a number and a business rule (that might involve checking the magnitude of a number)

Which is more important to you, validation of the request or validation of the affects of the request?

It sounds like the former, where you have a validator for each form, hence one for each request. Like building a wall around your domain objects so that you know they will always be protected from your users.

A form can span multiple webpages, we might have 1 validator per request or maybe 1 validator for the whole form, which then would service multiple requests. Also, we could be validating a form that could possibly affect multiple objects, I really don't see the need to instantiate all of them just to make sure the form was filled in correctly.

The most important to me though is that the preconditions (and invariants) of a contract are met before I message the object, it's the only way I can be sure that the object will obey the contract and meet it's postconditions.
In DbC, meeting the preconditions is the responsibility of the client, so from that view it makes no sense having validation in the supplier. OTOH, since PHP hasn't got contracts build into the language, I feel we need enough validation in the supplier to at least error if a client does not obey the contract. The problem with this is code duplication (yuck) and that's one of the things that could probably be resolved by building a contract tool; the tool could generate the internal validation and the contracts could also be used for generating various validators.

I guess you hate the idea of me throwing $_REQUEST straight into an object, and just expecting it to work

Well, considering my view on validation and contracts, it would at least make me seriously nervous... Unless the object you throw it into is a Validator

But that never happens. If the application halted on setAge, I might never get to setBirthday, so I wouldn't be able to tell them that February 36 doesn't exist.

The value that would go into the $age argument would be checked before calling setAge(). BUT, if someone would call setAge() with an illegal age, I want the app to error as quickly as possible to let the developer know. At that point, I'm not even worried about not getting to setBirthday(); when a client doesn't adhere to the contract, it's his fault and I want my components to tell him that asap.

Another issue is the seperation between ui and domain; we might have a fancy date picker in the form to select a date of birth, but our domain class might only have 'age' as property, so we might have to validate a string in "Month DayOfMonth Year" format, while our domain object just works with an age. Of course, that rather useless User class with the setAge() method we're using as example doesn't help either

It doesn't really matter whether I'm looking at a Signup page, or an EditUser page, or anything else. A valid user is a valid user. It means I have the equivalent of a validator per Thing-I-want-to-persist, rather than a validator per request. The Thing knows if it can be saved or if it can't be saved, lets ask it.

As I already pointed out, one could have a form that affect multiple objects, I would want to be sure that there weren't any mistakes made in filling out the form, before I message all the Things. In a shared-nothing-request-response environment that is your typical PHP app, I think that too is something to consider.

Aren't those just backdoors to polymorphism like BerislavLopac was trying to write using require?

I'm not denying the existance of alternatives to polymorphism, I'm just saying it is the best tool to use in this case. A strict naming convention and arrays will get you most of the way to reimplementing objects, I believe Drupal tries to pull off this trick. The getCalculator() method I covered, "so you might have $shape->area_calculator instead ... but then you only ever need to access it through $shape->area() so lets make $shape->area_calculator private ...", but you're not taking it as far as it will go. Unless you need a calculator which you can apply to something other than the original Shape object, but you'd need to show me where you need that to justify the extra code you need to handle that case. Hash tables, I assume using the class name as the key? You're back to function prefixes and passing $self variables around I'm afraid.

You're right of course, I just posted them as workarounds (or 'backdoors' if you prefer) for something I agree with was a bad idea to begin with (seperating calculating the are from the figure).
I still feel the analogy's skewed though: 'area' is a property of a closed figure, but what is 'being valid'? And whereof is it a property? And although not as elegant, polymorphism is still possible if needed, it just doesn't seem to be that necessary to me in this case.

The analogy might look a bit skewed if you are more interested in validating requests rather than the data you are putting in your database, but as I'm trying to argue, you don't need to validate the requests themselves.

The analogy looks skewed to me because it seems to regard a property and a concept as similar and I just don't think that's true.
Granted, you don't need to validate the requests, you don't need to do anything, you can even just take $_REQUEST, massage it into an sql query, send it over a db connection and see what happens

Btw, suppose one does not use the Active Record pattern, but a DataMapper. In your view, where does the validation part (after filling out a form) go then? I'm genuine curious because, if I understand you correctly, the validation code would be triggered when $user->save() is called (using AR). When using a DataMapper, it'll probably be something like $mapper->save($user), so would you put the validation code into the mapper then?

Although we seem to indeed come in on this with totally different angles, the discussion makes me think about the stuff just that little bit more, for which I thank you sincerely.

[DougBTX]

Btw, suppose one does not use the Active Record pattern, but a DataMapper. In your view, where does the validation part (after filling out a form) go then? I'm genuine curious because, if I understand you correctly, the validation code would be triggered when $user->save() is called (using AR). When using a DataMapper, it'll probably be something like $mapper->save($user), so would you put the validation code into the mapper then?

To keep similar external code:

$object = new Object($data);
if ( $object->save() )
{
    # code...
} 

vs

$object = new Object($data);
if ( $mapper->save($object) )
{
    # code...
} 

You end up with with this in the mapper:

class Mapper
{
  function save( $object )
  {
    if ( $object->is_valid() )
    {
        // map and save the object
    }
  }

} 

but then it isn't a real Mapper anymore, because it depends on a method on $object.

Here's how I see it, as a distinction between Models and Domain objects:

A Domain object represents something inside the domain, which must be mapped to a database.

A Model represents something external the we want to manipulate, which must handle its own persistance.

The rest is down to how you want to implement Design by Contract (not whether you have contracts or not). If you took a DomainObject and an ObjectMapper, then wrapped them in a Façade which could enforce pre-conditions when you invoke the Mapper, you would probably end up with something close to the Model that I'm thinking of.

With domain objects, you've got to put the contracts up front, because, almost by definition, you can't do any validation during mapping. Because I like the Shape more than the User too, and I still need to justify my example, I'll stick with it for now.

Here is our rule:

I, Shape, will take on only a secondary colour, "green", "orange" or "purple".

Now, with a somewhat litteral reading of design by contract, I think that gives us this:

class Shape
{
    function __construct ( $color )
    {
        $this->assert('color', in_array($color, array("green", "orange", "purple")), "What is this colour?");
        
        #   passed the contract, so continue
        $this->color = $color;
    }
    
    function assert ($field, $valid, $message = "Contract not fulfilled" )
    {
        if ( !$valid )
        {
            throw new ConditionException ($field, $message);
        }
    }
    
} 

So, with this code, I think we can write a RequestValidator which would act something like this:

if (in_array($_REQUEST['color'], array("green", "orange", "purple"))
{
    $shape = new Shape($_REQUEST['color']);
    $mapper->save($shape);
}
else
{
    #   send the form back with error messages...
} 

But, as you say, that creates lots of unnecessary duplication (in_array is inside and outside Shape), but this would work much the same:

try {
    $shape = new Shape($_REQUEST['color']);
    $mapper->save($shape);
}
catch (Exception $e)
{
    #   send the form back with error messages...
} 

and you still have your Domain object blisfully unaware of the world around them. That's about all I can see to do with the given Shape object, if your validation setup is very much different, I'd like to see it. Mapper could also throw exceptions to send complaints back to the user, so this code would work even if we assume a Model instead of a Domain object:

try {
    $shape = new Shape($_REQUEST['color']);
    $shape->save();
}
catch (Exception $e)
{
    #   send the form back with error messages...
} 

I hadn't thought of a setup like that until I started writing this message, and I can see it working, but... I don't much like using exceptions for things which I can expect to happen, and I like the code duplication in the first example even less (even if it was generated code duplication).

So this is why I like the Model setup more, because it lets you get around that, (barring revelations of something I've missed about the Domain object implementation )

An ActiveRecord model could look like this:

class Shape extends ActiveRecordBase
{
    function __construct ( $color )
    {
        $this->color = $color;
    }
    
    function validate ( )
    {
        $this->assert('color', in_array($this->color, array("green", "orange", "purple")), "What is this colour?");
    }

} 

As you can see, the contract code is still there, it is just in a different place. The magic, is that it lets us think of things in a rather different way. Note that the assert line now uses $this->color instead of testing the input $color directly, this means we can test compliance with the contract whenever it is most useful for us. (If you called $this->validate() every time $color came through a setter, then it would have much the same behaviour as the original code.)

So, when is it most useful to call the validate method?

Because we are on the web, with corse grained requests (all the user data from the request comes in in one block of url encoded data, all our responce goes out in one block of XML encoded data), I think we should call validate as late as possible. It doesn't matter how $this->color gets set, as long as it is set by the time we want to do anything with it.

And what do we want to do with it? One thing is to save it to the database, which is a perfect oppertunity to call validate. save() belongs to ActiveRecordBase, which will call our validate code, letting us write:

$shape = new Shape($_REQUEST['color']);
if ( !$shape->save() )
{
    #   send the form back with error messages...
} 

That's got us where the Domain style code got us, though without the duplication or exceptions.

But we're not done yet. As we are are talking about ActiveRecords, we can ask the Record what it looks like. What do you look like? I've got a "color" field!

That means we don't need to write the $this->color = $color line anymore, we can just pass in an array, and the ActiveRecord can pull in the data it has fields for. One less thing to worry about. Here's what our reflective ActiveRecord class looks like:

class Shape extends ActiveRecordBase
{
    function validate ( )
    {
        $this->assert('color', in_array($this->color, array("green", "orange", "purple")), "What is this colour?");
    }
} 

It is just a contract. That's all we have to write. We now have the option of constructing the new Shape implicitly:

$shape = new Shape($_REQUEST);
if ( !$shape->save() )
{
    #   send the form back with error messages...
} 

And hopefully that demonstrates the ideas

Regards,
Douglas

[McGruff]

Here's how I see it, as a distinction between Models and Domain objects:

A Domain object represents something inside the domain, which must be mapped to a database.

A Model represents something external the we want to manipulate, which must handle its own persistance.

The domain layer and the MVC model are one and the same.

[DougBTX]

The domain layer and the MVC model are one and the same.

They are related, but they are not the same.

Specifically: using a domain is one way to write a model layer. MVC is a more generic idea than domains.

[McGruff]

Sorry to contradict you but that's definitely not correct. What are your references for this?