This could be further simplified by not having the entire clause negated.

This feels weird to me.

// One would normally write A.
if (!isValid($r, $b, $b)) return null;
else return toHex($r, $g, $b);
// or B.
if (isValid($r, $b, $b)) return toHex($r, $g, $b);
else return null;
// but not C.
if (isInvalid($r, $b, $b)) return null;
else return toHex($r, $g, $b);
// and certainly not D.
if (!isInvalid($r, $b, $b)) return toHex($r, $g, $b);
else return null;

If you really want to remove the negation, the most readable IMO would be option B:

if (
    \is_int($r) && $r >= 0 && $r <= 255
    && \is_int($g) && $g >= 0 && $g <= 255
    && \is_int($b) && $b >= 0 && $b <= 255
) {
    return sprintf('%02x%02x%02x', $r, $g, $b);
}

return null;

I would agree with you if we had a function like isValid to call here, but since we don't have one, I think we shouldn't reason about readability / understandability based on a hypothetical function. Your option B would be okay from a readability perspective, but then it would no longer use an early return, which is a good convention to avoid a lot of nesting of if clauses.

So IMO it makes sense to either go with my suggestion, or introduce a separate function like your isValid that we can call here to simplify readability even more.

@ju1ius Will you be adding a function like dominant_color_is_valid_rgb()?

Hi, @westonruter.

The dominant_color_rgb_to_hex function currently has two call sites and in both of them the input is guaranteed to be valid RGB, so the validation steps are useless.

In the Imagick editor case, the ImagickPixel::getColor method guarantees that the r, g and b keys of the returned array are integers in the [0, 255] range.

In the GD editor case, the imagecolorat function returns int|false. The PHP type coercion rules and the following bit operations therefore guarantee that the $r, $g and $b variables are integers in the [0, 255] range.

Therefore my suggestion would be to just inline the sprintf calls, remove the dominant_color_rgb_to_hex function to reduce API surface, and optionally fix the error checking in the GD editor so that a WP_Error is returned instead of "000000".

Thanks for that. In 31fd033 I've added an explicit check to ensure that we only use the imagecolorat() return value if it is an int.

Also, note that in #1203 the use of PHP type hints is introduced so that this code can be simplified to:

	if ( ! (
		$red >= 0 && $red <= 255 && 
		$green >= 0 && $green <= 255 &&
		$blue >= 0 && $blue <= 255
	) ) {

I've applied that simplification with fixing the merge conflict for that PR: 7e85330