Basic OOP question

[kn0wl3dg3]

getter and setter names should match the property names. This way, when other PHP programmers want to use your objects, they will know that if you have a method/function called 'set_name()', there will be a property/variable called 'name'.

 

Is this an accurate statement? Even if there's like 20+ properties? That would be 40+ methods and that's not including doing anything with those properties but setting/getting. That sounds...I don't know...against the grain.

[KevinM1]

That tends to be the way most people do it, although you can use the __get and __set magic methods to reduce boilerplate at the cost of efficiency (take a look at some of my initial thoughts/playing with the idea: http://stackoverflow.com/questions/6550550/dumb-experiement-creating-c-esque-properties-in-php).

 

That said, a common mistake many burgeoning OOP devs make is using setters and getters as a step in a larger operation.  If you're calling getProperty() only to do more work on that property, then you need to ask yourself if it makes sense for your object to have a performComplexActionWithProperty() method instead.  If you're merely treating an object as a property bag, then you're not doing OOP.

 

Also, if your object has 20+ fields, you're likely doing it wrong.  Objects should adhere to the single responsibility principle.  If, when describing what your object does, you have to use the words and or or more than once, you're trying to do too much.  Refactor the other functionality into their own classes.

[kn0wl3dg3]

That tends to be the way most people do it, although you can use the __get and __set magic methods to reduce boilerplate at the cost of efficiency (take a look at some of my initial thoughts/playing with the idea: http://stackoverflow.com/questions/6550550/dumb-experiement-creating-c-esque-properties-in-php).

 

I'll check it out thanks.

 

That said, a common mistake many burgeoning OOP devs make is using setters and getters as a step in a larger operation.  If you're calling getProperty() only to do more work on that property, then you need to ask yourself if it makes sense for your object to have a performComplexActionWithProperty() method instead.  If you're merely treating an object as a property bag, then you're not doing OOP.

 

I have those too, which led to researching the question "why all these methods" and the quote I posted. Ie. why write functions get_name, get_id, get_something_else, set_name, set_id, set_something_else when it could be done getProperty('name, id or whatever here') setProperty('name, id or whatever here')

 

You always hear people saying "you should write a function to do that" so it just seemed counter-intuitive to write out so many functions that do the same thing just to different properties. I hope I'm stating my confusion clearly.

 

Also, if your object has 20+ fields, you're likely doing it wrong.  Objects should adhere to the single responsibility principle.  If, when describing what your object does, you have to use the words and or or more than once, you're trying to do too much.  Refactor the other functionality into their own classes.

 

Alot of things hold that much data, think of a recipe for example, or a report. A recipe would have more than 10 properties that I can think of offhand, that just store info. I don't see how breaking that up into other classes would be useful. A report would be different. It would need to take all kindfs of data and perform processes on them. I can see where breaking it out then would help.

 

I'm not arguing, I swear! Just trying to learn the proper way to do things.

 

Thanks so much for your helpful response.

[KevinM1]

Again, it all comes down to what an object is trying to do.  A recipe is essentially a data type in and of itself, as it both represents data (ingredients) and the actions done to that data (the process of cooking).  It naturally adheres to the single responsibility principle.

 

Funny that you also mention a report, as it's sometimes used as an example of the Composite pattern....

[kn0wl3dg3]

Again, it all comes down to what an object is trying to do.  A recipe is essentially a data type in and of itself, as it both represents data (ingredients) and the actions done to that data (the process of cooking).  It naturally adheres to the single responsibility principle.

 

Ok, that makes sense. So I'm not too far off base on my reasoning there.

 

Funny that you also mention a report, as it's sometimes used as an example of the Composite pattern....

 

I feel there's a very important hint in there somewhere...

[kn0wl3dg3]

Ok, I read a couple of articles on Composite classes. To my understanding, it's basically just an array of objects in another object, written in such a way that method calls behave the same for all objects in the list. i.e. a call to display() will determine how to get there depending on the object it's called on, but will always result in something being displayed to screen (for example).

 

Am I on course?

 

I understand the examples used in the articles (book shelves and to-do lists), but am having trouble seeing how this would be used for a report, and how I would set it up. Maybe that's my biggest issue, I don't really know how to "see" objects, or think that way. Any suggestions?

 

Thanks for the help, I appreciate it

[KevinM1]

It depends on the structure of the report in question.  Can it contain things that are like itself or other things within it (line items?).  If so, that's a Composite.

 

Have you ever played Civilization?  Armies are Composite objects.  Each unit in an army may have unique stats and abilities, but they all do the same thing, and can be used the same way, regardless of whether they're alone or in a group.  When a group of things can be treated - from the outside - as a singular thing of that same base type, that's a Composite.

 

Finally, keep in mind that patterns aren't one-size-fits-all solutions.  They're a starting point, a way to look and think about a problem, not necessarily an end point.  Moreover, patterns have different implementations depending on the actual problem at hand.

 

Your best bet is to get Zandstra's book.  It's the best primer on OOP in PHP available.

[kn0wl3dg3]

It depends on the structure of the report in question.  Can it contain things that are like itself or other things within it (line items?).  If so, that's a Composite.

 

I'm not sure, that's the part I'm having trouble recognizing I think. I would like to try it on a basic usage report, just telling an employer how many people are using the site, what topics they go to, etc.

 

Have you ever played Civilization?  Armies are Composite objects.  Each unit in an army may have unique stats and abilities, but they all do the same thing, and can be used the same way, regardless of whether they're alone or in a group.  When a group of things can be treated - from the outside - as a singular thing of that same base type, that's a Composite.

 

That was actually one of the examples used in an article (not that particular game). I do understand that part. Maybe thats part of the problem with the examples I'm finding; they're too perfect, or obvious maybe.

 

Your best bet is to get Zandstra's book.  It's the best primer on OOP in PHP available.

 

Going to do that!

 

Thanks

[KevinM1]

Yeah, I've always found that the hardest part of OOP is actually trying to define the problem I'm currently facing.  There are a lot of times when a problem doesn't 100% fit a particular pattern, or is in actuality a problem that needs to be addressed by several patterns working in concert.  That recognition can only come with experience.

[kn0wl3dg3]

That recognition can only come with experience.

 

Tell me about it

 

I would like to try it on a basic usage report, just telling an employer how many people are using the site, what topics they go to, etc.

 

If I tried this, would you be willing to look at it and tell me where I'm going wrong and how to fix it?

[ignace]

If I tried this, would you be willing to look at it and tell me where I'm going wrong and how to fix it?

 

That's why this forum exists in the first place ;-) To help/advice people when they are stuck on some code.

[kn0wl3dg3]

That's why this forum exists in the first place ;-) To help/advice people when they are stuck on some code.

 

You know what I meant 

 

But that was a dense question in retrospect

[kn0wl3dg3]

Ok, here's what I had when I came on today with my original question. It's what I thought I should be doing.

 

A little primer: the purpose of our site is for a company to offer this for they're employees and families to use. This report would show usage for that company.

 

Let me have it  (keep in mind it was a first draft, not something that is meant to go immediately into a live environment)

 

Thanks

 

<?php
require_once('employer.class.php');

class Report {

public $Employer; // Employer object - holds db id, name, contact info, etc.*
// Activations
protected $employeesactivations; // total number of activated employees*
protected $familyactivations; // total number of activated family members*
protected $totalactivations; // total number of activated employees and family*
protected $activationlist; // list of users and activation status*
// User Counts
protected $employeecount; // count of users using system*
protected $familycount; // count of family members using system*
protected $totalcount; // employees and family members*
protected $malecount; // count of male users*
protected $femalecount; // count of female users*
protected $unansweredcount; // count of unanswered users*
// Logins
protected $totallogins; // employees and family members*
protected $employeelogins; // count of employee logins*
protected $familylogins; // count of family logins*
protected $malelogins; // count of male user logins*
protected $femalelogins; // count of female user logins*
protected $unansweredlogins; // count of unanswered user logins*
// MISC
protected $populartopics; // Most clicked on topics*
public $error = false; // error message*

public function __construct($id) {
$this->Employer = new Employer($id);
$this->setPopularTopics(5);
$this->setEmployeeCount();
$this->setFamilyCount();
$this->setTotalUserCount();
$this->setEmployeeLogins();
$this->setFamilyLogins();
$this->setTotalLogins();
$this->setEmployeeActivations();
$this->setFamilyActivations();
$this->setTotalActivations();
$this->setActivationList();
$this->setMaleLoginCount();
$this->setFemaleLoginCount();
$this->setUnansweredLoginCount();
$this->setMaleUserCount();
$this->setFemaleUserCount();
$this->setUnansweredUserCount();
}

public function setProperty($prop, $value) {
$prop = strtolower($prop);
$this->$prop = $value;
}	
public function getProperty($prop) {
$prop = strtolower($prop);
return $this->$prop;
}
public function error($message = "") {
if ($message != "") {$this->error = $message;}
return $this->error;
}
private function queryDB($sql) { // this will ultimitaely be replaced with a class such as MySQLi or abstraction layer such as AdoDB
$res = mysql_query($sql);
// if theres no result, set error message and return false; otherwise return the result
if (!$res) {$this->error = "Could not access database: ".mysql_error(); return false;}
return $res;
}
private function setPopularTopics($limit) {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) {$this->error = "There was a problem with the database"; return false;}
while ($row = mysql_fetch_assoc($res)) {
	$this->populartopics[] = $row['Category'];
}
}
private function setEmployeeCount() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->setProperty('employeecount',$row[0]);	
}
private function setFamilyCount() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->setProperty('familycount',$row[0]);	
}
private function setTotalUserCount() {
$this->setProperty('totalcount', $this->employeecount + $this->familycount);
}
private function setEmployeeLogins() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->setProperty('employeelogins',$row[0]);
}
private function setFamilyLogins() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->setProperty('familylogins',$row[0]);
}
private function setTotalLogins() {
$this->setProperty('totallogins', $this->employeelogins + $this->familylogins);
}
private function setEmployeeActivations() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->setProperty('employeeactivations',$row[0]);
}
private function setFamilyActivations() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->setProperty('familyactivations',$row[0]);
}
private function setTotalActivations() {
$this->setProperty('totalactivations', $this->employeeactivations + $this->familyactivations);
}
private function setActivationList() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
while ($row = mysql_fetch_assoc($res)) {
	$status = ($row['UserName'] != "") ? "Activated" : "Not activated" ;
	$this->activationlist[] = "$row[F_Name] $row[L_Name] - $status";
}
}
private function setMaleLoginCount() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->setProperty('malelogins',$row[0]);
}

