0

some strange question, here is my code: I have to echo $list, but now it's NULL. the "class GuestBook" read from a file from server, then I add some new info to array and I want to return a new array (array is $list), now it display that $list is NULL

<?php

class GuestBook {
public function __construct($path)
{
    $this->path = $path;
    $this->list = $list;

}

public function getData() {
    $list = file($this->path);
    return $this->list;
}

public function append($text)
{
    $list = file($this->path);
    $list[] = $text;
    var_dump($list);
    return $this->list;
}

public function getList()
{
    var_dump($list); // NULL

}

};

$newBook = new GuestBook(__DIR__ . '/HW_db.txt');
$newBook->getData();
$newBook->append('HI!');
$newBook->getList(); // NULL??


var_dump($newBook->list); // NULL ??

What is the error here??

3
  • 2
    In your constructor you are using a $list variable, where did you defined it? Commented Apr 8, 2017 at 15:14
  • php.net/manual/en/language.oop5.decon.php - __construct($path) you're only using one argument here and 2 after. So $this->list = $list; will not pass. Commented Apr 8, 2017 at 15:15
  • i'll explain: I have a function in a class, how I have to return a variable to use it in another function in a class? I want to update a variable in a class. Commented Apr 8, 2017 at 15:22

4 Answers 4

2

Simply put, this will work.

<?php

class GuestBook {

    public $path,$list;

    public function __construct($path){
        $this->path = $path;
    }
    public function getData() {
        if(file_exists($this->path)){
            $this->list = file($this->path);
            return $this;
        }
        throw new \Exception(" Your file path is invalid ");
        return null;
    }
    public function append($text){
        $this->list[] = $text;
        return $this;
    }
    public function getList(){
        return $this->list;
    }
}
//usage

$guestbook = new GuestBook(__DIR__ . '/HW_db.txt');
$guestbook->getData()->append("HI");

var_dump($guestbook->getList());
?>

How does it work

Ok, so let's start from the top; first we declare the class name, after which we set the variables. In this case you used $path and $list, for simplicity we set them to public. If you would like to know more about variable scope, I refer to the manual.

Then we move on to the constructor, we set the path we get passed to that public $path variable, so we can use it throughout the class.

The getData() function, we check if the file exists. If it exists, we load the data into the public $list variable. If it doesn't we throw an Exception.

In order to append to that list, we just add to the array of public $list.

Then in order to get the entire list, we return $this->list in the getList() function.

With all that, you have a well functioning class.

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

Comments

1

You are using local variable, instead of a class property. Correct is:

public function getList()
{
    var_dump($this->list);
}

3 Comments

You are correct in the fact that that will return the class property, but it will still return null as he never sets it.
Thank you. I've seen an error in constructor, but missed that $this->list isn't set anyway.
Yes, it's a messy class to begin with. There is more then 1 issue.
0

What is wrong in your code?

Simply put: everything.

How to fix it

Let's analyze each piece of it and see how to correct and improve it.

class GuestBook {
    public function __construct($path)
    {
        $this->path = $path;
        $this->list = $list;
    }

The first statement of the constructor is correct, it does what the constructor should do: it initializes the class members. It would be nice for the readers of the code to also declare $path and $list on top of the class definition like this:

class GuessBook {
    private $path;
    private $list;

    public function __construct($path)
    ...

The second statement of the constructor, however, initializes the list property of the object ($this->list) with the value of variable $list that is not mentioned anywhere in the function; it doesn't exist and that means it is undefined and its value is NULL.

You can either pass $list as the second argument of the constructor (public function __construct($path, $list)) or initialize $this->list with a fixed value. From the way you use the class I think the second option is the way to go and the most appropriate value for $this->list is an empty array().


public function getData()
{
    $list = file($this->path);
    return $this->list;
}

The first statement loads the content of a file, splits it into lines and stores the array of lines into the local variable $list. Nothing wrong here. But the second statement returns the value of the list property of the current object that, let's remember, was initialized in the constructor with NULL.

I believe your intention is to store in the list property the array of lines loaded from the file. That is achieved by changing the first line of code to:

$this->list = file($this->path);

While we are here, the name of the method doesn't clearly represent what it does. Its main purpose is to load the content of the file in $this->list. A better name for it is loadData().


public function append($text)
{
    $list = file($this->path);
    $list[] = $text;
    var_dump($list);
    return $this->list;
}

First of all, the var_dump() has no place here (or everywhere). It is a debugging function, one puts it into the code, uses the data it displays and removes it from the code as soon as the code was fixed and its presence is not needed any more. I hope you let it there in the code posted here and you remove it from the real code.

This method has the same problem as getData(). It loads the data into the local variable $list that is destroyed as soon as the function completes. However, using $this->list instead of $list fixes only half of the problem.

What happens when you call append() twice in a row (after you have fixed it)? The second call discards the content of $this->list prepared by the first call. This (partially) happens because append() does two things: loads the data and appends a new line. Let getData() load the data and change append() to only append one new line:

public function append($text)
{
    $this->list[] = $text;
    return $this->list;
}

public function getList()
{
    var_dump($list); // NULL
}

The function name (getList()) says it returns a value. Instead it displays something. This is confusing. Again, there is no place for var_dump() here.

But the main problem of this function is that it attempts to use the local variable $list that is not initialized. It should probably read:

public function getList()
{
    return $this->list;
}

};

There is no need to put a semicolon (;) after a class definition. It is not an error, though. It is just an empty statement.


$newBook = new GuestBook(__DIR__ . '/HW_db.txt');
$newBook->getData();
$newBook->append('HI!');
$newBook->getList(); // NULL??

Nothing wrong here, just unexpected. A reader that doesn't know the code of class GuestBook expects $newBook->getList() returns a value and not print anything.


var_dump($newBook->list); // NULL ??

$newBook->list is NULL because the one and only time when something was stored in it is in the constructor. And the value put in it by the constructor is the value of an uninitialized variable, i.e. NULL.

More than that, the property list is accessible from outside the class because you didn't declare it and PHP created it as public the first time when it was set. In a correct OOP code, most of the times the properties are private or protected, only (some of) the methods are public.

How to make it work

Let's fix and put everything together:

class GuestBook {
    private $path;
    private $list;

