Forming error messages from a multidimensional associative array

Question

I'm working with an input array, which is 3 levels deep, and creating error message strings accordingly in a separate output array. Below is the code.

$e['invalid']['key'][] = 'a123';
$e['invalid']['key'][] = 'a456';
$e['invalid']['color'][] = 'red';
$e['missing']['key'][] = 'b72';
$e['missing']['color'][] = 'blue';
$e['missing']['color'][] = 'green';
echo '<pre>' . print_r($e, 1) . '</pre>';


function generateErrorMessages($e)
{
    $errors;
    foreach ($e as $type => $array)
    {
        foreach ($array as $key => $values)
        {               
            foreach ($values as $v)
            {
                switch($type)
                {
                    case 'invalid':
                                    $errors[$type][$key][] = $v . ' is not a valid ' . $key;
                                    break;
                    case 'missing':
                                    $errors[$type][$key][] = $v . ' is a required ' . $key . ' - other functions have dependencies on it';
                                    break;
                    default:        
                                    // do nothing                   
                }
            }
        }
    }
    return $errors;
}

$msgs = generateErrorMessages($e);
echo '<pre>' . print_r($msgs, 1) . '</pre>';

While this achieves the desired outcome, the nested foreach loops in the generateErrorMessages() method seem onerous and difficult to read. Is there a more concise way to generate the error messages?

Here is a screenshot of the input array:

Here is a screenshot of the output array:

Answer 1

In my opinion, this code is all right. As any explicit code, these loops are quite straightforward to follow. And making it more concise will likely make it more cryptic as well. I would only make a minor brush-up, removing the unnecessary default clause and make the proper indentation. Variable interpolation is also more natural to read, in my opinion.

Also, variables must be explicitly initialized, just a mention is not enough. If you need an empty array then you must initialize an empty array. Although in the present code the $errors variable doesn't exist before the loop, the code could evolve over time, and there could be the possibility that a variable with the same name will be used above. If you just mention it, as $errors; it will keep the previous value. If you initialize it as $errors = []; you will always know it's an empty array.

function generateErrorMessages($e)
{
    $errors = [];
    foreach ($e as $type => $array)
    {
        foreach ($array as $key => $values)
        {               
            foreach ($values as $v)
            {
                switch($type)
                {
                    case 'invalid':
                        $errors[$type][$key][] = "$v is not a valid $key";
                        break;
                    case 'missing':
                        $errors[$type][$key][] = "$v is a required $key - other functions have dependencies on it";
                        break;
                }
            }
        }
    }
    return $errors;
}

Answer 2

I agree with what "Your Common Sense" said, but I would like to have better variable names. Names that make clear what a variable contains. Seen in isolation variable names like $e, $array and $v don't really convey what they contain. Abbreviated variable names don't make your code easier to read. Also $errors does contain errors, but more precisely it contains error messages. So, I would write this as:

function convertToErrorMessages($errors)
{
    $errorMessages = [];
    foreach ($errors as $errorType => $error)
    {
        foreach ($error as $property => $values)
        {   
            $valueMessages = [];            
            foreach ($values as $value)
            {
                switch($errorType)
                {
                    case 'invalid':
                        $valueMessages[] = "$value is not a valid $property";
                        break;
                    case 'missing':
                        $valueMessages[] = "$value is a required $property".
                                           " - other functions have dependencies on it";
                        break;
                }
            }
            $errorMessages[$errorType][$property] = $valueMessages;
        }
    }
    return $errorMessages;
}

Note that I also collect all the value messages before assigning them to the error messages array. This prevents code repetition, especially when you have many error types. I also don't like long long lines, so I split those.

One thing I sometimes do when there are many nested braces, is leaving out braces that will never be useful. Like this:

function convertToErrorMessages($errors)
{
    $errorMessages = [];
    foreach ($errors as $errorType => $error)
    foreach ($error as $property => $values)
    {   
        $valueMessages = [];            
        foreach ($values as $value)
        switch($errorType)
        {
            case 'invalid':
                $valueMessages[] = "$value is not a valid $property";
                break;
            case 'missing':
                $valueMessages[] = "$value is a required $property".
                                   " - other functions have dependencies on it";
                break;
        }
        $errorMessages[$errorType][$property] = $valueMessages;
    }
    return $errorMessages;
}

