0
\$\begingroup\$

I denotes independent, no dependencies.

<?php
/**
 *Input - One Dimensional Non-Empty Post Array
 *Ouput - Boolean on pass or fail
 */
class IText
{
    private $text_array = array();
    private $patterns = array(
       'domain' =>              '/:\/\/(www\.)?(.[^\/:]+)/',
       'prefix_url' =>          '/^(http:)|(https:)\/\//',
       'url' =>                 '/^.{1,2048}$/',
       'tweet' =>               '/^.{1,40}$/',
       'title' =>               '/^.{1,32}$/',
       'name' =>                '/^.{1,64}$/',
       'email' =>               '/^.{1,64}@.{1,255}$/',
       'pass' =>                '/^.{6,20}$/'
    );
    public function __construct()
    {
        if ( count( $_POST ) === 0 )
        {
            echo "Hack Attempt";
            return;
        }
        else
        {
            foreach( $_POST as $key => $value )
            {
                if( !is_scalar( $value ) ) 
                {
                    echo "Hack Attempt";
                    return;
                }
                $this->text_array[ $key ] = htmlentities( $value );
            }
        }
    }
    public function get( $key ) // basic getter
    {
        return $this->text_array[ $key ];
    }
    public function set( $key, $value ) // basic setter
    {
        $this->text_array[ $key ] = $value;
    }
    public function checkPattern( $pattern ) // checks for pattern and returns bool
    {
        return ( ( boolean )preg_match( $this->patterns[ $pattern ], $this->text_array[ $pattern ] ) ); 
    }
    public function checkEmpty() // checks for empty and returns bool
    {
        return ( !in_array( '', $this->text_array, TRUE ) );
    }
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

I've tried to cover as many things I could. A few of my explanations are rather brief, so if you want me to explain or defend anything, let me know. And hopefully it doesn't read too ramble-y :).

IText

IName typically denotes an interface. It's not a naming scheme often used in PHP, but it's used often enough that you should avoid prefixing an I onto a class that isn't an interface. This is confusing. In fact, when I first saw the name, I was expecting an interface.

As a developer consuming the class, it really is not all that important if it's "independent" since most IDEs will show the constructor signature. Interface vs abstract vs concrete tends to matter, but I do not feel that independent necessitates its own naming scheme.

mixed concerns

I'm not sure if I understand what your class is doing. Based on the name, it looks like you're trying to model a string. BUt what it's actually doing is sort-of processing POST.

Instead of doing this, I would create validators that are separate from the model of a string (or the model of any data type really). Also, why hold an array of values inside of the object? Is the object meant to be some kind of repository or something?

Once you get a lot of validations, or once you get a few very complicated ones, this class will grow to be unmanagable.

Personally, I like the model of Zend_Validate. It completely separates the concept of data and the validation of data. In your class, storage, validation and manipulation have all been (needlessly) coupled. (Obviously storage and validation go together when storing things long term, but in this context, I mean storing as in storing in PHP-land.)

A perfect example of your validation and storage coupling is your checkPattern method. What if you want to validate something as a URL, but it's not stored in $_POST['url']? What if you want to check $_POST['website']? Or what if you have an arbitrary $url that you want to check? Your code now does not facilitate that. It is directly tied to the expectation that the validation and the data will be stored with the same keys.

htmlentities

Your values are not being used in the context of HTML, so they should not be treated as HTML. Only treat HTML as HTML. In other words, don't escape data until it actually needs to be escaped. Your validations are going to be confused when they see a &#38; b instead of a & b (or any other substitutions htmlentities may make).

$_POST

A lot of the comments in the "mixed concerns" section, you're probably thinking "but it's only meant to manipulate post?"

Well, what if at some point in the future, you have an arbitrary array you want to handle? There's no need to couple your code to $_POST. Instead, pass the array in to the constructor. (Well, in all honesty, I think your design is flawed, but if you do stick with it, pass the array in.) What if at some point in the future you have a JSON based API and you want to validate that? How would you do that with your current code?

technicalities

If nothing is posted (count($_POST) === 0), then text_array is going to be NULL. This will mean that every array access will issue a notice. You should initialize the member to an array either in the declaration or in the constructor. I would do it in the declaration:

private $text_array = array();

Also, you're assuming $_POST is one dimensional. If someone posts a form like:

<input type="text" name="test[]" value="array">

Then $_POST will look like:

array('test' => array('array'))

This means that you'll end up passing an array to htmlentities. In other words, don't assume that POST values are strings. (Though they are always either a string or an array.)


in addPrefix, you've used textArray instead of text_array

Constants

Instead of using names of the validations, use class constants instead. This will mean that calling code doens't need to know the array keys, and that they can be changed if needed/wanted. It will also tend towards cleaner looking and more maintainable code since typos will be errors instead of falling through the cracks.

For example:

const DOMAIN = "domain";

Then:

$text->checkPattern(IText::DOMAIN);

