SitePoint Sponsor

User Tag List

Results 1 to 7 of 7
  1. #1
    SitePoint Wizard
    Join Date
    Jan 2005
    Location
    blahblahblah
    Posts
    1,447
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Help me refactor my code / make it more elegant

    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.


    1.
    PHP Code:
    if($var === false) {
        
    $var 'foo';
    } else {
        
    $var $foo;


    2.
    PHP Code:
    if(isset($_GET[$var])) {
            
            
            if(
    trim($_GET[$var]) == 'foor' and isset($otherVar)) {
                
    //code
            

            


    3 (I only need to work with array_key).
    PHP Code:

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


    That's a start

    Regards.

    -jj.

  2. #2
    Foozle Reducer ServerStorm's Avatar
    Join Date
    Feb 2005
    Location
    Burlington, Canada
    Posts
    2,699
    Mentioned
    89 Post(s)
    Tagged
    6 Thread(s)
    @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
    ictus==""

  3. #3
    SitePoint Wizard
    Join Date
    Jan 2005
    Location
    blahblahblah
    Posts
    1,447
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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.


  4. #4
    Foozle Reducer ServerStorm's Avatar
    Join Date
    Feb 2005
    Location
    Burlington, Canada
    Posts
    2,699
    Mentioned
    89 Post(s)
    Tagged
    6 Thread(s)
    @jjshell

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

    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, Michael Morris or lastcraft 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:
    PHP Code:
    $foo = new FooTastic();  
    $var = ($var === false) ? 'foo' $foo
    Which is the same thing as
    PHP Code:
    $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!"
    Regards,
    Steve
    ictus==""

  5. #5
    SitePoint Wizard
    Join Date
    Jan 2005
    Location
    blahblahblah
    Posts
    1,447
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Yeah 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. #6
    SitePoint Enthusiast jakub_polak's Avatar
    Join Date
    Jan 2012
    Location
    Slovakia
    Posts
    35
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    1.)
    PHP Code:
    if($var === false) {
        
    $var 'foo';
    } else {
        
    $var $foo;

    Correct, shorter version:

    PHP Code:
    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.)
    PHP Code:
    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.

    PHP Code:
    $var = ($var === false) ? 'foo' $foo

  7. #7
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    2.
    PHP Code:
    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.

    PHP Code:
    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.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •