2
\$\begingroup\$

I am validating two variables of an Identification card (cedula in the code). The first one is a string and the second is an array.

My inputs are:

  • 1- e.g. 3-728-2208
  • 2- e.g. array('1' => '3-728-2208', '2' => '1-728-2208')

My method is :

public function validateId($cedula)
{
    $rules = ['cedula' => 'cedula'];

    $input = ['cedula' => $cedula];
    if(is_array($cedula))
    {
        $validator = $this->validateIdIsArray($cedula);
    } else {
        $validator = Validator::make($input, $rules);
    }

    return !($validator->fails());
}


public function validateIdIsArray($cedula)
{
    $rules = ['cedula' => 'cedula'];

    foreach ($cedula as $ids) {
        $input = ['cedula' => $ids];
        $validator = Validator::make($input, $rules);
    }

    return !($validator->fails());
}

My method from Rule:

public function passes($attribute, $value)
{
   return preg_match('/^((\d{1,2}|N|E|PE|\d{1,2}PI)-\d{1,5}-\d{1,6})$/u', $value);
}
\$\endgroup\$
2
  • \$\begingroup\$ that's is my and it's done honestly i dont know how can i improve it \$\endgroup\$ Nov 13, 2017 at 16:46
  • 1
    \$\begingroup\$ oh sorry - your title made it sound like you were asking for advice on future code \$\endgroup\$ Nov 13, 2017 at 16:47

3 Answers 3

3
\$\begingroup\$

I think I would do away with public function validateIdIsArray($cedula) entirely and write public function validateIds($cedula) to work on array data by default:

public function validateIds($cedula){  // or $ballot I guess
    $rules = ['cedula' => 'cedula'];
    $cedula=(array)$cedula;  // unconditionally set type as array (if was string, it will be single element array)
    foreach($cedula as $input){
        if(Validator::make(['cedula'=>$input], $rules)->fails()){
            return false;  // invalid.  Is it not suitable to do an early return on first failure?
        }
    }
    return true;  // valid
}

I may be confusing some of your subsequent function calls make, passes, fails -- so you may need to adjust my suggested code to return the intended boolean value.

Your pattern can be condensed. You can logically remove the outer capture group, convert all capture groups to non-capture groups, reduce alternatives by leveraging ?, and remove the unicode flag:

Demo Link

/^(?:\d{1,2}(?:PI)?|P?E|N)-\d{1,5}-\d{1,6}$/

\$\endgroup\$
7
  • \$\begingroup\$ Ok, this helped me a lot, but i have other question how can i do a regular expression and cannot pass if i just put 1 like id card \$\endgroup\$ Nov 22, 2017 at 17:03
  • \$\begingroup\$ I'm not sure what you are asking. 1 will not be matched by this pattern. 1-1-1 would be a match. I suppose I am asking: "What is your concern?" \$\endgroup\$ Nov 22, 2017 at 23:45
  • \$\begingroup\$ i want to improve the regular expression \$\endgroup\$ Dec 20, 2017 at 13:08
  • \$\begingroup\$ ...the suspense is killing me. \$\endgroup\$ Dec 21, 2017 at 12:58
  • \$\begingroup\$ not really your code is code i fixed the method thank you \$\endgroup\$ Dec 21, 2017 at 14:55
3
\$\begingroup\$

Validation error when iterating over array elements

I noticed the method validateIdIsArray has a foreach:

public function validateIdIsArray($cedula)
{
    $rules = ['cedula' => 'cedula'];
    foreach ($cedula as $cedulas) {
        $input = ['cedula' => $cedulas];
        $validator = Validator::make($input, $rules);
    }
    return !($validator->fails());
}

And each time $validator gets re-assigned. Thus if any of the array elements before the last one fail the validation, this code does not capture that. That could be remedied by returning false as soon as any element fails the validation.

/**
 * @param $cedula
 * @return bool
 */
public function validateIdIsArray($cedula)
{
    $rules = ['cedula' => 'cedula'];

    foreach ($cedula as $ids) {
        $input = ['cedula' => $ids];
        $validator = Validator::make($input, $rules);
        if ($validator->fails()) {
            return false;
        } 
    }

    return true;
}

Return value from validateIdIsArray() is a boolean

The code in method validateId() calls validateIdIsArray() when the parameter $cedula is an array ($validator = $this->validateIdIsArray($cedula);). When that happens, $validator should be assigned a boolean. After that, the return statement calls $validator->fails() but that should error because it would be calling the method fails() on a boolean... thus leading to that error you saw:

Call to a member function fails() on boolean

To avoid that, return the result of calling the method validateIdIsArray()

/**
 * @param $cedula
 * @return bool
 */
public function validateId($cedula)
{
    $rules = ['cedula' => 'cedula'];

    $input = ['cedula' => $cedula];
    if(is_array($cedula))
    {
        return $this->validateIdIsArray($cedula);
    } else {
        $validator = Validator::make($input, $rules);
        return !($validator->fails());
    }
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ i got the error Call to a member function fails() on boolean \$\endgroup\$ Nov 13, 2017 at 18:58
  • \$\begingroup\$ That is what I was mentioning in the second paragraph... \$\endgroup\$ Nov 13, 2017 at 19:03
  • \$\begingroup\$ how can i do it ? any example code please \$\endgroup\$ Nov 13, 2017 at 19:34
  • \$\begingroup\$ it's the same problem "call to a member function fails()" \$\endgroup\$ Nov 13, 2017 at 19:57
  • \$\begingroup\$ sorry, I was confusing the two points - see updated answer now \$\endgroup\$ Nov 13, 2017 at 20:01
3
\$\begingroup\$

Your code looks good, but I notice that you are a Portuguese speaker (as I am), so my only advice would be: Code in English.

There is a good article about idioms and programming called "Why you shouldn’t code in Spanish". In the article the author even uses PHP in the scripts as examples.

Like the old saying:

"Always code as if the person who ends up maintaining your code is a violent psychopath who knows where you live."

We should code our code in a way that the majority of programmers will understand, so in a way we should (almost always) code in english.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Sometimes I point users to this post if the explanation is not in English. For code I typically don't mind since that might be a big effort for some users depending on the length of the code being appended, but for code this short it probably is a good idea. \$\endgroup\$ Nov 13, 2017 at 18:25

Your Answer

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

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