This might not be to everyones liking, but I think it is acceptable or even slightly easier to read.

I also had a look as "mickmackusa" solution. A lookup array can make sense, for instance when you're using multiple languages. Also, the idea to separate the configuration from the processor code is a valid one, but for now it only seems to complicate the code (PS: mickmackusa updated his answer and it looks a lot better now).

Answer 3

Before I get started with the script polishing, I just want to voice that I don't think it makes sense to bloat/hydrate your otherwise lean data storage with redundant text. If you are planning on presenting this to the end user as a means to communicate on a "human-friendly" level, then abandon the array-like structure and write plain English sentences.

How can you make your code more manageable? I recommend a lookup array. This will allow you to separate your "processing" code from your "config" code. The processing part will be built "generally" so that it will appropriately handle incoming data based on the "specific" data in the lookup array.

By declaring the lookup as a constant (because it won't vary -- it doesn't need to be a variable), the lookup will enjoy a global scope. This will benefit you if plan to write a custom function to encapsulate the processing code. IOW, you won't need to pass the lookup into your function as an argument or use [shiver] global.

Now whenever you want to extend your error array-hydrating function to accommodate new types, you ONLY need to add a single line of code to the lookup (ERROR_LOOKUP) -- you never need to touch the processor. In contrast, a switch block will require 3 new lines of code for each new allowance. This makes scaling your script much easier, cleaner, and more concise.

Code: (Demo)

define("ERROR_LOOKUP", [
    'invalid' => '%s is not a valid %s',
    'missing' => '%s is a required %s - other functions have dependencies on it',
]);

$errors = [];
foreach ($e as $type => $subtypes) {
    if (!isset(ERROR_LOOKUP[$type])) {
        continue;
    }
    foreach ($subtypes as $subtype => $entry) {               
        foreach ($entry as $string) {
            $errors[] = sprintf(ERROR_LOOKUP[$type], $subtype, $string);
        }
    }
}
echo implode("\n", $errors);

Output:

a123 is not a valid key
a456 is not a valid key
red is not a valid color
b72 is a required key - other functions have dependencies on it
blue is a required color - other functions have dependencies on it
green is a required color - other functions have dependencies on it

Your output strings may have static text on either/both sides of the $subtype value, so sprintf() makes the variable insertion very clean and flexible. Credit to @NigelRen for suggesting this improvement to my snippet.

The continue in the outermost loop ensures that no wasted iteration occurs on deadend array data. No "do nothing" outcomes on inner loops. Alternatively, you could use array_intersect_key() to replace the conditional continue (Demo).

p.s. I have a deep-seated hatred for switch block syntax with so many break lines. This is why I often replace them with lookup arrays.

p.p.s. If you are writing an OOP structured script/project, see @slepic's post.

Answer 4

I know this question has already been answered. But what I am showing here is just response to mickmackusa's answer, which OP called a "bonus". And so let OP and everyone wandering here in future can see one possible way to avoid spoiling global scope with constants as shown in mickmackusa's answer. The code below is his solution encapsulated in a static class:

final class ErrorMessagesConverter
{
  private static $lookupTable = [
    'invalid' => '%s is not a valid %s',
    'missing' => '%s is a required %s - other functions have dependencies on it',
  ];

  private function __construct() {};

  public static function convert(iterable $input): array
  {
    $errors = [];
    foreach ($input as $type => $subtypes) {
        if (!\array_key_exists($type, self::lookupTable)) {
            continue;
        }
        foreach ($subtypes as $subtype => $entry) {               
            foreach ($entry as $string) {
                $errors[] = sprintf(self::lookupTable[$type], $subtype, $string);
            }
        }
    }
    return $errors;
  }
}

Shall I add that every $subtypes and $entry should be checked for being iterable before actualy iterating them...