Is it good to have a method accepting an array of keys to return it's values? Does it break OOP concepts?

Question

Here is my ValueObject example class. I wrote this example for my OOP research. I know I could use ArrayAccess interface but I am curious abobut feedbacks about this class. Does it break OOP concepts? Is there any problem with solid principles? Any feedback is apprecitated...

<?php

namespace MidoriKocak;

/**
 * Class ValueObject
 * @package MidoriKocak
 */
class ValueObject
{
    /**
     * Private array to hold data.
     *
     * @array
     */
    private $fields;

    /**
     * @param array $fields
     * @return array
     */
    public function fields(array $fields = [])
    {
        try{
            if (empty($fields)) return $fields;
            if(empty(array_diff($fields,$this->fields()))){
                return array_intersect_key($this->fields, array_flip($fields));
            }
            else{
                throw new \Exception();
            }
        }
        catch(\Exception $e){
            echo 'Bad request: ',  $e->getMessage(), "\n";
        }
    }
}

Answer 1

In your call to array_dif() you are not passing $this->fields, but $this->fields(). This will always return an empty array, because calling $this->fields() with no parameter sets the $fields argument to an empty array by default which is then returned. I assume that this is a typo and you meant to pass $this->fields.

Also I find throwing an Exception and then catching it yourself a very weird construct. Why not replace the throw new \Exception(...) with the block of code in the catch statement? Also you do not private a message to the exception that you throw yourself and then proceed to call $e->getMessage(), which is weird.

Another oddity is that you class has no non-default constructor, so there is no way of setting any data into your private $fields field. Consider adding a constructor that takes an array of values and sets it to this field or create some public addValue() method.

Furthermore, your function should not be echoing stuff. It should do one thing and be good at that one thing. In the case that no matching keys are found in your private fields array, an empty array should be returned. Places that call this function can then decide what to do when an empty array is returned, for example like so:

if ($values != $value_object->fields([...])) {
    echo 'bad request!'.
}

Also, besides for the typehinting docblocks, your comments are very redundant. I can already see what the name of the class is, so you don't have to comment that in. Also I can already see in what package the class belongs, as you have placed it in a namespace. Finally, 'Private array to hold data' is a very meaningless comment as you can already see that it is private, that it is an array (@var array) and that it holds data, because it is a variable.

Your function fields() does not have a very good name. Your function name should be descriptive and tell the user what it does. How should I know that I should pass keys to this function, the parameter is not names $keys and the function name doesn't contain the word keys either. I would suggest a name like:

public function getValuesForKeys(array $keys) {
    // ....
}

Note how I removed the default value for the $keys parameter. What is the point of calling this function without any parameters? Calling this function without any parameters should probably fail with an error, and now it does.

Consider renaming your $fields variable to something like $values, which indicates that it is an array (plural name) and indicates, well, that it contains values, which seems natural for a class names ValueObject.

There is no reason to first perform an array_dif() before calling array_intersect_keys(), since array_interesct_keys() already returns an empty array, which, as stated by me above, is what we want to do when there are no matching values for the keys.

Taking all of this into account, the result looks something like this:

class ValueObject() {
    /*
     * @var array
     */
    private $values;

    public function __construct(array $values) {
        $this->values = $values;
    }

    /*
     * @return array
     */
    public function getValuesForKeys(array $keys) {
        return array_intersect_keys($this->values, $keys);
    }
}

Now that we have refactored/redesigned the class, you can see that it actually doesn't do anything that isn't already provided by PHP itself. All you do is create an object that is basically an array and allow the caller to get values from that array based on an array of keys, which is already possible with array_intersect_keys(). In fact it is even more limiting than an array since it only provides a very small subset of the functionalities that an actual array provides (with very small subset I mean only array_intersect_keys().

In conclusion, your entire class doesn't solve any problem nor does it perform any new/useful actions. If anything it is a restriction of the array datatype.

As a direct answer to your question, the only obvious violation of OOP is that you violated SRP (Single Responsibility Principle), because your method should not be responsible for echoing a result.

Answer 2

• fields is a bad method name, as it doesn't really describe what the method does. Methods are also generally actions, and should be named as such. It's also quite confusing to have a method be named the same as its argument.
• Your documentation doesn't really help. I already know that a private field is private and holds data. What I don't know is what fields does, so an extensive comment here would be a lot more helpful.
• Your class could also use some more documentation. What is it actually used for? Why do you have a generic fields array instead of the specific fields that you need?
• Don't echo anything in the model. It makes no sense, it makes the model difficult to reuse, and it makes it difficult to control what is printed to the user.

Answer 3

Initial thoughts...

• this * @array should be * @var array
• this line does not look right if(empty(array_diff($fields,$this->fields()))){ - trying to check a condition of the parameter coming in against the method you are already in?
• throw empty exception is not good, but also immediately catch it, and do an echo is not the right design
• the return type is an array, should always return an array even if empty
• the value object is doing too much, this looks more like a domain object