1
\$\begingroup\$

I'm writing this method that elaborates an image name..

I think my logic is not being the best one here but I'm not very savvy in programming yet.

Right now it looks like this:

if($this->isCustom($product)){
            $image_path = PRODUCT_IMAGE_PATH . $this->order['order_number'] . $index . '.jpg';
            $index++;
        }
        else{
            $image_path = PRODUCT_IMAGE_PATH . $product['sku'] . '.jpg';
        }

        if($this->checkRemoteImage($image_path)){
            return $image_path;
        }

        else{
            if (!$this->isCustom($product)){
                $this->notifyOnNonCustoms($image_path);
            }
            return "No Image";
        }

I believe something like this would be better:

if ($this->isCustom($product)){
            $image_path = PRODUCT_IMAGE_PATH . $this->order['order_number'] . $index . '.jpg';
            $index++;

            if (!$this->checkRemoteImage($image_path)){
                return "No Image";
            }
            return $image_path;
        }

        else{
            $image_path = PRODUCT_IMAGE_PATH . $product['sku'] . '.jpg';

            if (!$this->checkRemoteImage($image_path)){
                $this->notifyOnNonCustoms($image_path);
                return "No Image";
            }
            return $image_path;
        }
    }

The flow is meant to be like this:

If the product is custom, the image path should be the path + order number + ascending numbers ( using outer index here )

If not, it should be the image path + the product's sku

There should be a check if the image does exist in the server. If it doesn't it should return "No Image" and if it's not custom, call the method notifyOnNonCustoms I'm not sure on the best way to structure this.

\$\endgroup\$
6
  • \$\begingroup\$ Have you tested both versions? Are one version's results more correct than the other? \$\endgroup\$
    – Phrancis
    Commented Mar 21, 2016 at 18:27
  • \$\begingroup\$ I haven't finished testing the 2nd one but the first one saved the path into a file twice ( it should do it once ). \$\endgroup\$
    – Onilol
    Commented Mar 21, 2016 at 18:31
  • \$\begingroup\$ In that case your code is not working as intended and thereby broken. I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. Code Review is not the site to ask for help in fixing or changing what your code does. Once the code does what you want, we would love to help you do the same thing in a cleaner way! Please see our help center for more information. \$\endgroup\$
    – Mast
    Commented Mar 21, 2016 at 18:33
  • \$\begingroup\$ I'm not here to help fix it.. I'm just asking for help in writing it better. \$\endgroup\$
    – Onilol
    Commented Mar 21, 2016 at 18:35
  • 1
    \$\begingroup\$ Yes, m8. It does. I'm only afraid that my logic in this case might not be the best. \$\endgroup\$
    – Onilol
    Commented Mar 21, 2016 at 18:50

1 Answer 1

2
\$\begingroup\$

I am going to shorten up some of your code a little bit,

if ($this->isCustom($product)){
            $image_path = PRODUCT_IMAGE_PATH . $this->order['order_number'] . $index . '.jpg';
            $index++;

            if (!$this->checkRemoteImage($image_path)){
                return "No Image";
            }
            return $image_path;
        }

        else{
            $image_path = PRODUCT_IMAGE_PATH . $product['sku'] . '.jpg';

            if (!$this->checkRemoteImage($image_path)){
                $this->notifyOnNonCustoms($image_path);
                return "No Image";
            }
            return $image_path;
        }
    }

you should allow your image path variable to live a little longer and use it to return once.

this is kind of what I was thinking

if ($this->isCustom($product)){
    $image_path = PRODUCT_IMAGE_PATH . $this->order['order_number'] . $index . '.jpg';
    $index++;

    $image_path = $this->checkRemoteImage($image_path) ? "No Image" : $image_path;
}
else{
    $image_path = PRODUCT_IMAGE_PATH . $product['sku'] . '.jpg';

    if (!$this->checkRemoteImage($image_path)){
        $this->notifyOnNonCustoms($image_path);
        $image_path = "No Image";
    }
}   
return $image_path;

a little less indentation inside the if statement block. I couldn't do much with the else statement block.


Your first set of code is nice in that it creates the path and checks it, but it evaluates

$this->isCustom($product)

twice in some circumstances, and you don't want that because it adds more operations to your algorithm.

Your second set of code is much more precise, it still only creates the $image_path once, but does it differently depending on the value of the truthy statement. also you have different operations that need to be done for custom and not custom if the image path doesn't exist.

Your second block of code is better than the first.

\$\endgroup\$
1
  • \$\begingroup\$ It seems wiser to return the image_path outside the conditionals. Thanks. \$\endgroup\$
    – Onilol
    Commented Mar 21, 2016 at 19:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.