A better way of writing a PHP function

Question

As the title says, I'm looking for a better way of writing the following PHP function as it's a very very long one. It's part of a bigger class and I'm trying to keep the amount of code as little as possible.

The reason why I needed so many cases is because the function can accept all kinds of arguments in different ways. It might not make any sense just by looking at it, but I can provide a short description of the behavior :

public function get_new_files() {

        $arguments = [];
        $args = func_get_args();
        $args_count = func_num_args();
        $files = [];
        $set = $this->methods["Public"][8];
        $public_url = $this->public_url;

        switch ($args_count) {
            case 1:

                switch (gettype($args[0])) {
                    case 'string':

                        $arguments = $this->filter_arguments_recursive($args, $args_count);

                        switch (count($arguments)) {
                            case 0:
                            case 1:
                                return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Insufficient ][ 2 Required ] ]");
                                break;
                            case 2:

                                $url = preg_replace('/set/i', $set . ":" . $arguments[0] . "," . $arguments[1], $public_url);

                                $files = [

                                    "Marketplaces" => [

                                        "Marketplace" => $arguments[0],
                                        "Categories" => [
                                            $arguments[1] => $this->fetch($url, $set)
                                        ]
                                    ]
                                ];

                                break;
                            case 3:
                            default:
                                return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Exceeded ][ 2 Required ] ]");
                                break;
                        };

                        break;
                    case 'array':

                        switch (count($args[0])) {
                            case 0:
                                return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Insufficient ][ 2 Required ] ]");
                                break;
                            case 1:

                                $arguments = $this->filter_arguments_recursive($args, $args_count);

                                switch (count($arguments)) {
                                    case 0:
                                    case 1:
                                        return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Insufficient ][ 2 Required ] ]");
                                        break;
                                    case 2:

                                        $url = preg_replace('/set/i', $set . ":" . $arguments[0] . "," . $arguments[1], $public_url);

                                        $files = [

                                            "Marketplaces" => [

                                                "Marketplace" => $arguments[0],
                                                "Categories" => [
                                                    $arguments[1] => $this->fetch($url, $set)
                                                ]
                                            ]
                                        ];

                                        break;
                                    case 3:
                                    default:
                                        return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Exceeded ][ 2 Required ] ]");
                                        break;
                                };

                                break;
                            case 2:

                                switch (gettype($args[0][0])) {
                                    case 'string':

                                        switch (gettype($args[0][1])) {
                                            case 'string':

                                                $arguments = $this->filter_arguments_recursive([ $args[0][1] ], 1);

                                                switch (count($arguments)) {
                                                    case 0:
                                                        return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Category ][ Missing ][ 1 + Required ] ]");
                                                        break;
                                                    case 1:

                                                        $url = preg_replace('/set/i', $set . ":" . $args[0][0] . "," . $arguments[0], $public_url);

                                                        $files = [

                                                            "Marketplaces" => [

                                                                "Marketplace" => $args[0][0],
                                                                "Categories" => [
                                                                    $arguments[0] => $this->fetch($url, $set)
                                                                ]
                                                            ]
                                                        ];

                                                        break;
                                                    case 2:
                                                    default:

                                                        $results = [];

                                                        foreach ($arguments as $key => $argument) {
                                                            $url = preg_replace('/set/i', $set . ":" . $args[0][0] . "," . $argument, $public_url);
                                                            array_push($results, [ $argument => $this->fetch($url, $set) ] );
                                                        };
                                                        unset($argument);

                                                        $files = [

                                                            "Marketplaces" => [

                                                                "Marketplace" => $args[0][0],
                                                                "Categories" => $results
                                                            ]
                                                        ];

                                                        break;
                                                };

                                                break;
                                            case 'array':

                                                $results = [];

                                                foreach ($args[0][1] as $key => $argument) {
                                                    $url = preg_replace('/set/i', $set . ":" . $args[0][0] . "," . $argument, $public_url);
                                                    array_push($results, [ $argument => $this->fetch($url, $set) ] );
                                                };
                                                unset($argument);

                                                $files = [

                                                    "Marketplaces" => [

                                                        "Marketplace" => $args[0][0],
                                                        "Categories" => $results
                                                    ]
                                                ];

                                                break;
                                            default:
                                                return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Unknown Type ][ String Required ] ]");
                                                break;
                                        };

                                        break;
                                    default:
                                        return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Unknown Type ][ String Required ] ]");
                                        break;
                                };

                                break;
                            default:
                                return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Exceeded ][ 2 Required ] ]");
                                break;
                        };

                        break;
                    default:
                        return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Unknown Type ][ String Required ] ]");
                        break;
                };

                break;
            case 2:

                switch (gettype($args[0])) {
                    case 'string':

                        switch (gettype($args[1])) {
                            case 'string':

                                $arguments = $this->filter_arguments_recursive([ $args[1] ], 1);

                                switch (count($arguments)) {
                                    case 0:
                                        return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Category ][ Missing ][ 1 + Required ] ]");
                                        break;
                                    case 1:

                                        $url = preg_replace('/set/i', $set . ":" . $args[0] . "," . $arguments[0], $public_url);
                                        $files = [

                                            "Marketplaces" => [

                                                "Marketplace" => $args[0],
                                                "Categories" => [
                                                    $arguments[0] => $this->fetch($url, $set)
                                                ]
                                            ]
                                        ];

                                        break;
                                    case 2:
                                    default:

                                        $results = [];

                                        foreach ($arguments as $key => $argument) {
                                            $url = preg_replace('/set/i', $set . ":" . $args[0] . "," . $argument, $public_url);
                                            array_push($results, [ $argument => $this->fetch($url, $set) ] );
                                        };
                                        unset($argument);

                                        $files = [

                                            "Marketplaces" => [

                                                "Marketplace" => $args[0],
                                                "Categories" => $results
                                            ]
                                        ];

                                        break;
                                };

                                break;
                            case 'array':

                                $results = [];

                                foreach ($args[1] as $key => $argument) {
                                    $url = preg_replace('/set/i', $set . ":" . $args[0] . "," . $argument, $public_url);
                                    array_push($results, [ $argument => $this->fetch($url, $set) ] );
                                };
                                unset($argument);

                                $files = [

                                    "Marketplaces" => [

                                        "Marketplace" => $args[0],
                                        "Categories" => $results
                                    ]
                                ];

                                break;
                            default:
                                return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Unknown Type ][ String Required ] ]");
                                break;
                        };

                        break;
                    default:
                        return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Unknown Type ][ String Required ] ]");
                        break;
                };

                break;
            default:
                return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Exceeded ][ 2 Required ] ]");
                break;
        };

        return $this->json_encode_f_object([ "Items" => [ "New" => $files ] ]);
    }