    public function __construct($path)
    {
        $this->path = $path;
        $this->list = array();
    }

    public function loadData()
    {
        $this->list = file($this->path);
        return $this->list;
    }

    public function append($text)
    {
        $this->list[] = $text;
        return $this->list;
    }

    public function getList()
    {
        return $this->list;
    }
}

$newBook = new GuestBook(__DIR__ . '/HW_db.txt');
$newBook->loadData();
$newBook->append('HI!');
print_r($newBook->getList());

While this code works, it still shows several weak points. If you call loadData() again after append(), the previously appended data is discarded and the content of the file is loaded again. If you don't call loadData() at all, the content of the file is ignored.

These flaws can be easily fixed by merging the code of loadData() into the constructor and removing the method loadData() altogether. This way the class goes one step further towards a correct OOP implementation. The users of the class does not and should not care about loading or saving the data; this is an implementation detail and it's the responsibility of the class.

Also, you need a method to put the data back into the file. This can be done in the destructor of the class.

class GuestBook {
    private $path;
    private $list;

    public function __construct($path)
    {
        $this->path = $path;
        $this->list = file($this->path);
    }

    public function append($text)
    {
        $this->list[] = $text;
        return $this->list;
    }

    public function getList()
    {
        return $this->list;
    }

    public function __destruct()
    {
        file_put_contents(this->file, implode('', $this->list));
    }
}

$newBook = new GuestBook(__DIR__ . '/HW_db.txt');
$newBook->append('HI!');
print_r($newBook->getList());
unset($newBook);              // This calls the destructor 

The code still has a flaw: file() loads the content of the file, splits it into lines but each line ends with the end-of-line character ("\n"). When the data is saved back into the file, the lines are joined together. If, because of append(), the list contains entries that do not end with "\n" or entries that contain embedded newlines, the next time when the data is loaded from file the content of $this->list will be different.

My suggestion is to always make sure the elements of $this->list do not contain new line characters.

Change the constructor to remove the newlines and the destructor to put them back when it saves the data in the file. Also change append() to replace the newline characters with spaces.

The final code

class GuestBook {
    private $path;
    private $list;

    public function __construct($path)
    {
        $this->path = $path;
        $this->list = explode("\n", file_get_contents($this->path));
    }

    public function append($text)
    {
        $this->list[] = trim(str_replace("\n", ' ', $text));
        return $this->list;
    }

    public function getList()
    {
        return $this->list;
    }

    public function __destruct()
    {
        file_put_contents(this->file, implode("\n", $this->list));
    }
}

What this code doesn't do and it must do

The code above works fine in an ideal world where all the files exist and are readable and writable and nothing bad ever happens. Since we are living in the real world and not in a fairy tale, bad things sometimes happen and the code must be ready to handle them.

For the sake of simplicity, the code above doesn't handle the errors: file doesn't exist, it cannot be read or written. This can be easily accomplished by using the function file_exists() and by checking the values returned by file_get_contents() and file_put_contents().

Further reading

Read the entire section about PHP classes and objects. It doesn't replace a good introduction into OOP, though. Find and read one if you feel you need (after or while you read the PHP documentation).

The documentation of PHP functions used in the code: file(), file_get_contents(), file_put_contents(), trim(), str_replace(), explode(), implode().

Also read about the scope of variables in PHP.

Comments

-1
public function append($text) {
  $list = file($this - > path);
  $list[] = $text;
  var_dump($list);
  return $this - > getlist($list); // GET LIST
}

public function getList($list) {
  var_dump($list); // NULL
}

2 Comments

return $this->getlist($list); // GET LIST } public function getList($list) { var_dump($list); // NULL }
It does trigger the getList() function in some "correct" way. How does this solve the whole thing though? You still walk into a couple of issues with this class. Thereby, it's not how a class should be written.

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.