How can I rewrite this code to improve its clarity?

Question

Could you write this 'cleaner' ? Just a simple question from a beginner:)

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 this even shorter, but the question was how it could be written to “improve its clarity”. And I understand clarity as something that makes it more easy to understand, what happens in a part of code. And I think the actual logic behind the original code gets quite clear with my solution.

Answer 2

Definitely not the 'cleanest' solution, but a lot 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 3

I see nothing bad in your code apart from lack of 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 benefit from a function like this

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

Answer 4

Here is one way. I would however probably do something differently with the tid,fid,mid stuff if I knew what they was intended 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);
}

Answer 5

This is nearly identical logically to what poke did (+1 for poke for beating me to it), but since we're talking about clarity I thought I'd show my take on it. I like to use FALSE instead of empty strings when it means something isn't being used. It feels like a more explicit way of saying "no". Also, I rarely use the non-bracketed version of if/else but for really short assignment statements I find it way easier 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 6

Careful with those raw GET values. You should clean those values up before processing them to make sure you are getting exactly what you want, especially if this is about to insert values to a database.