private function setFemaleLoginCount() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->setProperty('femalelogins',$row[0]);
}
private function setUnansweredLoginCount() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->setProperty('unansweredlogins',$row[0]);
}
private function setMaleUserCount() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->setProperty('malecount',$row[0]);
}
private function setFemaleUserCount() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->setProperty('femalecount',$row[0]);
}
private function setUnansweredUserCount() {
$sql = "********";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->setProperty('unansweredcount',$row[0]);
}



// THERE WOULD BE MORE FUNCTIONALITY ADDED HERE, BUT YOU CAN SEE WHERE THIS IS HEADING


}
?>

 

 

[ignace]

Your class has too much knowledge of everything. Since you are keeping track of Employee information (lastLogin, loginCount, sex, ..) and want to calculate totals, you could add an Employee and EmployeeCollection class.

 

class EmployeeCollection implements Iterator, Countable {
    public function add(Employee $e) {}
    public function getAt($offset) {}
    public function setAt(Employee $e, $at) {}
    public function removeAt($offset) {}
}

 

class Employee {
    private /*@EmployeeSex*/ $_sex = null;
    private /*@DateTime*/ $_lastLogin = null;
    private $_loginCount = 0;
}

class EmployeeSex { // who said programming isn't sexy? ;-)
    const MALE = 'm';
    const FEMALE = 'f';
    const UNKNOWN = 'yet to be determined';
    
