How can I rewrite this code to improve its clarity?

Question

Could you write this cleaner? Just a question from a newbie :)

if(isset($_GET['tid']) && trim($_GET['tid'])!==""){ $act = 'tid'; $tid = trim($_GET['tid']); }elseif(isset($_GET['fid']) && trim($_GET['fid'])!==""){ $act = 'fid'; $fid = trim($_GET['fid']); }elseif(isset($_GET['mid']) && trim($_GET['mid'])!==""){ $act = 'mid'; }elseif(isset($_GET['act']) && trim($_GET['act'])!==""){ $act = trim($_GET['act']); }else{ $act = ""; } 

Answer 1

I would do it like this:

 $tid = isset( $_GET['tid'] ) ? trim( $_GET['tid'] ) : ''; $fid = isset( $_GET['fid'] ) ? trim( $_GET['fid'] ) : ''; $mid = isset( $_GET['mid'] ) ? trim( $_GET['mid'] ) : ''; $act = isset( $_GET['act'] ) ? trim( $_GET['act'] ) : ''; if ( empty( $act ) ) // act not set, construct the act from the other GET vars { if ( !empty( $tid ) ) $act = 'tid'; else if ( !empty( $fid ) ) $act = 'fid'; else if ( !empty( $mid ) ) $act = 'mid'; } 

edit: Of course, you could make it even shorter, but the question was how to write it in order to "improve its clarity." And I understand clarity as something that makes it easier to understand what is happening in a piece of code. And I think the actual logic of the source code becomes clear with my solution.

Answer 2

I see nothing wrong with your code except for the indentation:

 if(isset($_GET['tid']) && trim($_GET['tid'])!==""){ $act = 'tid'; $tid = trim($_GET['tid']); }elseif(isset($_GET['fid']) && trim($_GET['fid'])!==""){ $act = 'fid'; $fid = trim($_GET['fid']); }elseif(isset($_GET['mid']) && trim($_GET['mid'])!==""){ $act = 'mid'; }elseif(isset($_GET['act']) && trim($_GET['act'])!==""){ $act = trim($_GET['act']); }else{ $act = ""; } 

Although perhaps you could use a feature like

 function get_non_empty($field){ return isset($_GET[$field]) && trim($_GET[$field])!='' ? $_GET[$field] : NULL; } 

Answer 3

Definitely not the “cleanest” solution, but much shorter:

 $act = ''; foreach(array('tid', 'fid', 'mid', 'act') as $a) { if(isset($_GET[$a]) && strlen(trim($_GET[$a])) > 0) { $$a = trim($_GET[$act = $a]); break; } } 

Answer 4

This is almost identical logically for what poke did (+1 for beating me), but since we are talking about clarity , I thought I would show how I would do it. I like to use FALSE instead of empty lines when it means that something is not in use. This seems like a more explicit way of saying no. Also, I rarely use non-parentheses of the if / else version, but for really short assignment operators, it’s easier for me to read.

 $tid = isset($_GET['tid']) ? trim($_GET['tid']) : FALSE; $fid = isset($_GET['fid']) ? trim($_GET['fid']) : FALSE; $mid = isset($_GET['mid']) ? trim($_GET['mid']) : FALSE; $act = isset($_GET['act']) ? trim($_GET['act']) : FALSE; if ($act){ // act not set, construct the act from the other GET vars if ($tid) $act = 'tid'; else if ($fid) $act = 'fid'; else if ($mid) $act = 'mid'; } 

Answer 5

Caution with these raw GET values. You must clear these values ​​before processing them to make sure that you get exactly what you want, especially if it is necessary to insert the values ​​into the database.

Answer 6

Here is one way. However, I would probably do something different with tid, fid, mid if I knew what they were for.

 list($act,$val) = firstValidGETIn('tid','fid','mid','act'); switch($act) { case 'act': $act = $val; break; case null : $act = ""; break; default : $$act = $val; } function firstValidGETIn() { foreach(func_get_args() as $key) { if(array_key_exists($key,$_GET) && trim($_GET[$key])) return array($key, trim($_GET[$key])); } return array(null,null); }