2
\$\begingroup\$

Note, I choose not to use the Factory Pattern out of preference.

This is a ( M ) odel class in the MVC pattern.

Private functions return Booleans and public functions return strings which I then output to the browser from the Controller. The class returns a string from the invoke call.

Here is the only call to the class:

Call

$model = new UserNew( new IUniversals() , new IDatabase(), new IText(), new IMessage() ); 
echo $model->invoke();

Class

class MUserNew extends MUser // set mysql defaults
{
    private $universal_object;
    private $text_object;
    private $message_object;
    private $input_fields = array( 'name', 'email', 'pass' );   

    public function __construct( $universal_object, $text_object, $message_object  )
    {
        parent::__construct();
        $this->universal_object = $universal_object;
        $this->text_object = $text_object;
        $this->message_object = $message_object;
    }
    public function invoke()
    {
        $not_validated = $this->checkInput();
        if( $this->universal_object->get( 'load' ) && ( $not_validated !== true ) )
        {
            return $not_validated;
        }
        else
        {
            $email_is_unique = $this->checkEmail();
            if( ( $email_is_unique === true ) )
            {
                return $this->message_object->get( 'PASS' ) . $this->insertRecord() ;
            }
            else
            {
                return $email_is_unique;
            }
        }
    }
    private function checkInput()
    {
        if( !$this->text_object->checkEmpty() )
        {
            return $this->text_object->get( 'empty' );
        }
        foreach( $this->input_fields AS $pattern ) 
        {
            if( !$this->text_object->checkPattern( $pattern ) ) 
            {
                return $this->message_object->get( $pattern );
            }
        }
        return true;
    }
    private function checkEmail()
    {
        if( $this->database_object->_pdoQuery( 'single', 'signup_check', array($this->text_object->get('email') ) ) )
        {
            return $this->$message_object->get( 'taken' );
        }
        else
        {
            return true;
        }
    }
    private function insertRecord()
    {
        srand( time() );  
        $hash = crypt( $this->text_object->get( 'pass' ), substr( strval( rand() ), -2 ) );
        $this->text_object->set('pass', $hash );
        $this->database_object->_pdoQuery( 'single', 'signup_insert', array( '', $this->text_object->get('name'), $this->text_object->get('email'), $this->text_object->get('pass'), 0, 0 ) );
        $this->database_object->_pdoQuery( 'none', 'bookmark_insert', array($result['id'], 'Facebook', 'http://www.facebook.com', 'facebook.com', 'social', 'http://static.ak.fbcdn.net/rsrc.php/yi/r/q9U99v3_saj.ico'));                
        return $hash;
    }

}

\$\endgroup\$
3
\$\begingroup\$

Where do I begin?

Simplest first. Make sure you assign your constructor a state (public, private, or protected). Most likely public.

Now... Where are all these extra properties coming from? $ISharedObject is not even remotely similar to anything else, so it isn't a typo. And its not defined as a class property, so I can't even be sure this is supposed to be here. The class you provide does not extend a parent class, so it isn't from there. It just appeared in invoke() out of no where. The method it uses suggests that it is most likely a $MessageObject but I don't know.

It looks like you've either copy-pasted this all together, or had a collaboration with someone else and you've crossed your naming schemes. $MessageObject and $TextObject looks like they should be $message_object and $text_object respectively. They should be renamed as such or vice-versa. It is probably easier to rename where they are defined, less to change, but up to you.

You have a $not_validated and $validated variable, of which only the first was defined.

If checkEmail() only ever returns a boolean, like its name suggests, then there is no need for absolute equality operators "===". Just if( $email_is_unique ). If it returns something other than a boolean, then it is doing too much and needs to be separated into multiple functions.

Arrays and loops are your best friend for repititive code.

$patterns = array(
    'name',
    'email',
    'pass',
);

foreach( $patterns AS $pattern ) {
    if( !$this->TextObject->checkPattern( $pattern ) ) {
        return $this->MessageObject->get( $pattern );
    }
}
return true;

Change these first few things and I'll take another look, but right now this is very confusing.

UPDATE

"This has irked me for a long time...code that is DRY ( Don't Repeat Yourself )...is less efficient then code that has some repetition. Dry is a good concept unless taken to extreme...as far as looping goes...either way is fine I think....I'll add the loop in as a matter of preference even though a tad bit less efficient."

How do you figure this is less efficient?

$patterns = array( 'name', 'email', 'pass' );   
foreach( $patterns AS $pattern ) 
{
    if( !$this->$text_object->checkPattern( $pattern ) ) 
    {
        return $this->$message_object->get( $pattern );
    }
}

Than this?

if( !$this->$text_object->checkPattern( 'name' ) ) 
{
    return $this->$message_object->get( 'name' );
}
if( !$this->$text_object->checkPattern( 'email' ) ) 
{
    return $this->$message_object->get( 'email' );
}
if( !$this->$text_object->checkPattern( 'pass' ) ) 
{
    return $this->$message_object->get( 'pass' );
}

The first reduces the amount of code written, which in turn reduces the amount of times you can make mistakes, which in turn reduces the amount of lines you'll have to wade through should you ever need to debug it. It also automates similar tasks for any parameters you wish to throw at it so that if that task should ever need to change, you'll only ever have to do so once. Or should you ever wish to add to it, you only need to add a parameter to an array. Where did you hear that DRY is inefficient? I admit abusing it could get ridiculous, but you have to go out of your way to accomplish that.

Anyways, here's what I have for your update.

You still have that issue with variables and properties appearing out of thin air. $validated I'm assuming should be $not_validated. Though I, personally, would switch that around. I like imagining my variables like this to be asking a question "validated?" and not "telling" you its state. But thats a preference. $TextObject should probably be $text_object.