Description : Well, the main purpose of this function is to get the newest items published on some website ( through their API ). The function needs two arguments as a starting point : a marketplace and a category ;

Based on those arguments, I get a JSON back with some info. What I do before doing that is filtering the arguments :

• first case would be that the user simply gives the function one string which contains the market and the category to ( only one category and market at a time is accepted ) separated by whitespace, comma, etc. So what I do is split that string and return an array with the two values and then fire the actual GET;
• another case would be that the user gives two strings, then do the above again to make sure I don't have whitespace or more arguments in each string
• then, the user can also pass an array with one string which meets the same conditions as the first case, or with two strings which again meets the same case as the first, but if the second string contains more than one word ( separated again by whitespace, comma, etc. ) then do a GET for the same market but with each category

I could go on with the description and be more specific, but I think the above covers the bigger picture and why I have so many cases :)

I hope someone has a better view of how I could approach this as I haven't coded PHP in some years now so I'm a bit rusty :)

Function Call Example : I think it would make sense to paste an example of how this could be called :

$test->get_new_files_from_market("themeforest", ["wordpress", "cms-themes", "psd-templates"]);

$test->get_new_files_from_market("themeforest", "wordpress, cms-themes, psd-templates");

$test->get_new_files_from_market("themeforest", "wordpress");

$test->get_new_files_from_market("themeforest, wordpress");

$test->get_new_files_from_market(["themeforest", "wordpress"]);

The way of calling it could go on, as maybe you can figure out from the function's cases :)

Answer 1

If there's anything in this answer - concepts, words, principles, etc. - that you find confusing, please don't hesitate to ask for a clarification.

It might not make any sense just by looking at it

It's good that you can detect and acknowledge this, because it is an important warning sign.

the function can accept all kinds of arguments in different ways.

Why should it have to?

I think this function is trying to do too much and that's the main reason you're struggling to keep it simple.

Either way, here goes my analysis:

First of all, having both $args and $arguments variables is confusing. Those two names are very similar so how is a new reader of the code supposed to initially know which is which? Also, it is very easy for a person to accidentally write $arguments when he/she meant $args and vice-versa. Consider renaming one of the variables to something more obvious.

You use $args[0] a lot. You should create a new variable to hold that value (and give that variable a meaningful name).

Your function seems to (usually) only get up to two arguments. So is there really any reason to use varargs? Generally speaking, you shouldn't use func_get_args and func_num_args when normal function arguments will work.

You cover lots of use cases in this function, which makes me suspect you are assigning too much responsibility to this single function. Have you considered splitting the function into three new ones? (one for each case) This would, by itself, improve the quality of the code:

• Each function would be smaller
• Each function would have better cohesion
• Each function would be easier to understand - particularly since it should be possible to examine each of these new functions in isolation.
• It would reduce the maximum nesting level