    private $_sex = self::UNKNOWN;
    
    public function __construct($sex) {/*$sex can't be UNKNOWN*/}
    
    public function isMale() {}
    public function isFemale() {}
}

 

class Report {
    private /*@EmployeeCollection*/ $_collection = null;
}

 

You would query the collection for the necessary information:

- total number of activated employees

- total number of activated family members

- count of male users

- ..

[KevinM1]

Well, there are a couple of properties you can get rid of.  Anything that's a total of other fields can simply be generated when you access them.  So, if you have a method named getTotalLogins(), you don't need a $totalLogins property.  Simply compute the total logins in the method.

 

That said, I'm a bit confused about where the db is coming from.

 

You have your set/getProperty methods as public, which makes no sense.  These are utility methods which should not be exposed to the public.  Rather, your named setters and getters should be public, and the utility methods private or protected.

 

Ultimately, I can't help but think you're approaching it from the wrong end.  An Employer should generate a report, not the other way around.  I mean think about it - when someone passes an id to your Report constructor, what context will they assume they're in?  They'll think that the id is the Report id, not the Employer id.  And doesn't it make more sense to do something like:

 

$employer = EmployerFactory::getInstance($id); // Look up Factory Method on Google, or in Zandstra's book, which you should buy

$employer->generateReport(/* optional args for report type, or date, or something */);
// send/display report (which could be rolled into generateReport)

 

