5
\$\begingroup\$

I have a class that always has 14 instances. I'm just wondering if there is a better or more effective way of doing this. In this class I also have an array that is filled by calling 4 functions multiple times. Because this class is instanced 14 times, the array is duplicated a lot. What would be a better way to do this?

The following code is just placed at the bottom of the class file.

$machine1 = new machine('ZW01001', 41, "192.168.0.223");
$machine4 = new machine('ZW01004', 37, "192.168.0.216");
$machine5 = new machine('ZW01005', 28, "192.168.0.222 ");
$machine6 = new machine('ZW01006', 38, "192.168.0.217");  
$machine7 = new machine('ZW01007', 30, "192.168.0.16");
$machine8 = new machine('ZW01008', 31, " 192.168.0.220");
$machine9 = new machine('ZW01009', 32, "192.168.0.218");
$machine10 = new machine('ZW01010', 40, "192.168.0.221");
$machine21 = new machine('ZW01021', 13, "192.168.0.215");
$machine22 = new machine('ZW01022', 2, "192.168.0.206");
$machine23 = new machine('ZW01023', 33, "192.168.0.213");
$machine24 = new machine('ZW01024', 34, "192.168.0.227");
$machine25 = new machine('ZW01025', 35, "192.168.0.226");
$machine26 = new machine('ZW01026', 36, "192.168.0.217");

$Cycle = (object) array(
        'Machines' => array(
            array(
                'cycle' => $machine1->Cycle(),
                'percent' => $machine1->GetM(),
                'Data' => $machine1->Data(),
                'ip' => $machine1->pingAddress(),
            ),
            array(
                'cycle' => $machine4->Cycle(),
                'percent' => $machine4->GetM(),
                'Data' => $machine4->Data(),
                'ip' => $machine4->pingAddress(),
            ),
            array(
                'cycle' => $machine5->Cycle(),
                'percent' => $machine5->GetM(),
                'Data' => $machine5->Data(),
                'ip' => $machine5->pingAddress(),
            ),
            array(
                'cycle' => $machine6->Cycle(),
                'percent' => $machine6->GetM(),
                'Data' => $machine6->Data(),
                'ip' => $machine6->pingAddress(),
            ),
            array(
                'cycle' => $machine7->Cycle(),
                'percent' => $machine7->GetM(),
                'Data' => $machine7->Data(),
                'ip' => $machine7->pingAddress(),
            ),
            array(
                'cycle' => $machine8->Cycle(),
                'percent' => $machine8->GetM(),
                'Data' => $machine8->Data(),
                'ip' => $machine8->pingAddress(),
            ),
            array(
                'cycle' => $machine9->Cycle(),
                'percent' => $machine9->GetM(),
                'Data' => $machine9->Data(),
                'ip' => $machine9->pingAddress(),
            ),
            array(
                'cycle' => $machine10->Cycle(),
                'percent' => $machine10->GetM(),
                'Data' => $machine10->Data(),
                'ip' => $machine10->pingAddress(),
            ),
            array(
                'cycle' => $machine21->Cycle(),
                'percent' => $machine21->GetM(),
                'Data' => $machine21->Data(),
                'ip' => $machine21->pingAddress(),
            ),
            array(
                'cycle' => $machine22->Cycle(),
                'percent' => $machine22->GetM(),
                'Data' => $machine22->Data(),
                'ip' => $machine22->pingAddress(),
            ),
            array(
                'cycle' => $machine23->Cycle(),
                'percent' => $machine23->GetM(),
                'Data' => $machine23->Data(),
                'ip' => $machine23->pingAddress(),
            ),
            array(
                'cycle' => $machine24->Cycle(),
                'percent' => $machine24->GetM(),
                'Data' => $machine24->Data(),
                'ip' => $machine24->pingAddress(),
            ),
            array(
                'cycle' => $machine25->Cycle(),
                'percent' => $machine25->GetM(),
                'Data' => $machine25->Data(),
                'ip' => $machine25->pingAddress(),
            ),
            array(
                'cycle' => $machine26->Cycle(),
                'percent' => $machine26->GetM(),
                'Data' => $machine26->Data(),
                'ip' => $machine26->pingAddress(),
            ),
        )
);


