PHP array returns nothing (OOP newbie needs some help)

Question

I am trying to create an form validator class that will check to make sure that people have typed a particular number of characters into specified fields. Here is my page:

if ($_SERVER['REQUEST_METHOD'] == 'POST') { 
    require_once('functions.php');
    require_once('formValidator.php');


    $validator = new FormValidator();
    $validator->setFirst($_POST['first']);
    $validator->setLast($_POST['last']);
    $validator->setEmail($_POST['email']);

    $errors = array();
    $validator->checkLength($errors);
    echo $errors;


    if (!empty($errors)) {
        echo '<p>' . $message . '</p>';
    }
}

// form here (code above is in PHP tags, of course)

Here is my class:

class FormValidator {

    public $first;
    public $last;
    public $email;
    public $fields_with_lengths;

    public function setFirst($first) {   
        $this->first = $first; 
    }

    public function setLast($last) {   
        $this->last = $last; 
    }

    public function setEmail($email) {   
        $this->email = $email; 
    }

    function setLengths() {
        $this->fields_with_lengths = array('first' => 2, 'last' => 2, 'email' => 8);
    }

    function checkLength($fields_with_lengths) {
        $length_errors = array();

        foreach($fields_with_lengths as $fieldname => $maxlength) {
            if (strlen(trim($_POST[$fieldname])) > $maxlength) {
                $length_errors[] = $fieldname;
            }
        }

        if (!empty($length_errors)) {
            $message = 'There were no errors';
        } else {
            $message = 'There were errors';

        }
        return $message;
    }

}

$validator = new FormValidator();
$validator->setFirst($_POST['first']);
$validator->setLast($_POST['last']);
$validator->setEmail($_POST['email']);

Can anyone see what I am doing wrong? I'd be grateful for some help. Thanks.

Mark Baker · Accepted Answer · 2010-12-14 18:14:55Z

2

You're passing an empty array into checkLength(), then iterating over it (so the for loop will never have anything to iterate over). You're returning $message from checkLength() but not assigning it to anything.

$errors = array('first' => 8,
                'last' => 8,
                'email' => 16
               );
$message = $validator->checkLength($errors);

answered Dec 14, 2010 at 18:14

Mark Baker

213k34 gold badges354 silver badges390 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

ssice Over a year ago

But someone who is starting to program in PHP should not be left with that short answer, because even if it solves the problem, does not explain why the code is not nice at all.

ssice · Accepted Answer · 2010-12-14 18:35:08Z

In PHP by default all values to functions are passed as values, not references (unless they are objects).

First of all, if you want to use a variable as a return value, you'd have to define checkLength($var) as:

function checkLength(&$fields_with_lengths) { /* ... */ }

You could in PHP4 do a call like checkLength(&$errors), but that's deprecated as of PHP5 and will throw an E_STRICT warning. Of course, you still can (but that functionality may be thrown away soon), though the "correct" way is, as said, giving it as reference.

An even more elegant way of acting, and what should really be done in PHP, is to return the $fields_with_lengths. This way:

function checkLength() {
  $var = array();
  // Your code
  return $var;
}

And in the call to the function, just say

$return = $validator->checkLength();

But that does not solve the logic of your problem.

You are iterating over a parameter which is an empty array. You are not iterating over $validator->fields_with_lengths. Even more, $fields_with_lengths, in your code is not initialized ever.

You would have to call $validator->setLengths(). A better way of doing is (you are using PHP5) declaring a constructor, which in PHP5 is done with the magic function __construct().

However, as you are just initializing the variable always with the same constant values, you can do that in the very definition, just like this:

So you would have something like:

class FormValidator {

    pubilc $first;
    pubilc $last;
    pubilc $email;
    pubilc $fields_with_lengths = array('first' => 2, 'last' => 2, 'email' => 8);

    // Rest of code
}

What's more.. ¿Why are the attributes marked as public?

OOP theory tells us to close the scope of visibility as much as possible. And as you have setters, you don't need to access the attributes directly. You can in PHP have a magic__get($var)to return a private $attribute if you want the syntactic sugar ofecho $validator->first;`.

Example:

function __get($var) {
    if(isset($this->{$var})) return $this->{$var}; // this $this->{$var} could even be written as $this->$var but I did it like that for clarity.
}

If you also want the syntactic sugar of $validator->first = "John"; you can do the same with the setter, as in:

function __set($var, $value) {
    if(isset($this->{$var})) {
        return $this->{$var} = $value;
    } return null; // Or you can throw an exception if you like
}

In PHP, as opposed to other languages, such as Java, you must explicitly use $this as the current instance of an object within a function.

So your checkLength is misdefined.

You should also do something with the return value, instead of just dropping it.

With just that minor modifications, you could fix your code with

function checkLength() {
    $length_errors = array();

    foreach($this->fields_with_lengths as $fieldname => $maxlength) {
        if (strlen(trim($_POST[$fieldname])) > $maxlength) {
            $length_errors[] = $fieldname;
        }
    }

    if (!empty($length_errors)) {
        $message = 'There were no errors';
    } else {
        $message = 'There were errors';

    }
    return array($length_errors, $message);
}

You were using a variable ($length_errors otherwise inaccessible! so you were computing data which would never be useful).

And in the call to the function, something like $validator = new FormValidator();

// You could leave the setXXX($_POST[xxx])

$validator->first = $_POST['first'];
$validator->last = $_POST['last'];
$validator->email = ($_POST['email'];

$errors = $validator->checkLength();
echo "<p> {$errors[1]} </p>";

if(!empty($errors[0])) {
    // You could do something like foreach($errors[0] as $k => $fieldname) { echo "Error in $fieldname<br />" }
}

@shummel7845 You didn't mark yet any answer as 'approved' (the grey tick at the bottom of the vote count). Doing so would cause that answer to get just beneath the answer, so other people will find the right answer to your same question faster. By the way, welcome to StackOverflow :)

Mike C · Accepted Answer · 2010-12-14 18:17:17Z

0

$errors is empty because you initialize it but never change it. $message is unset because you never assign anything to it.

answered Dec 14, 2010 at 18:17

Mike C

1,79815 silver badges18 bronze badges

Comments

Matt Huggins · Accepted Answer · 2010-12-14 18:24:16Z

0

You're defining $errors = array();, making the errors an empty array. Nowhere do you attempt to set it to any other value, thus it remains an empty array.

answered Dec 14, 2010 at 18:24

Matt Huggins

83.7k37 gold badges153 silver badges220 bronze badges

2 Comments

ssice Over a year ago

You did like me, a short answer without carefully looking at the code.

Matt Huggins Over a year ago

No, I looked. I'm assuming you're implying that it's not being set by reference in the checkLength method, but it's not. It's not being set to anything but an empty array.

Collectives™ on Stack Overflow

PHP array returns nothing (OOP newbie needs some help)

4 Answers 4

1 Comment

1 Comment

Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

1 Comment

1 Comment

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related