It is unclear what filter_arguments_recursive does.
filter seems to imply that the result will have, at most, as many elements as the argument it receives.
So in $arguments = $this->filter_arguments_recursive($args, $args_count), $arguments should be at most as long as $args.
However, after this piece of code:

    switch ($args_count) {
        case 1:

we see this:

                    $arguments = $this->filter_arguments_recursive($args, $args_count);

                    switch (count($arguments)) {
                        case 0:
                        case 1:
                            return json_encode("[ Get Items [ New ][ Failed ] | Arguments [ Insufficient ][ 2 Required ] ]");
                            break;
                        case 2:

So either we have some redundant code here(case 2 shouldn't happen) or the name filter_arguments_recursive is misleading. (either way, the name could probably be improved: filter arguments? What filter is being applied here? It's not immediately obvious)

Additionally, I see some break statements after return. These are redundant. Return immediately exits the function, therefore the loop is broken right there. In fact, those breaks are never even executed.

Also, we see strings similar to "[ Get Items [ New ][ Failed ] | Arguments [ Unknown Type ][ String Required ] ]" very often. What are these strings used for? Perhaps you could isolate this in a function by itself? Or even just return an error code and let the caller code format it? Remember that ideally each function should have only a single responsibility.

json_encode_f_object What is an "F object"? Consider renaming this function to something more meaningful.

What does $this->methods["Public"] contain?

This is the low hanging fruit I can find. After solving these issues, newer ones should be exposed. It would also be helpful to see an example of this function being called.

Short version:

• Use semantically meaningful names when possible
• Avoid having multiple variables with similar names in the same function
• Try not to do too much in a single function. When in doubt, break down the function into multiple other ones.

EDIT: To address the comments

I don't find anything confusing

But would others find it confusing? And will you still think it is obvious if you come back to this code a few months in the future, when your memory isn't as fresh?

the user could call

So this is just a function that somebody else will call. Then why the focus on JSON?

Generally, we use JSON when we want to communicate with "the outside world". For instance, when I have PHP code and want to send data to JavaScript, JSON is a good option. However, as the return value of a function, I don't see the need for JSON. Why not just return normal PHP data and let the user convert it to JSON if he really wants/needs it?

The main reason why I assign that much to this function is because it is supposed to be just one method that the user could call and get whatever results

Would the user of the function be truly inconvenienced if this was split in three functions, one for each use-case you mentioned? Or would it be so bad if only one of the three formats was accepted?

if you have a comma between the words it's going to split that

That's not what "filter" usually means. "filter" means to "remove unwanted stuff" from a mix. For instance, a water filter removes the impurities from water. I'd expect an "argument filter" to remove unwanted arguments.

So what your "filter" function does, if I understand correctly, is this:

• Split when a comma appears between words: Not expected in filtering
• Strip all the extra whitespace: This can indeed be considered filtering
• Strip all repeated values (making the result unique): This, too, can be considered filtering
• Do so recursively: This part is acceptable in a "recursive filter" function.
• Return a "flat" result: Not expected in filtering.

Then:

• It is unclear what the second argument does.
• Indeed, I can't think of a good name for this function. Perhaps a better question would be "why is it doing it?" Sometimes, good names come up that way.

By the way, is there a chance the data this function receives comes from an end-user? Is the following use-case possible/expected?

$test->get_new_files_from_market($_GET["something"], $_GET["more_stuff"]);

If so, then you may want to consider asking yourself "what happens when the function is called and the arguments contain characters like '/' or '?'. You mention a GET request, so I'm just wondering if you thought about that and reviewed it under a security perspective. I'm not saying you are vulnerable, just saying you should double-check. If you are not vulnerable, add a few comments explaining why you are safe.

If the user is supposed to do some sanitization himself before passing data to the function, document that too.

$test->get_new_files_from_market("themeforest, wordpress");
$test->get_new_files_from_market(["themeforest", "wordpress"]);
$test->get_new_files_from_market("themeforest", ["wordpress"]);

Are these three supposed to do the same? (Your description seems to imply so)

If so, then I would once again consider choosing just one format for the method. I'd also try to move the $this->filter_arguments_recursive($args, $args_count); to right before the outer switch, but $arguments = $this->filter_arguments_recursive([ $args[0][1] ], 1); suggests this might not be correct.

I just noticed this:

case 3:
default:

This is unusual. Did you leave case 3 there by accident? Because in this case, case 3 will just execute the code in default.

json_encode_f_object is force encoding the array to object

Have you considered a name like json_encode_as_object?

$this->methods["Public"] is just a global array of strings. I will also consider placing all those message strings into a function or some global variable

The reason I asked was because of the line $set = $this->methods["Public"][8];. It is not clear in that context what 8 is supposed to be. (are you familiar with the concept of "magic numbers"?)

Answer 2

Just my 2 cents: I would replace those switch-case blocks with the Strategy pattern and outsource those code lines to external classes implementing the same strategy.