$fp = fopen('Rag.json', 'w');
fwrite($fp, json_encode($Cycle));
fclose($fp);
\$\endgroup\$
7
\$\begingroup\$

I've often found myself in this situation (having an instance of an object, and needing an array). A couple of examples of where one might encounter this situation:

  • DataModels that need to be mapped to a db table (for use in queries with prepared statements)
  • Objects that represent data that is coming from/or will be sent to a service through an API (XML or JSON are common formats)
  • NoSQL databases like mongodb

Either way, after blundering along and producing awful code, trying to find the best way of doing this, I've often found that extending my model from an abstract class that has a final public function toArray() is more than enough (9/10 times).
This does require you to write classes in a disciplined fashion, which actually is a good thing, but no matter...

Provided your class properties are not private, and each property has an appropriate getter, I tend to add this method:

final public function toArray($asBind = false)
{
    $properties = get_object_vars($this);//only visible, non-statics
    $array = array();
    foreach ($properties as $name => $val)
    {
        if ($asBind === true)
            $array[':'.$name] = $val;
        else
            $array[$name] = $val;
    }
    return $array;
}

If you want this applied to your case, just define the four properties you need in that array as protected or public, and the rest of the properties as private and this'll work.
Alternatively, you can add an $arrayMap property to the abstract level, that you call from the constructor:

abstract class ArrayAble
{
    private $arrayMap = array();
    final protected function setArrayMap(array $map)
    {
        $this->arrayMap = $map;
        return $this;
    }
    final public function toArray($asBind = false)
    {
        $array = array();
        foreach ($arrayMap as $name)
        {
            if ($asBind === true)
                $array[':'.$name] = $this->{$name};
            else
                $array[$name] = $this->{$name};
        }
        return $array;
    }
}
class YourModel extends ArrayAble
{
    protected $foo = 'array-value'
    private $bar = 'invisible';
    public function __construct()
    {
        $this->setArrayMap(array('foo'));
    }
}
$x = new YourModel();
var_dump($x->toArray());//array('foo' => 'array-value');

Ok, so we've got this working, but you've probably noticed that this requires you to know how the toArray method works, and how it does what it does. Chances are you'll forget sooner or later.
If someone else starts working on this code, it becomes even more likely this will bite you.

What we actually want, is for an object to behave like an array. You need a class, to offer an array-like programming interface.

Enter: Interfaces

PHP has a couple of interfaces as standard that allow you to do just that. All of the interfaces that extend from the generic (non-usable) Traversable interface.
The interfaces you might want to use are

  • ArrayAccess: Easy to implement, but makes object overloading dangerously easy
  • Iterator: Powerful, relatively easy, but requires some "constructor magic" to set up on each instance. Harder to maintain
  • IteratorAggregate: Very easy to work with, albeit at the cost of reduced performance. However: micro-optimization is the root of all evil. I'd recommend you use this interface:

    abstract class BaseClass implements IteratorAggregate
    {
        /**
         * Implement interface, abstract implementation
         * means private properties of child remain hidden
         * @return ArrayIterator
         */
        public function getIterator()
        {
            return new ArrayIterator($this);
        }
    }
    

Extend all your models from this abstract class, and you can then loop over them using a foreach construct:

class SomeClass extends Base
{
    protected $foo = 'hello';
    protected $bar = 'World';
    private $spy = 'You can\'t see me!';
}
$x = new SomeClass;
foreach ($x as $k => $v)
    echo $k, ' => ', $v, PHP_EOL;
//output:
foo => hello
bar => world

Notice, private properties remain hidden, if you need those to be visible, you'll have to override the getIterator method in the child class itself.

More details here

On the Iterator interface:
As I've mentioned above: it's slightly trickier to implement. Looking at the PHP documentation, this would seem to be an absurd claim. However: all of the examples rely on a single array storing all the data that otherwise would have been separate properties.

