Should I be avoiding a long list of IF statements?

Question

Should I be avoiding having a long list of serialized IF statements in my code? Sometimes it seems unavoidable, but I wonder if that's just my inexperience.

For example, if you're processing an image the user has uploaded -- and you wish to give accurate feedback on any errors -- you might have something like:

if($file["size"] == 0) { 
    throw new Exception("ERROR: File was empty");
}

if (($file["type"] != "image/gif")
|| ($file["type"] != "image/jpeg")
|| ($file["type"] != "image/pjpeg")
|| ($file["type"] != "image/png")) {
    throw new Exception("ERROR: Image must be either GIF, PNG or JPEG!");
}

if ($file["size"] > 2000000) {
    throw new Exception("ERROR: Image must be than less 2MB!");
}

    if ($file["error"] > 0) {
    throw new Exception("UNKNOWN ERROR: ".$file['error']);
}

$imgDetails = getimagesize($file["tmp_name"]);

if($imgDetails['channels'] != 3){
    throw new Exception("ERROR: Image must be RGB.)";
}

if($imgDetails['0'] < 50 && $imgDetails['1'] < 50) {
    throw new Exception("ERROR: Image must be larger then 50 x 50.)";           
}

Etc. etc. etc. Until eventually the file passes all the tests and is processed.

Is this "bad practice"?

score 1 · Accepted Answer · answered Feb 26 '13 at 19:02

1

You could use in_array to shorten the type check, but generally this is good form with the exception of pushing output directly out of this function. If this done with a class, you may want to use Exceptions instead of pushing error output directly.

Return Early, Return Often.

answered Feb 26 '13 at 19:02

datasage

19,153
2
48
54

That sounds wise! :) I was reading up on Exceptions though, and this blog totally confused me. What type of Exceptions are the above...? http://blogs.msdn.com/b/kcwalina/archive/2007/01/30/exceptionhierarchies.aspx – Chuck Le Butt Feb 26 '13 at 19:14
It depends how far you want to go with it. You could get away with using just the base exception class if all you want to do is pass a message out. Setting up exception classes allows you to handle each type of exception differently, which may not be needed. – datasage Feb 26 '13 at 19:17
Thanks, I've updated the question now -- just for good measure :) – Chuck Le Butt Feb 26 '13 at 20:29

score 0 · Answer 2 · edited May 23 '17 at 12:12

0

you could make a collection of restrictions and do a code that looks over that collection for you, rather than listing arbitrary restrictions.

Although I forgot the way to do this in javascript, something similar to How do I check if an array includes an object in JavaScript?

Hopefully this kinda helped >.>

edited May 23 '17 at 12:12

Community

1
1

answered Feb 26 '13 at 18:57

Dmytro

5,068
4
39
50

1

This is PHP, not Javascript. – Barmar Feb 26 '13 at 19:08

score 0 · Answer 3 · answered Feb 26 '13 at 19:01

I find that usually the kind of if chain you're talking about can be morphed into a hashmap of functions, with some selector as their key.

If you've got a hash map that's like $processImage = { "image/gif" => function processGIF(){blah} } you can access each function in O(1) instead of O(n/2) average (assuming a normal distribution of calls which is also inaccurate). Which, you know, admittedly doesn't matter much unless it happens a whole lot.

Then you can call that function using $processImage["image/gif"](); (I think this is valid syntax, I've done a lot more JS than php in recent years.)

I do find the hashmap approach to be pretty extensible and readable too though, so it has that going for it.

There are befits to refactoring those, sure.. but in many cases the benefits to doing so are minimal/situational.

Should I be avoiding a long list of IF statements?

3 Answers3