2
\$\begingroup\$

Hey guys have a bit of code below I'm looking to optimize, the code is working, just feel it could be shorter as I had to ignore DRY principles sadly - couldn't think of a better way.

Objective: Minimize the if ($isMulti) { } else {} statement

PHPDoc:

/**
 * Syncs an array key => value pair to database column => value
 *
 * Will ignore keys where a column does not exist for which will allow you to enter a dirty array
 * as long as it has the key => value pairs that you wish to update.
 *
 * Cannot override
 *
 * @param array $data
 *
 * @returns void
 */

Code:

final protected function syncToDatabase(array $data)
{
    $isMulti = FALSE;

    $model      = $this->model;
    $identifier = $this->tableIdColumn;

    $sh = (count($data) > 0) ? $data : FALSE;

    if (!$sh) {
        return;
    }

    // determine if the array has multiple result sets
    foreach ($sh as $x) {
        if (is_array($x)) {
            $isMulti = TRUE;
            break;
        }
    }
    /////////////////////

    if ($isMulti) {
        foreach ($sh as $item) {
            $row = $model::get()
                ->filter(
                    array(
                        "{$identifier}" => $item[ $identifier ]
                    )
                )
                ->first();

            if (!is_null($row)) {
                // update existing row
                $row->update($item);
                $row->write();
            }
            else {
                // row not found insert
                $new = $model::create();
                foreach ($item as $res => $val) {
                    $new->$res = $val;
                }
                $new->write();
            }
        }
    }
    else {
        $row = $model::get()
            ->filter(
                array(
                    "{$identifier}" => $sh[ $identifier ]
                )
            )
            ->first();

        if (!is_null($row)) {
            // update existing row
            $row->update($sh);
            $row->write();
        }
        else {
            // row not found insert
            $new = $model::create();
            foreach ($sh as $res => $val) {
                $new->$res = $val;
            }
            $new->write();
        }
    }
}

Example Invocations:

$array1 = array(
    "Column1" => "Foo",
    "Column2" => "Bar",
    "Column3" => "Foobar",
);

$array2 = array(
    array(
        "Column1" => "Foo",
        "Column2" => "Bar",
        "Column3" => "Foobar",
    ),
    array(
        "Column1" => "Some",
        "Column2" => "Random",
        "Column3" => "Values",
    )
);

syncToDatabase($array1);
syncToDatabase($array2);
\$\endgroup\$
  • \$\begingroup\$ Just a simple note: why "{$identifier}" and not simply $identifier? They should be the same, beside the type. If you want string specifically, use (string) $identifier which is more expressive. \$\endgroup\$ – vfsoraki Aug 30 '16 at 5:25
  • \$\begingroup\$ They are the same definitely, the only explicit need for {$var} is where you would need {$var}_concat_me. However the use of it here is for readability as most IDE's highlight them differently. \$\endgroup\$ – zanderwar Aug 30 '16 at 5:34
1
\$\begingroup\$

Simplifying

    $sh = (count($data) > 0) ? $data : FALSE;

    if (!$sh) {
        return;
    }

Why not just say

    if (empty($data)) {
        return;
    }

    $sh = data;

There doesn't seem any point to assigning a value purely to check it on the next line.

I'm also not entirely convinced that you need $sh. It would seem that you could just operate on $data directly.

Bug?

    foreach ($sh as $x) {
        if (is_array($x)) {
            $isMulti = TRUE;
        }
        break;
    }

It would seem that you either want to do

    foreach ($sh as $x) {
        if (is_array($x)) {
            $isMulti = TRUE;
            break;
        }
    }

Or

    # check if the elements are arrays, if the first is, they all are
    list($first) = $sh;
    $isMulti = is_array($first);

The latter is the simple version of what you are actually doing, which is checking if the first element of $sh is an array itself. The former is what I first though that you might be trying to do, which is check every element and if any are arrays, mark it as multilevel.

Note that in the latter version, you don't have to initialize $isMulti to false.

