Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 12 down vote favorite
3

Consider the following. It's a simple list of items in a list, which allow the user to dynamically generate that list of events. Then a controller action does the work of serializing that into the database.

The issue is that there's the PHP generated HTML segment, and there's a separate Javascript segment to do the additions (for when someone presses the "add new page" button). This is not only duplicated; when it's done inline in the javascript it's extremely ugly to look at (look at the length of that line!).

Is there a better way of doing this?

<?php
/** @var $this Zend_View */

$this->headLink()->appendStylesheet($this->baseUrl('css/redmond/jquery-ui-1.8.5.custom.css'));

$this->headScript()->appendFile($this->baseUrl('js/jquery.js'));
$this->headScript()->appendFile($this->baseUrl('js/jquery-ui-1.8.5.custom.min.js'));

$this->headScript()->captureStart(); ?>
//<script language="text/javascript">
    function CreateDateboxes(jqObject) {
        jqObject.datepicker({
            dateFormat: 'yy-mm-dd',
            showOn: 'button',
            changeYear: true,
            changeMonth: true
        });
    }

    function RemoveParent() {
        $(this).parent().parent().remove();
    }

    function AddNewEvent() {
        var today, html, temp, datestring;
        today = new Date();
        datestring = (today.getYear()+1900) + '-' + (today.getMonth()+1) + '-' + today.getDate();
        html = '<li class="ui-content">\n    <div style="float: left; width: 200px;">\n        <span class="ui-icon ui-icon-trash ui-button ui-state-active" style="float: left; margin:3px;"></span>\n        <input name="dates[]" class="datebox" style="width: 120px;" type="text" value="' + datestring + '" />\n    </div>\n    <div style="padding-left: 200px;">\n        <input name="contents[]" type="text" style="width: 100%;" />\n    </div>\n</li>';
        $(this).after(html);
        temp = $(this).next();
        CreateDateboxes($('.datebox', temp));
        $('.ui-icon-trash', temp).click(RemoveParent);
    }

    $(function() {
        CreateDateboxes($('.datebox'));
        $('.ui-icon-trash').click(RemoveParent);
        $('.ui-icon-plus').parent().click(AddNewEvent);
    });
//</script>
<?php $this->headScript()->captureEnd(); ?>
<div class="story">
    <form action="<?= $this->url(array('controller' => 'admin', 'action' => 'applyEvents')) ?>" method="post">
        <ul style="list-style: none; margin-bottom: 1em; padding: 0; width: 100%;">
            <li class="ui-button ui-state-default" style="width: 100%; margin-bottom: 5px;"><span class="ui-icon ui-icon-plus" style="float: left;"></span>
                Add a new Event</li>
            <? foreach ($this->events as $event) { ?>
                <li class="ui-content">
                    <div style="float: left; width: 200px;">
                    <span class="ui-icon ui-icon-trash ui-button ui-state-active" style="float: left; margin:3px;"></span>
                    <input name="dates[]" class="datebox" style="width: 120px;" type="text" value="<?= $event->GetDate()->format('Y-m-d') ?>" />
                    </div>
                    <div style="padding-left: 200px;">
                        <input name="contents[]" type="text" value="<?= $event->GetMessage() ?>" style="width: 100%;" />
                    </div>
                </li>
            <? } ?>
        </ul>
        <input type="submit" name="submit" value="Apply" />
        <input type="submit" name="submit" value="Cancel" />
    </form>
</div>
share|improve this question

4 Answers

up vote 4 down vote accepted

I'm assuming from looking the code you're using Zend Framework. I've solved similar issues in Zend and Symfony using php partials.

You could create a partial _list_item.phtml

<li class="ui-content">
    <div style="float: left; width: 200px;">
        <span class="ui-icon ui-icon-trash ui-button ui-state-active" style="float: left; margin:3px;"></span>
        <input name="dates[]" class="datebox" style="width: 120px;" type="text" value="<?php echo $date ?>" />
    </div>
    <div style="padding-left: 200px;">
        <input name="contents[]" type="text" value="<?php echo $message ?>" style="width: 100%;" />
    </div>
