1
\$\begingroup\$

This class holds message types that are sent to the client which knows what to do with them. It is basically a small registry pattern where I can consolidate message types. |P| and |F| are read by the client...and I use these for simple data passing when I don't need the complexity of JSON. The programmer is expected to know which messages types are available.

I've added in a array_key_exists() as a defensive measure against a progammer calling a message that does not exit. I did this as someone said this is best practice...I'm looking for a second opinion...as in this case it is not a big deal but overall "defensive coding" would increase my library size / efficiency by 10-20%. Can I take this out?

<?php
class IMessage
{
    private $PASS = '|P|';
    private $FAIL = '|F|';
    private $messages = array();
    public function __construct()
    {
        $this->messages['PASS'] =  $this->PASS;
        $this->messages['name'] =  $this->FAIL . 'name';
        $this->messages['email'] = $this->FAIL . 'email_s';
        $this->messages['pass'] =  $this->FAIL . 'pass';
        $this->messages['url'] =   $this->FAIL . 'url';
        $this->messages['title'] = $this->FAIL . 'title';
        $this->messages['tweet'] = $this->FAIL . 'tweet';
        $this->messages['empty'] = $this->FAIL . 'empty';
        $this->messages['same'] =  $this->FAIL . 'same';
        $this->messages['taken'] = $this->FAIL . 'taken';
        $this->messages['validate'] = $this->FAIL . 'validate';
    }
    public function get( $type )
    {
        if( !array_key_exists($type, $this->messages ) ) 
        {
            echo "Programmer Error: You called a key that does not exist";
        }
        else
        {
            return $this->messages[ $type ];
        }
    }
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Well, you are doing this a little redundantly...

$keys = array(
    'name',
    'email',
    //etc...
);

$messages = array(
    'PASS' => $this->PASS,
);

foreach( $keys AS $key ) { $messages[ $key ] = $this->FAIL . $key; }

$this->messages = $messages;

Also, your get function has a fatal error. You currently have a check to see if a key exists, and if it doesn't it will output an error message. This is incomplete, if that key doesn't exist it will output the error message then PHP will throw an undefined index error because $key does not exist. To avoid this you should have it return that error message when you set it or have it return FALSE. I think such a check is good, since it seems you are dealing with user supplied information. If this was a private method and you knew the information you were passing it was reliable, then I would say its unnecessary.

\$\endgroup\$
5
  • \$\begingroup\$ @stack.user.0 I didn't mean to comment on its lack of content, only to point out that there is a fatal error that will occur because of it. Will change answer so it is more clear \$\endgroup\$
    – mseancole
    Commented Jun 18, 2012 at 18:11
  • \$\begingroup\$ @stack.user.0 Sorry gotdistracted for a moment. The answer is updated. \$\endgroup\$
    – mseancole
    Commented Jun 18, 2012 at 18:24
  • \$\begingroup\$ thanks..good catch...this is programmer supplied input..not a site user...a programmer will write object->get()...considering that programmer might be me...a check is indeed a good idea. I just need to put it an else statement. done. \$\endgroup\$
    – user7459
    Commented Jun 18, 2012 at 18:31
  • \$\begingroup\$ If you do .js need input on codereview.stackexchange.com/questions/12695/… \$\endgroup\$
    – user7459
    Commented Jun 18, 2012 at 18:55
  • \$\begingroup\$ @stack.user.0: very lightly mostly basic needs and minor AJAX. I can take a look at it and give you my perspective from a purely PHP point of view though. Should translate relatively well. \$\endgroup\$
    – mseancole
    Commented Jun 18, 2012 at 19:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.