DRY with recursion

    if ($isMulti) {
        foreach ($sh as $item) {
            $row = $model::get()
                ->filter(
                    array(
                        "{$identifier}" => $item[ $identifier ]
                    )
                )
                ->first();

            if (!is_null($row)) {
                // update existing row
                $row->update($item);
                $row->write();
            }
            else {
                // row not found insert
                $new = $model::create();
                foreach ($item as $res => $val) {
                    $new->$res = $val;
                }
                $new->write();
            }
        }

So you want to either insert something or call foreach and insert each of those. The simple way would be to call the function recursively.

    if ($isMulti) {
        foreach ($sh as $item) {
            $this->syncToDatabase($item);
        }

DRY with a helper method

If you don't want to do that, then move the logic into its own method.

private function writeRow($item) {
    $row = $model::get()
        ->filter(
            array(
                "{$identifier}" => $item[ $identifier ]
            )
        )
        ->first();

    if (is_null($row)) {
        // row not found insert
        $new = $model::create();
        foreach ($item as $res => $val) {
            $new->$res = $val;
        }
        $new->write();
    }
    else {
        // update existing row
        $row->update($item);
        $row->write();
    }
}

Then you just call it like

    if ($isMulti) {
        foreach ($sh as $item) {
            $this->writeRow($item);
        }
    }
    else {
        $this->writeRow($sh);
    }

or even

    # check if the elements are arrays, if the first is, they all are
    list($first) = $data;
    if (is_array($first)) {
        foreach ($data as $item) {
            $this->writeRow($item);
        }
    }
    else {
        $this->writeRow($data);
    }
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Brilliant! You got me on the break; "bug", although wouldn't cause issues would just pointlessly iterate over the array even after it discovered multi. Perf++ \$\endgroup\$ – zanderwar Aug 29 '16 at 5:56
3
\$\begingroup\$

A function does one thing, and one thing only.

final protected function syncToDatabase(array $data)
{
    $isMulti = FALSE;

    // ...

    // determine if the array has multiple result sets
    foreach ($sh as $x) {
        if (is_array($x)) {
            $isMulti = TRUE;
            break;
        }
    }

    if ($isMulti) {
        // ...
    } else {
        // ...
    }
}

Please don't do this. You are trying to provide two different functions under a single interface. That's error prone, and not exactly necessary either.

The calling site should already know whether it's invoking this function with a single result set, or an array of such.

Use this knowledge in your API design. In your case, this means stripping your syncToDatabase of all the special handling regarding multi updates.

final protected function syncToDatabase(array $item) {
    $model      = $this->model;
    $identifier = $this->tableIdColumn;

    $row = $model::get()
        ->filter(
            array(
                "{$identifier}" => $item[ $identifier ]
            )
        )
        ->first();

    if (is_null($row)) {
        // row not found insert
        $new = $model::create();
        foreach ($item as $res => $val) {
            $new->$res = $val;
        }
        $new->write();
    }
    else {
        // update existing row
        $row->update($item);
        $row->write();
    }
}

If you know that you need multi row updates commonly, provide that by a dedicated function, only for that task:

final protected function syncToDatabaseAll(array $items) {
    foreach ($items as $item) {
        $this->writeRow($item);
    }
}

Reinventing the wheel

foreach ($item as $res => $val) {
    $new->$res = $val;
}

Why? Only 3 lines further down, you used the correct function to update a row from an array.

$row->update($item);

Even worse - what you just implemented yourself can have unitended side effects if the row object has additional properties with special meaning, which coincidentally collide with (not actually ignored) keys in the passed data.

So your logic can be written more compact and less error prone once again:

$row = $model::get()->[...]->first();

if (is_null($row)) {
    $row = $model::create();
}

$row->update($item);
$row->write();

syncToDatabase is a pretty long name, you would commonly just call the whole process persist.


Your PHPDoc comment is a lie.

Will ignore keys where a column does not exist for which will allow you to enter a dirty array as long as it has the key => value pairs that you wish to update.

This however is not what your function actually does.

For starters, you implicitly require a valid key/value pair for the primary key.

Not provided? $item[ $identifier ] will throw a warning and then default to null, as you attempt to read from a key which didn't exist before.

Neither did your comment describe in any way the "multi update" functionality. Especially the heuristic you are using to determine whether the passed array contains one or more data sets.

If only one of the (not actually ignored) additional fields is an array, the heuristic triggers and leads to a wrong handling of the argument.


Error handling?

$row->update($item);
$row->write();

Depending on where your ORM does validation, either of these two lines can yield runtime errors.

Especially in the case of the multi row persist, you need to think about how to handle the error case. If one row fails to insert/update, what to do with the rest?

Try to persist as much as possible? Inform the invoking site? And how does the ORM handle validation / SQL errors in the first place? Error codes? Exceptions?


Understanding what the database does

You've got a nasty bug / conceptual error in your code.

You code does work "fine" if the $identifier field is either entirely missing from $item, or if it contains a valid value.

Well, actually it does not work fine in the first case. Because what actually happens, is that accessing $item[ $identifier ] sets this field to null, so you are querying the database for a row with NULL as the primary key. You then continue to insert a new row with explicit null as the primary key, which the database then automatically replaces by the next sequence number. This side effect is a bug, and should be fixed.

But what happens if $item[ $identifier ] was set before, not to null, and not to an existing primary key either? You create a new row, but you did not use the sequence number generator intended by the database for that. Even being set to an empty string will most likely break this already.

Actually, when does it occur that the row, which was supposed to be updated, can't be found?

Either someone just guessed a random primary key (which should not happen), or the row was deleted in the meantime.

In the latter case - is it actually the expected behavior to re-add the formerly deleted row? Or throw an error? Un-assign the (known to be invalid) primary key and let the database assign a new one?

Either way, you certainly don't want a primary key in your table which wasn't generated by the sequence number generator. For various reasons.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for the extremely in-depth response, I should have mention the framework/ORM being used here is SilverStripe 4.4. It does most of the hard yards for me such as Will ignore keys where a column does not exist for which will allow you to enter a dirty array as long as it has the key => value pairs that you wish to update. \$\endgroup\$ – zanderwar Aug 29 '16 at 23:08

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.