</li>

Then in your view:

<?php foreach ($this->events as $event): ?>
    <?php echo $this->partial('list_item.phtml', array(
        'date' => $event->GetDate()->format('Y-m-d'),
        'message' => $event->GetMessage(),
    )); ?>
<?php endforeach; ?>

Then for your javascript:

function AddNewEvent()
{
    var today, html, temp;
    // call str_replace because javascript doesn't like new lines in strings
    html = '<?php echo str_replace(PHP_EOL, "\n", $this->partial('list_item.phtml', array(
        'date' => $event->GetDate()->format('Y-m-d'),
        'message' => '',
    ))); ?>';
    $(this).after(html);
    temp = $(this).next();
    CreateDateboxes($('.datebox', temp));
    $('.ui-icon-trash', temp).click(RemoveParent);
}

Viola! No template duplication because the affected code is in a partial. No excess javscript templating libraries, although if this kind of thing is prevalent in your application I'd suggest some refactoring and implementing such a templating library.

This approach will have the least impact on your work flow, little to no learning curve, and a short investment in development time for big decrease duplication (bosses love hearing that kind of thing).

share|improve this answer
1  
+1 Slick! (But if you're doing this in production you should probably use json_encode instead of str_replace :) – Billy ONeal Feb 10 '11 at 16:03
This also didn't set the date on the client as the preivous code had done. I ended up using something like this except I replaced the javascript with pastebin.com/MmFcrbHu – Billy ONeal Feb 13 '11 at 0:53
It should have print the date. The issue was a type in my partial _list_item.phtml. Just replace the echo $message with echo $date. My mistake. I've update the code above. – xzyfer Feb 13 '11 at 1:03
@zxyfer: Haha -- didn't even notice that (I didn't copy/paste this exactly...) – Billy ONeal Feb 13 '11 at 1:49
up vote 1 down vote

Very good question. Use something like Mustache ( http://mustache.github.com/ ).

Mustache is a templating framework that has renderers both on the client-side and the server side, which allows you to avoid code duplication.

share|improve this answer
up vote 1 down vote

Templating is definitely the answer. Another option is http://beebole.com/pure/ for client side templating.

With templating you can write your html in one place, then add it to a variable in javascript, then append the variables html into a node applying the template.

No duplicate html, no ajax.

share|improve this answer
Doesn't this cause the page to flicker as it's loaded because the JS doesn't run until the DOM is constructed? – Billy ONeal Feb 1 '11 at 1:44
@Billy, We use pure.js for our web app and there isn't any flickering – Mic Feb 1 '11 at 10:19
@Mic: Do you have a demonstration? The demonstrations on the pure.js page definitely flicker. – Billy ONeal Feb 10 '11 at 16:10
check our app beebole-apps.com/?demo&x=3&lang=en_US it is fully rendered client side – Mic Feb 10 '11 at 19:52
@Mic You can avoid the flicker if you print out regular html with PHP, then just store the html in a JS var and use pure to add new instances of it, but with a new values for the templates. static html, plus dynamic new html, all using the same HTML source. (on the server side you would put the HTML into a function or variable or something, so you could reuse it. Also to be super fancy, you could use a templating framework like Zend on the server side.) – user1272 May 4 '11 at 13:43
up vote 0 down vote

I ran into a similar problem just a week ago.
And I got tired of duplicating really fast, too.
So I just created one JavaScript function to do the printing of data to the page, and made my PHP code to print JavaScript calls to that function. The code became much cleaner that way.

share|improve this answer
How do you get the javascript function called? I.e. you can't inject into the DOM until the page is correctly loaded, and if you wait for that the initial layout of the page will be wrong... – Billy ONeal Feb 10 '11 at 16:01
I simply printed <script> addRow(params1); addRow(params2)</script> using php "echo". (a bit more complex than that, but the main idea was just like this) – Rogach Feb 10 '11 at 16:28

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.