Any Improvment suggestions.

tsarma

Background:
I have written a code below to accept a message from Html form.The form element has a input field with name="message".

I am trying my hand on OOPs way of doing the things, and I know there is a lots of room for improvement to what i have written below. Can you improve it for me with a explainations please.
P.S. the code below is functional,it does the work but may be in nasty way...

<?php

$var = $_POST['message'];

class Message {
public $file;
public $entry;

private function cleanIt($text) {
	return strip_tags($text);		
}

public function getFile($fileName) {
	$this->file = $fileName;	
}

public function getEntry($entry) {
	$this->entry = $this->cleanIt($entry);	
}

public function writeInto() {
  $handler = fopen($this->file,'a+');
  fwrite($handler,($this->entry."\n")); 
  fclose($handler);
}

public function readFrom() {
	$arr = file($this->file);
	$text;
	foreach($arr as $val)
		$text .= $val.'<br />';
	echo $text;
}

}

$Message = new Message;
$Message -> getFile('entries.txt');
$Message -> getEntry($var);
$Message -> writeInto();
$Message -> readFrom();

Thanks

laserlight

Please place your code in php bbcode tags, and remember to indent it properly.

tsarma

OK thanks for the Tip.
Anyway my question remains same as above the code is as follows.

<?php
$var = $_POST['message'];

class Message {
      public $file;
      public $entry;

  private function cleanIt($text) {
        return strip_tags($text);
  }

  public function getFile($fileName) {
        $this->file = $fileName;
  }

  public function getEntry($entry) {
        $this->entry = $this->cleanIt($entry);
  }

  public function writeInto() {
        $handler = fopen($this->file,'a+');
        fwrite($handler,($this->entry."\n"));
        fclose($handler);
  }

  public function readFrom() {
        $arr = file($this->file);
        $text;
        foreach($arr as $val)
        $text .= $val.'<br />';
        echo $text;
  }
}

$Message = new Message;
$Message -> getFile('entries.txt');
$Message -> getEntry($var);
$Message -> writeInto();
$Message -> readFrom();
?>

laserlight

Unless you have a good reason to do otherwise, give your member variables private access. This discourages direct tampering with your member variables, allowing you to change your internal implementation without requiring client code to change. In OOP marketing speak, this is called encapsulation (and/or data hiding, depending on who you talk to and how accurate you want to be).
Before writing a function, consider if there is good reason to have it. In this case the cleanIt() member function is just a wrapper for strip_tags(), so it is unnecessary overhead and should be removed. On the other hand, if you envision that more work may need to be done to cleanup the message, leaving cleanIt() as a member function that functions as a simple wrapper is okay. Either way it makes no difference to client code since it is implementation detail.
Member functions of the form getX() typically return the value of the X member variable. In your class, getFile() and getEntry() set member variables instead. This goes against convention, so you should either rename them, or make them return instead of set.
Suppose someone creates a Message object and then tries to call writeInto(). Clearly the member variables have not been set to the appropriate values, so the object is actually in an uninitialised state. To avoid this, we could require the client code to set the filename and entry value at the object's construction by providing a constructor. If we want the client code to have the option of changing these values later on, we would also provide setters. If not, the class would remain immutable, and perhaps we only provide getters() (as in point #3).
You could consider [man]file_put_contents/man when implementing writeInto().
You could consider [man]file_get_contents/man when implementing readFrom(). Also, a function should do one thing and do it well. readFrom() does two things: a) it reads from the file, b) it prints what was read. It is better to return what was read so that the client code has the option of deciding what to do with what was read.
Incoming variables should be checked with [man]isset[/man] or [man]empty[/man] before use.

As a wrap up, I would suggest the cumulative code improvements as:

<?php

class Message {
    private $file;
    private $entry;

public function __construct($file, $entry) {
    $this->file = $file;
    $this->entry = strip_tags($entry);
}

public function getFile() {
    return $this->file;
}

public function getEntry() {
    return $this->entry;
}

public function writeInto() {
    file_put_contents($this->file, $this->entry, FILE_APPEND);
}

public function readFrom() {
    return file_get_contents($this->file);
}
}

if (isset($_POST['message'])) {
    $message = new Message('entries.txt', $_POST['message']);
    $message->writeInto();
    echo $message->readFrom();
}

?>

tsarma

Thanks for the insight you gave me. This helped me realize how one should think before writing some functions.Hope others(newbie) too learn from this simple post.