views:

60

answers:

2

Hi,

I have a requirement to accept an array of checked items from a table and update a field based on which items have been selected. Initially my thoughts where to simply loop over each of these items in the array and access a function in the specific class to update the status.

I'm slightly concerned about this approach as it would mean instantiating an object for each iteration of the loop in order to update the relevant status.

foreach($example as $exampleId){
    $newExample=new Example($exampleId);
    $newExample->updateStatus('active');
}

Are there any better ways around this? This seems like bad practice but I'm struggling to find an alternative way.

+1  A: 

is this an option?

$newExample=new Example();
foreach($example as $exampleId){
    $newExample->updateStatus($exampleId,'active');
}

otherwise you could always do this:

foreach($example as $exampleId){
    $newExample=new Example($exampleId);
    $newExample->updateStatus('active');
    $newExample->__destruct(); 
    unset($newExample);
}

for this you would need anothe method in your class

$newExample=new Example();
foreach($example as $exampleId){
    $newExample->set_example_id($exampleId);
    $newExample->updateStatus('active');
}
Christian Smorra
Hmmm,not really because my constructor initially accepts a $exampleId in order to pre-populate some properties.$newExample::updateStatus() will use $newExample->exampleId to determine the relevant db record to update.Thanks for your input.
provided you with an alternative please see if this suites you
Christian Smorra
Looks good, will I'm assuming this will destroy the object before? Still though, I'll be making lots of DB calls for each instantiation.
yes it should destroy the latest object, what are you trying to do in the update status method? ill add another possibility in a sec
Christian Smorra
Update the status of the current (in loop) example and create a log item of the previous status basically. It seems like un-necessary __construct() logic just to update the status but I can't change this because of other dependencies. Thanks
The third example you've provided looks like the most viable and 'quickest' solution. Is this good practice? I.e. having all the other properties out of sync with the ->exampleId property.
I think I'm going to go with your third example, this seems the best approach.
Ooooooh example three won't work, to instantiate the class I need to pass a $exampleId to the constructor e.g. $newExample=new Example($exampleId); And whilst outside the loop I don't have a $exampleId.
i think in your case it would be far better than to instance an object every time
Christian Smorra
how about you just pass 0 or false on construction and then set the exampleid with a method?
Christian Smorra
Hmmmm I see your point Christian but this isn't good OOP practice (from what I believe).
thats certainly true but instancing your class everytime is neither :Pmaybe you could let me know what you are doing in your class constructor
Christian Smorra
You're right....I'm querying for the $exampleId details and setting some private variables for use within the class. The private variables are accessed using $this->get*** methods. It's a relatively simple SQL Join query in the constructor.
Tom Haigh's answer is a very good one but it would involve changing an aweful lot just to accommodate a status update.
i think thats general problem when coding, an aproach to solve this might be just to rename the __construct method to query_data or something and then call this methon on each loop rather than instancing an object everytime, then again this might break some dependancies
Christian Smorra
What would be the best OOP practice to this problem?
i would check in the constructor whether $exampleId is false, in that case supress the full execution of the __construct otherwise call the accual method to do what you do now in your construct.now that is just a workarround but it suppose that would be the best solution without rewriting the whole code imho.may i suggest you make this a seperate question? because there might be a better solution to this i might not know of.
Christian Smorra
Hmmm can't use the lazy load approach either!!!! Argh, looks like I'm looping - instantiating - unsetting........
i feel you pain m8 :D
Christian Smorra
+1  A: 

It sounds like creating your object has overhead because it is loading from a database or somewhere? Could you add a static method to Example which updates without having to create an object which loads and populates itself? Then you could do:

foreach($example as $exampleId){
    Example::UpdateExampleStatus($exampleId,'active');
}
Tom Haigh
This is a good idea and have thought of this myself, it would mean changing a few things because the way I do update status' at the moment is like $Example::setToActive() which accesses a protected method called $example::_updateStatus().So this will have an impact on how I access the other method as I can't access a class method within a static method.Would changing the $Example::setToActive() to public static work? And then changing $Example::updateStatus() to protected/private static.....
@user275074: I don't really know enough about your implementation. Obviously you wouldn't be able to use `$this` in those methods and you should also change any calling code to `self::method()` rather than `$this->method()`
Tom Haigh