??

 

As for the rest, I like ignace's approach.  As to how his EmployeeCollection fits with an Employer and a Report, an Employer would contain an EmployeeCollection.  From that collection, the Report would be generated.

[kn0wl3dg3]

Whoa, sensory overload. I'm going to have to read through these a few times to get what you guys are throwing down

 

Ok, @ignace, I don't know what to say yet, I need to look through that post alot closer

 

@Nightslyr, same thing, but I can explain one thing.

 

A user has an admin status set in the db, either 1 or 0, 1 means they are an admin. If they are, they get access to the reporting dashboard. Each User is associated with an Employer with a fk in the db. That would be the id sent into the class in my code. When they entered the dashboard, I would use the employer id for the company they work for to produce the report. Does that change your thoughts or am I still ass-backwards? Lol

 

And here:

You have your set/getProperty methods as public, which makes no sense.  These are utility methods which should not be exposed to the public.  Rather, your named setters and getters should be public, and the utility methods private or protected.

 

This is what brought up my original question. I thought it was better to expose the one function getProperty instead of all the getters/setters. You're saying I'm doing it wrong though is what I'm hearing

[ignace]

As for the rest, I like ignace's approach.  As to how his EmployeeCollection fits with an Employer and a Report, an Employer would contain an EmployeeCollection.  From that collection, the Report would be generated.

 

Yes that was the idea but it's an early sketch since there isn't much to go on. Maybe Employer and Family should also be separate classes since either would generate a different report?

 

The report should also be flexible in that it will look different for each company or family (Composite and Decorator come to mind). You would then fill the Report with all the necessary data and use Decorators to render the report. So that by feeding different decorators you would get different reports (themes). If you have experience with Zend_Form you get what I mean.

 

A generate(RenderEngine $e) could be used to tell what rendering engine should be used, the rendering engine would then inject the decorators to create PDF, DOCX, or simple HTML documents.

 

I'm just rambling here but you can see that there are a lot of opportunities here.

[KevinM1]

This is what brought up my original question. I thought it was better to expose the one function getProperty instead of all the getters/setters. You're saying I'm doing it wrong though is what I'm hearing

 

There's a difference between creating utility methods, like you're doing here, and using PHP's built-in __get and __set magic methods (like I did in that Stack Overflow link I gave you earlier) to remove the tedious boilerplate of writing simple/dumb setters and getters altogether.  It's a fine distinction, but an important one.  Right now, you have general methods that, if accessed directly on the object, would look like:

 

$report->getProperty("employeecount");
$report->setProperty("familylogins", 44);

 

Which I'm pretty sure is not what you're after.  Compare that with using __get and __set:

 

echo $report->employeecount;
$report->familylogins = 44;

 

Keep in mind that __get and __set are only invoked if no property of that name actually exists within the class.  That's why I had the props array and no other fields in my Stack Overflow example.  Also keep in mind that my Stack Overflow example quickly falls apart if different setters or getters need to execute specialized code in order to do their jobs, like, say, access a db.  At that point, __get/__set would likely be a long switch that delegated to specialized private methods, which, really, would negate much of the point of using them.

 

Of course, all of this begs the question - do any of the Report's fields need to be directly accessed?  Don't use setters and getters because that's how everyone does it.  Ask yourself: "Do I need to be able to set/get this particular field independently from the others?"  Only code the functionality you'll actually use.

[kn0wl3dg3]

Right now, you have general methods that, if accessed directly on the object, would look like:

 

$report->getProperty("employeecount");
$report->setProperty("familylogins", 44);

 

 

Which I'm pretty sure is not what you're after. 

 

Aww, I see. That makes sense

 

 

Of course, all of this begs the question - do any of the Report's fields need to be directly accessed?  Don't use setters and getters because that's how everyone does it.  Ask yourself: "Do I need to be able to set/get this particular field independently from the others?"  Only code the functionality you'll actually use.

 

Not sure I follow.

 

The report was supposed to build itself, then I could call different properties where I wanted to display them. That way, I could only get things from the report to show, but all the building of it was done internally. I'm taking it thats not the correct way..or at least I didn't accomplish that

[kn0wl3dg3]

@ignace: in regards to your example- I understand what you're getting at I'm excited to say. The actual implementation of that understanding is a totally different thing right now though  :-\ I get what you mean, but doubt I could do it yet. I'm afraid this is all a bit above me still. I'm excited to learn it though.

 

@Nightslyr: can you explain this a little bit?

Of course, all of this begs the question - do any of the Report's fields need to be directly accessed?  Don't use setters and getters because that's how everyone does it.  Ask yourself: "Do I need to be able to set/get this particular field independently from the others?"  Only code the functionality you'll actually use.

 

Oh, and I stopped last night at my local book store to get that book and they don't carry it, gotta order it online  Big Chain Store my ass

[kn0wl3dg3]

Is this a better attempt? I know it's not what you guys are suggesting, and I do want to do it the right way, I just realized there was more basic things that I was getting wrong, and want to correct those mistakes first...baby steps. So aside from not doing a composite class or collection yet, is this a better version? Are my basics right here?

 

Thanks guys, I can't tell you how much I appreciate the time

 

<?php
require_once('class_Employer.php');
class Report {
public $Employer; // Employer object*
// Activations
protected $employeesactivations; // total number of activated employees*
protected $familyactivations; // total number of activated family members*
// User Counts
protected $employeecount; // count of users using system*
protected $familycount; // count of family members using system*
protected $malecount; // count of male users*
protected $femalecount; // count of female users*
protected $unansweredcount; // count of unanswered users*
// Logins
protected $employeelogins; // count of employee logins*
protected $familylogins; // count of family logins*
protected $malelogins; // count of male user logins*
protected $femalelogins; // count of female user logins*
protected $unansweredlogins; // count of unanswered user logins*

// MISC
public $error = false; // error message*

public function __construct($id) {
$this->Employer = new Employer($id);
$this->setEmployeeCount();
$this->setFamilyCount();
$this->setEmployeeLogins();
$this->setFamilyLogins();
$this->setEmployeeActivations();
$this->setFamilyActivations();
$this->setMaleLoginCount();
$this->setFemaleLoginCount();
$this->setUnansweredLoginCount();
$this->setMaleUserCount();
$this->setFemaleUserCount();
$this->setUnansweredUserCount();
}

// general
public function error($message = "") {
if ($message != "") {$this->error = $message;}
return $this->error;
}
private function queryDB($sql) { // this will ultimitaely be replaced with a class such as MySQLi or abstraction layer such as AdoDB
$res = mysql_query($sql);
// if theres no result, set error message and return false; otherwise return the result
if (!$res) {$this->error = "Could not access database: ".mysql_error(); return false;}
return $res;
}

// topics
// get $limit most popular topics
public function getPopularTopics($limit) {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) {$this->error = "There was a problem with the database"; return false;}
while ($row = mysql_fetch_assoc($res)) {
	$topics[] = $row['Category'];
}
return $topics;
}

// user counts
private function setEmployeeCount() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->employeecount = $row[0];	
}
public function getEmployeeCount() {
return $this->employeecount;
}
private function setFamilyCount() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->familycount = $row[0];	
}
public function getFamilyCount() {
return $this->familycount;
}
public function getTotalUserCount() {
return $this->employeecount + $this->familycount;
}
private function setMaleUserCount() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->malecount = $row[0];
}

public function getMaleUserCount() {
return $this->malecount;
}
private function setFemaleUserCount() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->femalecount = $row[0];
}
public function getFemaleUserCount() {
return $this->femalecount;
}
private function setUnansweredUserCount() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->unansweredcount = $row[0];
}
public function getUnansweredUserCount() {
return $this->unansweredcount;
}

// logins
private function setEmployeeLogins() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->employeelogins = $row[0];
}
public function getEmployeeLogins() {
return $this->employeelogins;
}
private function setFamilyLogins() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->familylogins = $row[0];
}
public function getFamilyLogins() {
return $this->familylogins;
}
public function getTotalLogins() {
return $this->employeelogins + $this->familylogins;
}
private function setMaleLoginCount() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->malelogins = $row[0];
}
public function getMaleLogins() {
return $this->malelogins;
}
private function setFemaleLoginCount() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->femalelogins = $row[0];
}
public function getFemaleLogins() {
return $this->femalelogins;
}
private function setUnansweredLoginCount() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->unansweredlogins = $row[0];
}
public function getUnansweredLogins() {
return $this->unansweredlogins;
}

// activations
private function setEmployeeActivations() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->employeeactivations = $row[0];
}
public function getEmployeeActivations() {
return $this->employeeactivations;
}
private function setFamilyActivations() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
$row = mysql_fetch_row($res);
$this->familyactivations = $row[0];
}
public function getFamilyActivations() {
return $this->familyactivations;
}
public function getTotalActivations() {
return $this->employeeactivations + $this->familyactivations;
}
public function getActivationList() {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) return false;
while ($row = mysql_fetch_assoc($res)) {
	$status = ($row['UserName'] != "") ? "Activated" : "Not activated" ;
	$activationlist[] = "$row[F_Name] $row[L_Name] - $status";
}
return $activationlist;
}
}
?>

[ignace]

1. Abstract your database, even if you make it as simple as just a Database class it allows for more flexibility to allow your report to work with more than just MySQL [OR]

2. Better yet, create a new class that uses a Database and let the report query to that new class:

 

public function getPopularTopics($limit) {
$sql = "*******";
if (($res = $this->queryDB($sql)) === false) {$this->error = "There was a problem with the database"; return false;}
while ($row = mysql_fetch_assoc($res)) {
	$topics[] = $row['Category'];
}
return $topics;
}

 

So instead of having a function getPopularTopics in your Report you would put the function in ReportModel:

 

$this->getPopularTopics();

 

Then becomes:

 

$this->reportModel->getPopularTopics();

 

*TADA* all query related material is now outside your report class and the code becomes less cluttered. Keep identifying responsibilities and figure out who should carry that responsibility, use GRASP to guide you.

 

If GRASP is going above your hat, then try to get familiar with the concept through CRC cards.

[kn0wl3dg3]

2. Better yet, create a new class that uses a Database and let the report query to that new class:

 

That was my plan, or kind of, but I would have done it wrong by your example. I planned on adding AdoDB as an abstraction layer, but it sounds like you mean more than that.

 

Pretty much all my properties except for the totals are retrieved from the db so what would the report class actually do if the ReportModel does all that?

 

I'm going to read those links; maybe the answer is in there. Onward and upward..

 

Thanks

[ignace]

The ReportModel (maybe a wrong choice of words on my part) would do all the queries and the Report (or ReportView) would do the actual rendering.