I won't get all boring and go into too much detail, but distinct properties are faster, safer, and enforce a coding style that gives more control/safety (through setters that check the values etc...).
An example of data-models implementing the Iterator interface, without relying on an array to carry the actual data is this class.
It's still being developed, but the child classes can all be traversed using a foreach loop. I thought I'd share this link here, in case you were thinking of implementing the Iterator interface somewhere down the line. Perhaps the code I put on github can serve as inspiration.

Other niggles

Just some minor remarks on the rest of the code:

  • As always, I urge you, and everybody else, to subscribe to the most widely accepted coding standard. It's not official (yet), but all major players subscribe to it. As should you.
  • fopen + fwrite + fclose, as you use them could easily be replaced with file_put_contents('file.json',json_encode($array));. It's 1 line instead of 3.
  • Why create all those instances, and assign them to separate variables? I take it this is not your actual code, but in case it is: arrays are meant to hold related data. All these instances represent data that belongs together. Your code should reflect that.
  • Why are you casting the outer array (assigned to $Cycle) to an instance of stdClass? You're json_encoding it. Get rid of the cast, or assign the json-encoded string to $Cycle, write it to the file, and then write $Cycle = json_decode($Cycle);. Decoding has the advantage of creating objects recursively (a cast only casts the outer array to an object).
  • Use meaningful names, for variables, properties, and methods: $machine21->Data() doesn't mean anything to me.
\$\endgroup\$
4
\$\begingroup\$

This code can indeed use some Refractoring. Different approaches could be used here:

a __toString method can not be used here as Elias pointed out in the comments

Implement a toArray() method on the Machine Object:

public function toArray()
{
    return array(
        'cycle' => $this->cycle(),
        'percent' => $this->getM(),
        'data' => $this->data(),
        'ip' => $this->pingAddress()
    );
}

And then get the array of the Machine Object:

array(
    'Machines' => array(
        $machine1->toArray(),
        $machine2->toArray(),
        ...
    )
);

Put the logic in a function:

function machineToString(Machine $machine)
{
    return array(
        'cycle' => $this->Cycle(),
        'percent' => $this->GetM(),
        'data' => $this->Data(),
        'ip' => $this->pingAddress()
    );
}

And the call the function for every object:

array(
    'Machines' => array(
        machineToString($machine1),
        machineToString($machine2),
        ...
    )
);

We could ofcourse push it even further storing all machines in an array, and then array_walk / array_map the array to create the wanted output:

$machineArray = array(
    new Machine(...), //$machine1
    new Machine(...),  //$machine2
    ...
);

$output = array_map('machineToString', $machineArray);
//or array_walk
// !note: array_walk accepts a pointer to an array and changes those values.
// array_map simply returns a new array and keeps the old one intact.
// array_walk thus uses less memory, but destroys the original value.
array_walk($machineArray, 'machineToString');

Then some other notes:

You are casting the array to an object in PHP (expensive) and thn json_encoding it. You don't have to. In JavaScript even the 'array' is just an object. machines.machine1 === machines['machine1'] and this works for 'arrays' and 'objects'. Also, because your array is of the form key => value it will automatically be encoded to JSON using the '{' object brackets because the '[' array krackets are only used without a key given.

Then another big thing:

Be consistent and stick to standards

pingAddress() method starts with a lowercase, all other with uppercase. Some methods start with get some dont. Php standards (PSR) states that methods should be camelcased and start with a lower-case character.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Using the magic __toString method is just all shades of wrong. Taken from the official documentation: "The __toString() method allows a class to decide how it will react when it is treated like a string. For example, what echo $obj; will print. This method must return a string, as otherwise a fatal E_RECOVERABLE_ERROR level error is emitted." \$\endgroup\$ Jul 22 '14 at 14:29
  • \$\begingroup\$ edited answer, didn't think strait :) thx! \$\endgroup\$
    – Pinoniq
    Jul 22 '14 at 14:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.