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.
