Help me refactor my code / make it more elegant

PHP
1

Hello,

I don’t really have a specific PHP problem that I’d like to expose in this thread. There are just a couple of patterns I keep on running and I’d like to improve my coding skills when it comes to them.

Could you help me make my code more efficient? And more “correctly indented?”

Here we go.


if($var === false) {
    $var = 'foo';
} else {
    $var = $foo;
}



if(isset($_GET[$var])) {
    	
    	
    	if(trim($_GET[$var]) == 'foor' and isset($otherVar)) {
			//code
    	}
    	
}

3 (I only need to work with array_key).



foreach($array as $k => $v) {
    if(is_string($k)){
        //code
    }
}

That’s a start :slight_smile:

Regards.

-jj. :slight_smile:

2

@jjshell

Hi, currently your examples are snippets so we are not able to see how these are used in their broader context.

Your examples 1 and 2 seem like they could be business logic. To this end a decent Pattern to look at is Parameterized Specification which is used to validate and select http://www.martinfowler.com/apsupp/spec.pdf Your third example is part of the SPL Iteratory PHP library and is very much depending in what situation you typically find yourself in when parsing and array - too little to go on hard to reduce this more minimalistically than it already is.

Regards,
Steve

3

Hi ServerStorm. Thanks for your reply, I think you’re digging too deep. I’m just wondering if there would be a way to use the ternary operator, if while() should be used instead of foreach(). I’d like to know how to make those snippets look “more professional”. Just those snippets, not the whole architecture of an application.

:slight_smile:

4

@jjshell

Hi, Yup if that is what you want then I walked right of the you-know-what end :slight_smile:

Personally I would not use the Ternary operator as many people have difficulty reading it; one tenant of Professional code is clarity. Look at some of codefrom kyberfabrikken, [B][COLOR=#0071D8][URL=“http://www.sitepoint.com/forums/member.php?230575-Michael-Morris”]Michael Morris [/COLOR][/B]or [URL=“http://www.sitepoint.com/forums/member.php?28022-lastcraft”][B][COLOR=#0071D8]lastcraft[/COLOR][/B] in the search - even if you don’t want to do OOP it is good code to look at ; they each have very nicely thought-out code examples that are powerful but simple to understand.

The ternary operand involves a condition, a result for true, and a result for false. To use the ternary approach your code would be:

$foo = new FooTastic();  
$var = ($var === false) ? 'foo' : $foo;

Which is the same thing as


$foo = new FooTastic();  
if($var === false) { 
    $var = 'foo'; 
} else { 
    $var = $foo; 
}

‘Foreach loops’ are most appropriate for looping through arrays (like you show) or objects, ‘For loops’ are used for executing a block of code n number of times, and ‘While loops’ are best used when executing a block of code until a condition evaluates to true. So in your example you have chosen the best loop for arrays.

That is the last try I will do to help on this thread, cause if I got it wrong again it is ‘three strikes and I am am out’ “Batter Up!” :wink:
Regards,
Steve

5

Yeah :slight_smile: Thanks!

Is there a nicer way to code example 2? I’m always having those nested ifs when I check if something is set and then if it’s equal to something.

6

1.)

if($var === false) {
    $var = 'foo';
} else {
    $var = $foo;
}

Correct, shorter version:

if (!$var) $var = 'foo'; else $var = $foo;

It can be better to use $var === false, because !$var will be false for 0 or ‘’ as well, depends on situation.

2.)

if(isset($_GET[$var])) {
        if(trim($_GET[$var]) == 'foor' and isset($otherVar)) {
            //code
        }
}

Incorrect use of isset, you should use array_key_exists instead, because $var is key of $_GET array, it is not variable itself.

I can’t recommend using the code below, because from your code is way more easier to understand what does the code do.

$var = ($var === false) ? 'foo' : $foo;
7 
if(isset($_GET[$var])) { 
         
         
        if(trim($_GET[$var]) == 'foor' and isset($otherVar)) { 
            //code 
        }

Bearing in mind your question is Isolated from any context, I can say that generally I’d prefer to get rid of the nested if(), and make it clear with formatting (and optionally, the extra parentheses as I have shown) that the condition is somewhat complex.

Using && instead of and also seems more implicit to me - and it in a visual scan of the code it cries out what it is doing.


if( (isset($_GET[$var])) 
 && (trim($_GET[$var]) == 'foor')
 && (isset($otherVar))
  ) { 

echo 'doing stuff'; 

}

That just makes more visual sense to me, and its much easier to add a 4th condition later (or indeed remove one) but that might just be me and I accept that not everyone will be in agreement.