Ricky55 Posted March 16, 2017 Share Posted March 16, 2017 Hi guys I only use PHP every now and again so my knowledge is basic at best. I have this code which is working. I was just wondering if there was a better way of writing this. <?php if(isset($_GET['selector'])) { $selector = $_GET['selector']; } if ($selector == 'airflow') { $href = '/folding-beds/browse-by/airflow'; $text = 'Beds with Airflow Fibre Mattresses'; } if ($selector == 'supreme') { $href = '/folding-beds/browse-by/supreme'; $text = 'Supreme Collection'; } if ($selector == '') { $href = '/folding-beds/browse-by/airflow'; $text = 'Beds with Airflow Fibre Mattresses'; } ?> Quote Link to comment Share on other sites More sharing options...
ginerjm Posted March 16, 2017 Share Posted March 16, 2017 Some points to think about: - The $selector var will not be set if there is no GET parm. This generates a warning message. You should handle it more directly. - instead of the multiple ifs, look up the switch statement and use that. Don't forget the 'breaks'. Quote Link to comment Share on other sites More sharing options...
Psycho Posted March 16, 2017 Share Posted March 16, 2017 (edited) Better is subjective. If you are only going to have a couple of options then a single if/else condition is fine. You don't need two conditions for 'airflow' and '' since you want them to do the same thing. But, if there will be more options - or if you think it may grow, a switch() condition may be better. However, one problem I do see is that there is nothing defined in the case where 'selector' is not passed or if it does not equal one of those three values. Examples below Option 1 (only two end results): if(isset($_GET['selector']) && $_GET['selector']=='supreme') { $href = '/folding-beds/browse-by/supreme'; $text = 'Supreme Collection'; } else { //Values if 'selector' not set or does not equal 'supreme' $href = '/folding-beds/browse-by/airflow'; $text = 'Beds with Airflow Fibre Mattresses'; } Option 2 (if there will be multiple results): //Need to define value conditionally on if it is set to prevent //error referencing undefined value $selector = (isset($_GET['selector']) ? isset($_GET['selector'] : false; //Switch statement to define different conditions based on $selector value switch($selector) { case 'supreme': $href = '/folding-beds/browse-by/supreme'; $text = 'Supreme Collection'; break; case 'deluxe': $href = '/folding-beds/browse-by/deluxe'; $text = 'Deluxe Collection'; break; //If $select = 'airflow' or default: if did not meet any previous conditions case 'airflow': default: $href = '/folding-beds/browse-by/airflow'; $text = 'Beds with Airflow Fibre Mattresses'; break; } Edited March 16, 2017 by Psycho Quote Link to comment Share on other sites More sharing options...
mac_gyver Posted March 16, 2017 Share Posted March 16, 2017 a data driven method that doesn't involve program logic for each choice - // define the choices in a data structure $choices = array(); $choices['airflow'] = 'Beds with Airflow Fibre Mattresses'; $choices['supreme'] = 'Supreme Collection'; // add more entries here... $base_url = '/folding-beds/browse-by/'; $selector = isset($_GET['selector']) && isset($choices[$_GET['selector']]) ? $_GET['selector'] : 'airflow'; // get the selector or default to 'airflow' $href = $base_url . $selector; $text = $choices[$selector]; Quote Link to comment Share on other sites More sharing options...
Ricky55 Posted March 16, 2017 Author Share Posted March 16, 2017 (edited) As per usual you guys didn't let me down, best forum on the web is this IMO and I'm not just saying that. You can keep your Stack Overflow with their over restrictive rules. Thanks so much guys I really mean that. I do help people with stuff I know, I'm not just one of these lazy sods. I think I'll use Pyscho's suggestion. Mac_gyver yours looks cool but I don't fully understand it :-) Once again thanks!!!! Ps There are 10 options thats why I asked! Edited March 16, 2017 by Ricky55 1 Quote Link to comment Share on other sites More sharing options...
Ricky55 Posted March 16, 2017 Author Share Posted March 16, 2017 (edited) Hi guys sorry for being a total noob but when I type this code I'm getting an error. Should this be wrapped in a if ? <?php $selector = (isset($_GET['selector']) ? isset($_GET['selector'] : false; switch($selector) { case 'supreme': $href = '/folding-beds/browse-by/supreme'; $text = 'Supreme Collection'; break; case 'deluxe': $href = '/folding-beds/browse-by/deluxe'; $text = 'Deluxe Collection'; break; case 'airflow': default: $href = '/folding-beds/browse-by/airflow'; $text = 'Beds with Airflow Fibre Mattresses'; break; } ?> Edited March 16, 2017 by Ricky55 Quote Link to comment Share on other sites More sharing options...
Psycho Posted March 16, 2017 Share Posted March 16, 2017 mac_gyver's solution is a good one too. One benefit of that solution is that you can easily change the options without having to touch the code/logic. You could even put the data in a separate (included) file. One assumption that he made in his example is that the passed value will necessarily be the value at the end of the URL and that all the URL will have the same base content. I would not make that assumption. Here is a revision of his code with some comments. // Define the choices in a data structure // This array can be defined at the top // of the page or in an included file $choices = array( 'airflow' => array( 'href' => '/folding-beds/browse-by/airflow', 'text' => 'Beds with Airflow Fibre Mattresses' ), 'deluxe' => array( 'href' => '/folding-beds/browse-by/deluxe', 'text' => 'Deluxe Collection' ), 'supreme' => array( 'href' => '/folding-beds/browse-by/supreme', 'text' => 'Supreme Collection' ) ); //Define the selection based on passed url parameter (or false if not set) $selector = isset($_GET['selector']) ? $_GET['selector'] : false; //Check if there is a value in the defined array for the $selector //If not, define the first key as the selector if(!isset($choices[$selector])) { $selector = key($choices); } //Define the text and href using the selector $href = $choices[$selector]['href']; $text = $choices[$selector]['text']; Quote Link to comment Share on other sites More sharing options...
Psycho Posted March 16, 2017 Share Posted March 16, 2017 (edited) sorry for being a total noob but when I type this code I'm getting an error. There was a typo. User this: $selector = (isset($_GET['selector'])) ? $_GET['selector'] : false; Edited March 16, 2017 by Psycho Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.