We help IT Professionals succeed at work.

Is this code repetitive?

edvinson
edvinson asked
on
It seems to me, that my code is a bit redundant. Can anyone show me a way to consolidate this?

I just think I could optimize this, but not sure how.

Thanks!
<?php
                // We need to display errors (or success messages) here
                if($this->session->flashdata('login_error'))
                {
                ?>
                <div class="alert error">
                    <span class="icon"></span><strong>Error</strong> 
                    <?php echo($this->session->flashdata('login_error'));?>
                                </div>
                <?php
                }
                ?>
                
                                <?php
                // Check for success message
                if($this->session->flashdata('register_success'))
                {
                ?>
                <div class="alert success">
                    <span class="icon"></span><strong>Success</strong> 
                    <?php echo($this->session->flashdata('register_success'));?>
                                </div>
                <?php
                }
                ?>

Open in new window

Comment
Watch Question

Project Lead
Top Expert 2011
Commented:
You can do something like this

 
  <?
              $class ="";
              $message ="";
 
                // We need to display errors (or success messages) here
                if($this->session->flashdata('login_error'))
                {
                  $class = "error";
                  $message =  $this->session->flashdata('login_error');
               
                }
                // Check for success message
                else
                {
                  $class = "success";
                  $message =  $this->session->flashdata('register_success');
                }
                ?>
               
               
                <div class="alert <?=$class?>">
                    <span class="icon"></span><strong><?=$class?></strong>
                    <?php echo($message);?>
                </div>
Most Valuable Expert 2011
Top Expert 2016
Commented:
If it works now, I would not "fix" it.  It is not well designed, and I say that because it intermixes logic if() statements with presentation in HTML code.  But it is so small a snippet with no loops, and so any change you might make cannot help much with the overhead of the system in question.

Groupings of data like this are usually handled through arrays (perhaps arrays of objects) and HEREDOC notation.  There are three elements that appear to be variable.  The controlling factor seems to be the return from $this->session->flashdata().  The dependent variables are the div class and the word inside the strong tag.  You might consider using a switch/case logic structure.  Interestingly, if that method returned TRUE for both arguments, you could wind up with a web page saying both "Error" and "Success" at the same time!  I am hoping the logic precludes that elsewhere.

Author

Commented:
Thanks to all. Both proposed solutions were very helpful.