Another thing I just noticed. You are declaring your properties like so: $this->$text_object; When they should be declared like so: $this->text_object; The first is called a variable variable. It states that whatever the value of $text_object is will be declared as the name of a new property. First of all, these are cosidered bad form, mostly because they are impossible to track and make your code unuseable by anyone but you. Not really, but pretty damn close, not even IDE's can track these. Here it is explained in code, hopefully more clear.

$a = 'apple';
$b = 'banana';
$this->a = $b;
$this->$a = 'orange';
$$a = 'tangerine';

echo $a;//apple
echo $b;//banana
echo $this->a;//banana
echo $this->apple;//orange
echo $apple;//tangerine
//$this->apple and $apple just magically appeared, this is because they were set from the value of $a.

You have a couple of really long statements that could be made more legible. There are a few ways you can do this. The simplest is to bring the parameters out of the method call and convert them to variables in the local scope. This is pretty common and helps clean up code quite a bit.

$email = array( $this->TextObject->get('email') );
if( $this->$database_object->_pdoQuery( 'single', 'signup_check', $email ) )

Another way, usually used in conjunction with the previous, is to use PHP's helpful nature to your advantage. PHP does not care about whitespace, so you can drop long statements like this to new lines and indent to your hearts content.

$this->$database_object->_pdoQuery(
    'single',
    'signup_insert',
    array(
        '',
        $this->TextObject->get('name'),
        $this->TextObject->get('email'),
        $this->TextObject->get('pass'),
        0,
        0
    )
);
//OR
$name = $this->TextObject->get('name');
$email = $this->TextObject->get('email');
$pass = $this->TextObject->get('pass');
$array = array(
    '',
    $name,
    $email,
    $pass,
    0,
    0
);
$this->$database_object->_pdoQuery(
    'single',
    'signup_insert',
    $array
);

Last thing I want to mention is magic numbers. Those are numbers that appear out of thin air and could mean anything. Much like those variable variables I mentioned earlier, they are almost impossible to track down. To you, as the author, you probably know what these are at a glance. But to someone reading your code for the first time, they will not have a clue. This could even be you in a few months after you've started working on other projects. Take a look at the code above. Those zeroes "0" and empty strings '' don't mean anything. They just exist. They are magic. Declare them and set them as variables so people know what they do.

\$\endgroup\$
  • \$\begingroup\$ class.IText.php - codereview.stackexchange.com/questions/12592/… \$\endgroup\$ – user7459 Jun 14 '12 at 20:34
  • \$\begingroup\$ @stack.user.0: Updated \$\endgroup\$ – mseancole Jun 14 '12 at 21:14
  • \$\begingroup\$ @stack.user.0 Yes, DRY code can be less efficient, but that's like saying that code that uses functions is less efficient. You could always throw every statement one line after another with all kinds of copy and pasted snippets. It would probably be more efficient, but a .0000001% efficiency increase is rarely worth less maintainable code. Also, the two snippets (the loop and unwound loop) are not exactly a shining example of DRY. DRY refers to encapsulating functionality, not unwinding loops. \$\endgroup\$ – Corbin Jun 14 '12 at 22:24
  • \$\begingroup\$ we're just trying to say that the overhead is so minuscule as to be unnoticeable. \$\endgroup\$ – mseancole Jun 14 '12 at 23:36
  • \$\begingroup\$ I look at overhead as a relative concept..in this case hitting the for loop 3 times with index lookup as opposed to 0 times...relative to this class...I'd say about.1% - the hashing done by crypt() I'm guessing is like 99.9% of the work...so it is updated. \$\endgroup\$ – user7459 Jun 17 '12 at 20:55
1
\$\begingroup\$

Its been a long while since I have seen your code for review. I remember when you were starting out and producing horrible code. It is very nice to see the massive improvement that you have made! This code looks quite reasonable.

The Good

  • Dependency Injection. You said that you didn't do it, but you have! You passed each dependency to the constructor here:

    $model = new UserNew( new IUniversals() , new IDatabase(), new IText(), new IMessage() );
    

    That is all there is to dependency injection, you have done it nice and simply.

  • Encapsulation/Interface Hiding:

    You have hidden all of the properties and given a simple interface to the user of the class with a limited number of public methods.

Keep doing those good things!

The Bad

  • invoke does not have much meaning and is similarly named to the magic method __invoke.

    Its hard to tell what invoking a user will do. It looks like it is creating a user. create might be a more meaningful name.

The Future

Things can always be done better, but endless improvement doesn't get things finished. I would recommend reading about the Single Responsibility Principle and Data Mappers. This could help simplify and increase the re-usability and maintainability of this code even further. The data mapper could also remove your dependence on a database as your persistent storage (although a database is a very good place to store things).

You have definitely come a long way, nice work.

\$\endgroup\$
  • \$\begingroup\$ I'm staring to write more .js after reading shop.oreilly.com/product/0636920018421.do - more state / functionality is being implemented in .js. \$\endgroup\$ – user7459 Jun 18 '12 at 17:44
  • \$\begingroup\$ My library is here its using the MIT license. Its still in alpha at this stage. \$\endgroup\$ – Paul Jun 19 '12 at 10:48
  • \$\begingroup\$ I see you are making dynamic function calls from your controller...but how do yo instantiate your models? \$\endgroup\$ – user7459 Jun 19 '12 at 20:05
  • \$\begingroup\$ Anyways...if you want to add my UploadImage class to your library...let me know and I can iron out to the specs you want...I think you have reviewed it before...but if not I an repost the updated version...thanks for your help again. \$\endgroup\$ – user7459 Jun 19 '12 at 20:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy