ansaurus

Question

Is this the correct way to do this PHP class?

Answer 1

+1 A:

Don't use a whole bunch of public variables. At worst, make it one variable, such as $profile. Then all the fields are $profile['body_type'] or whatever.

wallyk 2009-12-25 18:18:17

Using a property per field has an advantage : auto-completion in any decent IDE (your solution does not have that advantage -- at least, unless you define the right doc-blocks)

Pascal MARTIN 2009-12-25 18:20:44

What do you think about switching to use a method for each item, so I can run other code on each output such as converting numbers on number variables and on any variable that comes back from the DB as being blank, I could make the method cshow alternative text?

jasondavis 2009-12-25 18:24:23

Answer 2

+3 A:

You should look into making some kind of class to abstract this all, so that your "Profile" could extend it, and all that functionality you've written would already be in place.

You might be interested in a readymade solution - these are called object relational mappers.

You should check out PHP ActiveRecord, which should easily allow you to do something like this without writing ORM code yourself.

Other similar libraries include Doctrine and Outlet.

Veeti 2009-12-25 18:19:38

Answer 3

+4 A:

You might want to take a look at PHP's magic methods which allow you to create a small number of methods (typically "get" and "set" methods), which you can then use to return/set a large number of private/protected variables very easily. You could then have eg the following code (abstract but hopefully you'll get the idea):

class Profile
{
  private $_profile;

  // $_profile is set somewhere else, as per your original code

  public function __get($name)
  {
    if (array_key_exists($name, $this->_profile)) {
      return $this->_profile[$name];
    }
  }

  public function __set($name, $value)
  {
    // you would normally do some sanity checking here too
    // to make sure you're not just setting random variables

    $this->_profile[$name] = $value;
  }
}

As others have suggested as well, maybe looking into something like an ORM or similar (Doctrine, ActiveRecord etc) might be a worthwhile exercise, where all the above is done for you :-)

Edit: I should probably have mentioned how you'd access the properties after you implement the above (for completeness!)

$profile = new Profile;

// setting
$profile->user_name = "JoeBloggs";

// retrieving
echo $profile->user_name;

and these will use the magic methods defined above.

richsage 2009-12-25 18:24:19

+1 for magic, and using an ORM (I personnaly kind of like Doctrine) instead of re-inventing the wheel -- note that magic methods will also have to inconvenient of no auto-completion in IDE (unless, too, you use the right docblocks ;; from what I remember, Doctrine generates those, btw)

Pascal MARTIN 2009-12-25 18:26:48

there is around 50 different items for each profile page to show, a lot of these items need to be modified before showing, for example some of the dates are converted and I would like to show an alternative value if the result is empty, I beleive I could add all this functionality into your example but if I didn't use get/set should I build a class with like 50 different methods? Maybe the construct method can retrieve the data from DB then the other methods will show the results

jasondavis 2009-12-25 18:48:26

@jason you can either build it into a single object as per my example (array etc which I presume you'd get from the database as a single row), or you can have 50 individual class member variables. You could build your default values into the methods above dependant on the $name asked for, or you could create get*() methods for each item.

richsage 2009-12-25 19:28:17

Answer 4

A:

Normally, a class would be created to abstract the things you can do to an object (messages you can send). The way you created it is more like a dictionary: a one-to-one mapping of the PHP syntaxis to the database fields. There's not much added value in that: you insert one extra layer of indirection without having a clear benefit.

Rather, the class would have to contain what's called a 'state', containing e.g. an id field of a certain table, and some methods (some...) to e.g. "addressString()", "marriedTo()", ....

If you worry about performance, you can cache the fields of the table, which is a totally different concern and should be implemented by another class that can be aggregated (a Cache class or whatsoever).

The main OO violation I see in this design is the violation of the "Tell, don't ask" principle.

xtofl 2009-12-25 18:33:07

What do you think about me making like 50 different methods that process the items I need to show, for example if the item from the DB is a date, then the method for showing that date would run other code to format the way it shows and then prints it to screen, also each of these methods would check to see if the value is empty, if it is then the class method would make it show an alternative text. Is that improper?

jasondavis 2009-12-25 18:52:02

I would rather add a method to render the 'showingHtml' code, in an attempt to not expose the content of the object, but rather ask it how it would like to be displayed as e.g. a html form. This idea has been coined as the Visual Proxy pattern in one of Holub's articles. The article has a very neat explanation of some basic OO concepts, too (See http://www.javaworld.com/javaworld/jw-09-1999/jw-09-toolbox.html)

xtofl 2009-12-25 21:23:49

Answer 5

+1 A:

This looks like a Data Class to me, which Martin Fowler calls a code smell in his book Refactoring.

Data classes are like children. They are okay as a starting point, but to participate as a grownup object, they need to take some responsibility.

He points out that, as is the case here,

In the early stages these classes may have public fields. If so, you should immediately Encapsulate Field before anyone notices.

If you turn your many fields into one or several several associative arrays, then Fowler's advice is

check to see whether they are properly encapsulated and apply Encapsulate Collection if they aren't. Use Remove Setting Method on any field that should not be changed.

Later on, when you have your Profile class has been endowed with behaviors, and other classes (its clients) use those behaviors, it may make sense to move some of those behaviors (and any associated data) to the client classes using Move Method.

If you can't move a whole method, use Extract Method to create a method that can be moved. After a while, you can start using Hide Method on the getters and setters.

Ewan Todd 2009-12-25 19:08:00

Of course, this advice is fairly generic. One thing I like about your class is that you pass in `$session` instead of relying upon some global data. This is called dependency injection, and it makes it possible to give your class dummy data in a unit test.

Ewan Todd 2009-12-25 19:19:46

ansaurus

tags:

views:

answers:

Is this the correct way to do this PHP class?

related questions