Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I can't get my head around this easy step-by-step validation. I already had it in a while-loop, but found it even more horrible than with beginner-like if-cases. I still think though, that it could be written a bit simpler (and maybe improve readablity).

What I'm doing here ? Basically I check for required input fields and if they fail to validate, I need to go to another level of output, check for other required fields, and so on...

It already is a quite simple thing to do, but in the course of development it will get bigger and bigger, so it needs to stay simple.

    // CHECK FOR REQUIRED PROPERTIES
    if($properties['step'] == 2)
    {
        $req = array('cshort','cid');
        foreach($req as $required) 
        { 
            if(!validate_var($req,$properties[$req]) || empty($properties[$req])) 
            {
                $step = 1;
            }

        }
    }
    if($properties['step'] == 1)
    {
        $req = array('cshort');
        foreach($req as $required) 
        { 
            if(!validate_var($req,$properties[$req]) || empty($properties[$req])) 
            {
                $step = 1;
                return $st
            }

        }
    }
share|improve this question

2 Answers

up vote 2 down vote accepted

Firstly I'll review things as you have them.

Some things are backwards. You need to change it to:

$required = array('cshort');

foreach ($required as $req)
{
          if(empty($properties[$req]) || !validate_var($req,$properties[$req]))

The $req is now the value in the array rather than being the entire array (which would have been a mistake).

The check for empty will come first now because logical expressions are evaluated from left to right (after any operator precedence is determined).

Missing code:

$step = 1;
return $st

I'm guessing you wanted it to be return $step;. I would write this as return 1; rather than create a variable that does not vary.

How to Improve

You are wanting to check a list of requirements. The data structure that resembles that is an array (which you have used already). The best things for processing arrays are loops because they avoid the cut and pasted code.

Here is what I would do (there are comments in it with further explanation):

$required = array();

// Build the requirements first.
switch ($properties['step'])
{
// Case statements without a break cause the processing to continue into the
// next case.  Using this case 2 will add to the start of the array 'cid' and
// then will add 'cshort' before that.  
case 2:
    array_unshift($required, 'cid');
case 1:
    array_unshift($required, 'cshort');
    break;
default:
    throw new DomainException('Step is not within the correct range');
}

$step = 0;

// Loop through, processing all of the requirements and counting the successful steps.
foreach($required as $req) 
{ 
    if(empty($properties[$req]) || !validate_var($req,$properties[$req])) 
    {
        return $step;
    }

    $step++;
}

return $step;

Note: Building the list of requirements could be done many ways. The way I chose of letting the switch statement flow through the steps might be controversial. However the basic idea of looping over the array rather than having many cut and pasted loops is my main recommendation.

share|improve this answer
up vote 3 down vote

Why do you only return a value if $properties['step'] == 1? If you could somehow change it so that both return, or both didn't need to, you could make it neater.

$req = array('cshort');
$return = FALSE;

if($properties['step'] == 1) {
    $req[] = 'cid';
    $return = TRUE;
}

foreach($req as $required) {
    $step = 1;
    if($return) {
        return $st;
        /*
          $step? what is this where did it come from?
          Unless you are in a function, you don't need to return.
          Just use $st/$step when you need it since you only ever
          gave it one value. Just check if it isset first.
          If you want to break from the loop, just use break,
          or continue to skip.
        */
    }
}

You mentioned expanding. If this gets too much bigger you could try a switch statement instead of multiple if statements. Assuming the rest don't use the same format, otherwise just tweaking what I gave above should work.

switch($properties['step']) {
    case 1:
        foreach($req as $required) {
        }
    break;
    case 2:
        $req[] = 'cid';
        foreach($req as $required) {
        }
    break;
}
share|improve this answer
I seem to have gotten a few upvotes since last I looked. Be sure to read Paul's post as well. He did get a few things I missed and it is still very informative. – mseancole Apr 20 '12 at 16:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.