[Images][Taxon] Validation for unique code during creation/edition

[tuka217]

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	no
Related tickets	-
License	MIT

[pamil]

Getting all of them will be sloooooooooow.

[pamil]

We can filter them by owner.

[pamil]

$images -> $newImages
$imagesFromDatabase -> $persistedImages
?

[pamil]

addViolationAt is deprecated, we should use buildViolation($message)->atPath($path)->addViolation().

[pamil]

Btw. isn't something like that enough? Do we even need to use the repository?

$imagesCodesUsage = array_count_values(array_map(function (ImageInterface $image) { 
  return $image->getCode(); 
}, $images));

$duplicatedImages = array_filter($images, function (ImageInterface $image) use ($imagesCodesUsage) {
  return $imagesCodesUsage[$image->getCode()] > 1;
});

foreach (array_keys($duplicatedImages) as $key) {
  $this->context->addViolationAt(sprintf('[%d].code', $key), $constraint->message);
}

[tuka217]

But first step return an warning which stopped a work of validator. And second one too.

[pamil]

We should use new to create it inside spec, mocking constraint gives us no profit.

[tuka217]

What kind of profit gives creating new constraint? It has only an message.

[pamil]

An message which is null while mocking it :)

[tuka217]

But message can be any string. In fact it should be argument with type string

[pamil]

Yup, my bad, mocking does indeed preserve the properties value (but does not run the constructor).

[tuka217]

And ok, it should be create by new. But not because that the message is null, but more because the kind of instance of ImageUnigeCode does not metter.

[pamil]

Is second image even used in this spec?

[pamil]

Should be the first argument as it's injected to the constructor.

[pjedrzejewski]

@tuka217 Is it still a WIP?

[tuka217]

Yups, 5 minutes.

[michalmarcinkowski]

I think we don't have to check the owner, as we always receive collection of images of a single "owner". The following implementation would be simpler:

$imagesCodes = [];

/** @var ImageInterface[] $images */
foreach ($images as $key => $image) {
    if (array_key_exists($imagesCodes[$image->getCode()])) {
        $this->context->addViolationAt(sprintf('[%d].code', $key), $constraint->message);
    }
    $imagesCodes[$image->getCode()] = $image;
}

[tuka217]

It won't add the violation to a first element with duplicate code.

[michalmarcinkowski]

How about the following?

$imagesCodes = [];

/** @var ImageInterface[] $images */
foreach ($images as $key => $image) {
    if (!array_key_exists($imagesCodes[$image->getCode()])) {
        $imagesCodes[$image->getCode()] = $key;
        continue;
    }

    $this->context->addViolationAt(sprintf('[%d].code', $key), $constraint->message);
    if (false !== $imagesCodes[$image->getCode()]) {
        $this->context->addViolationAt(sprintf('[%d].code', $imagesCodes[$image->getCode()]), $constraint->message);
        $imagesCodes[$image->getCode()] = false;
    }
}

[tuka217]

It will be faster than filtering?

[michalmarcinkowski]

Filtering is like for loop, so for each image you iterate over all images and the complexity is exponential O(n^2), so for 10 images we have 100 iterations (10*10). With the above code the complexity is linear O(n), so we would have only 10 iterations.

[tuka217]

Solution with "filter" has only two loops, so if you want the more readable solution I would chose this. But, please decide which one will be the best, because it the 5th change for this simple class which (btw) will be evolved in the future.

[michalmarcinkowski]

Considering only collection of images it should never be so big that the complexity will influence the performance in any noticeable way, so I'm for @pamil's solution as it's clean and easy to understand. When there will be use cases for bigger collections and performance issues we can provide less complex solution.

[michalmarcinkowski]

@tuka217 the current solution is readable but doesn't assign validation error to the first image with duplicated code.

[tuka217]

It's assign, but I changed it do your solution and please, leave it in this form. It's not so hard to understand what this method do.

[michalmarcinkowski]

@tuka217 I hate this new Github interface! My comment was to the old implementation because I didn't see the updated code... 😕

[pjedrzejewski]

Thank you Ania, good job! ;)