current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

This is a page that allow user to do register:

<?php echo form_open('user/valid_register_page', 'autocomplete="off"');?>

    <?php $this->table->set_template(array('table_open'=>'<table class="table_form_register">')); 
    $this->table->add_row(form_label($label_email.
                                    form_label(form_error('email'),'',array('class' => 'error_label')), 
                                    'label_email',
                                     array('class' => 'form_label')), 

                           array('class' => 'align_right_td', 
                                 'data' => form_input('email', 
                                           set_value('email'),
                                           'class = "align_right_input require"')));  

    $this->table->add_row(form_label($label_invitation_key.
                                    form_label(form_error('invitation_key'),'',array('class' => 'error_label')),
                                    'label_invitation_key',
                                     array('class' => 'form_label')),

                          array('class' => 'align_right_td',  
                                 'data' => form_password('invitation_key',
                                           set_value('invitation_key'),
                                           'class = "align_right_input require"')));

    $this->table->add_row('',array('class' => 'button_td',
                  'data' => form_submit('register',  $button_register, 'class = "form_td_button"')));

    echo $this->table->generate();    

    ?>          
<?php echo form_close();?>

This code only have 2 label and 2 fields: one is email, another is the invitation keys. This is the code that render for generation the form, table, and the button with these 2 label and fields. But as you can see, it is very messy, if I need to maintain, it will become a headache. Any suggestions on how to make it simpler?

share|improve this question
    
Try to avoid mixing PHP and HTML. See this for some advice: stackoverflow.com/questions/62617#95027 –  Paul Aug 31 '11 at 15:32
1  
I really don't get these "HTML helpers". The name suggests they should be helping, while all I see is an incredibly verbose way to write what would be a lot more readable as plain HTML. –  Matteo Riva Sep 5 '11 at 21:05

3 Answers 3

up vote 2 down vote

You could start by removing dynamic HTML generation where there is no benefit in it. You would be much better of writing something like this :

<?php
   $form = $this->form;
?>
<form action="<?php echo $form->get_destination(); ?>" method="post">
    <ul>
        <li>
            <?php echo $form->render_field( 'email' ); ?>
        </li>
        <li>
            <?php echo $form->render_field( 'invitation_key' ); ?>
        </li>
        <li><?php echo $form->render_field( 'submit' ); ?></li>
    </ul>
</form>

Yes, none of it is CodeIgniter , but then again i have huge dislike for frameworks with crappy OOP.

share|improve this answer
up vote 1 down vote

This seems to me to be one single piece of php so therefore you should put all the code above into a single

<?php
//Put your php code here.

?>

Use indentation correctly and consistently.

share|improve this answer
    
Minus the last ?> as it's not needed and reduces the whitespace being sent to the client. –  Levinaris Aug 29 '11 at 17:31
    
I disagree. I would always include the final ?> in a file, unless specifically required to be left out such as in a drupal module file. –  Toby Allen Aug 30 '11 at 7:18
    
@TobyAllen I agree with Levinaris as do others: stackoverflow.com/questions/3219383/… –  Paul Dec 14 '11 at 2:35
    
and other seem to agree with me :) Each to his own it seems. –  Toby Allen Dec 14 '11 at 19:24
up vote 1 down vote

No need to open and close php tags over and over. just open them at the beginning, then close them at the end.

Also, you're stringing array after array in there. create variables to hold the arrays and create them before they're needed. Then, you can use them in the add_row calls. This will clean up most of the code.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.