critique my class plz!

[felipe_lopes]

Hi there!

I'd like to get from you guys some guidance on my code. I'm kind of new to OOP on php then I don't feel like I'm programming 100% OOP...

<?php

class random_phrase {

var $file;

//
function get_num_lines() {

	$lines = file($this->file);
	$number = count($lines);

	return $number;

}

//
function random_num() {

	$lines_num = $this->get_num_lines();
	$lines_num--;
	$random_line = rand(0, $lines_num);

	return $random_line;

}

//
function show_phrase() {

	$line = $this->random_num();
	$lines = file($this->file);

	print $lines[$line];

}

}

?>

I don't like using variables inside a class without the "$this->"...Can I use the $this-> on variables even if the variable is not declared on properties?

Critique my code, plz!

[Weedpacket]

You can, but then the variables become properties of the object. There's no harm just leaving them as variables.

Or you could shorten your code, eg:

function get_num_lines() {
       return count(file($this->file));
    } 

Too many short and simple lines starts to interfere with readability - a bit like "See Spot Run. See PEBoD kill." - because it looks like there's more going on than there actually is and someone reading the code might worry about not seeing the "rest of what it's doing".

On the other hand - from the perspective of using this class in the wild, I'd consider reading the file itself into the object when it's created - rather than reading and rereading the entire file every time you want to know something like how many lines it has.

[planetsim]

<?php
class random_phrase {

var $file;
var $lines=0; 
var $totalLines=0;
//
function get_num_lines() {

    $this->lines = file($this->file);
    $this->totalLines = count($this->lines);
	return $this->totalLines;
}

//
function random_num() {
	srand(microtime() * 1000000000000);
    $lines_num = $this->get_num_lines();
    $lines_num--;
    $random_line = rand(0, $lines_num);

    return $random_line;

}

//
function show_phrase() {

    $line = $this->random_num();

    print $this->lines[$line];

}

}
?>

Your code didnt randomize the file so here is a better version of it. Its not great i probably shouldnt be returning the result in get_num_lines().

Other than that good first attempt at OOP. Maybe we make a database abstraction layer class as your next go at it.

[felipe_lopes]

Didn't think about doing a constructor for reading the file...That's a good idea Weedpacket!

Thanks for your help planetsim! The code was working fine though...It is a class to show random phrases written on a txt file.

[drawmack]

I did a little work to your class. but I didn't test my changes so there are probably bugs

Change Log:
1) Added initialization code that allows the user to speficy the file in the initialization and checks to make sure the requested file exists.

2) added lines and lines_calced variables so that you don't have to do the costly recalculation of the file size all the time.

3) changed print phrase to get phrase because good oop coding dictates that you should never print from within a class.

Other changes that should be made:
Each function should use the initialized variable to make sure the class was properly initialized before executing.

<?php

class random_phrase {

var $file,$lines,$lines_calced,$initialized,$error;

//let's add some code to initialize this class
//sample call $my_phrase = new random_phrase(file_name.txt);
function random_phrase($file) {
    $this->file = $file;
    if(file_exists($file)) {
        $initialized = TRUE;
    } else {
        $this->initialized = FALSE;
        $this->error = "Specified file does not exist";
    }
    $this->lines_calced = FALSE;
} //end initialization code

function get_num_lines() {
    $this->lines = count(file($this->file));
    $this->lines_calced = TRUE; 
    return $this->lines;
}

function random_num() {
     if(!$this->lines_calced) {
        $this->get_num_lines();
     }

$random_line = rand(0, $this->lines);

return $random_line;
}

function get_phrase() {

    $line = $this->random_num();
    $lines = file($this->file);

    return $lines[$line];

}

}

?>

[felipe_lopes]

Thanks for your contribution drawmack!

You made my class easier to understand for those who read the code I think....

I'll try to avoid printing from inside a class now on!

[Weedpacket]

Originally posted by drawmack
3) changed print phrase to get phrase because good oop coding dictates that you should never print from within a class.

Unless of course it's an output handler class 🙂

[drawmack]

Hi, my name is weedpacket and I'm an overstater of the obvious.

HI WEEDPACKET

sorry, couldn't resist - yes every rule has it's exception.

[Weedpacket]

Originally posted by drawmack
yes every rule has it's exception.

Including this one🙂

"Good evening. Your name is Weedpacket and your specialist subject tonight is 'The Bleeding Obvious'."