Be paranoid

Never assume anything in code. In particular, don't assume that a variable will be the type that you expect or that an array key will exist.

Note that these both basically only apply to user data since if you create the data, you can know with certainty what it is.

Consider your checkPattern method:

public function checkPattern( $pattern )  // pattern checker returns bool
    {
        return ( preg_match( $this->patterns[ $pattern ], $this->text_array[ $pattern ] ) ); 
    }

What if an invalid pattern name is provided? Then suddenly you have at least 3 notices (two that the array key does not exist, and one that preg_match received an empty pattern).

You should be safe guarding things like this:

public function checkPattern($pattern)
{
    if (!array_key_exists($pattern, $this->patterns)) {
        //Pattern doesn't exist
        throw new Exception("Invalid pattern provided: " . $pattern);
    }
    if (!array_key_exists($pattern, $this->text_array[$pattern])) {
        //data doesn't exist
        return false;
    } else if (!is_scalar($this->text_array[$pattern])) {
        //If this were checked on construction, this would not be necessary
        throw new Exception("The data stored for {$pattern} is not a scalar, but it's being treated as such");
    }
    return (bool) ( preg_match( $this->patterns[ $pattern ], $this->text_array[ $pattern ] ) ); 
}

Also, while I'm at it, your comment is a lie. preg_match returns an int, not a bool. A wrong comment is significantly worse than no comment.

Final suggestions

I would consider decoupling your data and validations. What you could do is create a "Form" that encapsulates all of this. Basically your form would have elements and each element would have validations. The form would then be capable of taking in an arbitrary array and telling you if it's valid per the specification of the form.

If you want a few ideas on this, you could look at Zend_Form. It has a lot of limitations, and the rendering side of it can be a bit hard to get along with, but overall, it's a fairly decent modeling of a form. Zend_Form is likely not the best, just Zend Framework happens to be what I'm familiar with.

Edit: URL handling

Just noticed that you've used a regex to extract out the domain from a url. Instead of doing that, you might want to use parse_url.

\$\endgroup\$
18
  • \$\begingroup\$ @Corbin, just to say that your review was an inspiration! Tks, I mean it! :) \$\endgroup\$
    – Zuul
    Commented Jun 15, 2012 at 0:46
  • \$\begingroup\$ @stack.user.0 The idea isn't to resemble Zend. That wasn't meant to be a "why doesn't your code look like ZF's?" Was meant more as "if you want some examples to look at...." Anyway, you shouldn't worry about number of files or file sizes. Instead, you should focus on maintainable, extendable code. (Not extendable as in inheritance, but rather ability to continue to use in the future.) A lot of files will slow down execution since it requires a lot of disk IO, however, there are various ways around it (opcode caches for example), and worrying about that is very rarely necessary. \$\endgroup\$
    – Corbin
    Commented Jun 16, 2012 at 1:42
  • \$\begingroup\$ And yes, $_POST can be empty in the sense of count($_POST) === 0. If that's the case, the foreach loop will never run, leaving the object's member NULL. (Also, on a technical note, $_POST can actually be disabled as odd as it is.) \$\endgroup\$
    – Corbin
    Commented Jun 16, 2012 at 1:44
  • \$\begingroup\$ Making a note that it should be one dimensional is not sufficient. You should code defensively. That is, either handle multidimensional arrays, or put code in place to trigger errors if an array is not single dimensional. If you alter the code to accept a generic array, then the note would be understandable, however, that then just moves the responsibility out of the class (and thus it should still be checked really). You can't control whether $_POST is single or multi-dimensional, so the note doesn't make sense. \$\endgroup\$
    – Corbin
    Commented Jun 16, 2012 at 1:46
  • \$\begingroup\$ "I cast to an int but did not change my comment" There's no need to cast when it already returns an int. Perhaps I misunderstood that? \$\endgroup\$
    – Corbin
    Commented Jun 16, 2012 at 1:46
0
\$\begingroup\$

"Also, while I'm at it, your comment is a lie. preg_match returns an int, not a bool. A wrong comment is significantly worse than no comment." - Corbin

Beat me to it. Also, nice review. Looks like I've got another competitor :) Agree 100% with what Corbin has said.

Only one thing to add. addPrefix() is named odd. I would consider getUrl instead. You are not really prefixing anything, you are retrieving something. As a script running this, I won't know what you do to it within the method, I only know what I get back, and that's a URL.

\$\endgroup\$
2
  • \$\begingroup\$ If only there were a lot more competitors on CR :p. It's always so slow on CR. Anyway, addPrefix does not return anything. Though there is a major problem with it... If it's called more than once then it will be appended more than once. \$\endgroup\$
    – Corbin
    Commented Jun 14, 2012 at 21:58
  • \$\begingroup\$ Point taken, probably would be better if it just returned it :) \$\endgroup\$
    – mseancole
    Commented Jun 14, 2012 at 22:19