Syncs an array to a database table 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 =&gt; value pair to database column =&gt; 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 =&gt; 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-&gt;model;
    $identifier = $this-&gt;tableIdColumn;

    $sh = (count($data) &gt; 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()
                -&gt;filter(
                    array(
                        "{$identifier}" =&gt; $item[ $identifier ]
                    )
                )
                -&gt;first();

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

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


Example Invocations:

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

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

syncToDatabase($array1);
syncToDatabase($array2);
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-&gt;model;
    $identifier = $this-&gt;tableIdColumn;

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

    if (is_null($row)) {
        // row not found insert
        $new = $model::create();
        foreach ($item as $res =&gt; $val) {
            $new-&gt;$res = $val;
        }
        $new-&gt;write();
    }
    else {
        // update existing row
        $row-&gt;update($item);
        $row-&gt;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-&gt;writeRow($item);
    }
}




Reinventing the wheel

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


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

$row-&gt;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()-&gt;[...]-&gt;first();

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

$row-&gt;update($item);
$row-&gt;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-&gt;update($item);
$row-&gt;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.