3
\$\begingroup\$

It's CakePHP, though I'm more interested in what people think about the layout of the indentation, the mark-up, position of logic etc.

Any feedback that relates to the subject of writing CakePHP views / front end code should take precedence over CakePHP feedback or logic feedback, though the latters are welcome too 

                <ul class='content_list'>

<?php foreach ($list as $l): ?>

    <?php $english=$l['translation']['english']; ?>

    <?php $translate_to=$l['translation']['translate_to']; ?>

    <?php $content=$l['Content']; ?>

                    <li>

    <?php if($l['flag_count']>0): ?>

                        <span class='flag_count'><?=$l['flag_count'] ?></span>

    <?php else: ?>

                        <span class='zero_flags'><?=$l['flag_count'] ?></span>

    <?php endif; ?>

                        <?=$this->Html->link("Edit",$l['edit_route']) ?>

                        <span class='english'>

                            <?=$english[$content['content_type']."_name"] ?>

                        </span>

    <?php if ($english_id!=$current_language_id): ?>

                        <span class='translate_to'>

                            <?=$translate_to[$content['content_type']."_name"] ?>

                        </span>

    <?php endif; ?>

    <?php if ('range'==$content['content_type']): ?>

        <?php $slider_class=(1==$content['slider']) ? "in_slider" : ""; ?>

        <?php $slider_text=(1==$content['slider']) ? "on home" : ""; ?>

                        <div class='js_slider_toggle' data-content-id='<?=$content['id'] ?>'>

                            <span class='slider_label'>

                                display on home?

                            </span>

                            <span class='js_icon slider_toggle <?=$slider_class ?>'>                                

                            </span>

                        </div>

    <?php endif; ?>

                    </li>

<?php endforeach; ?>

                </ul>
Improve this question
\$\endgroup\$
  • 3
    \$\begingroup\$ Just a minor comment but your code has a lot of vertical whitespace. Is it intentionally double-spaced? \$\endgroup\$ – Schism Aug 6 '14 at 20:31
  • \$\begingroup\$ It's marked out with four space indents (copied directly from ST), it's intentionally interleaved to show both HTML and logical PHP structure more clearly \$\endgroup\$ – Toni Leigh Aug 6 '14 at 20:33
  • \$\begingroup\$ it's a boring read @ColinSharpe. I don't recommend doing it like that \$\endgroup\$ – Malachi♦ Aug 6 '14 at 20:34
  • \$\begingroup\$ Why not use a templating system like Twig or Blade? \$\endgroup\$ – jsanc623 Aug 6 '14 at 21:23
  • \$\begingroup\$ @Malachi boring? such an objective way to measure things lol \$\endgroup\$ – Toni Leigh Dec 1 '15 at 20:40
2
\$\begingroup\$

Why not go with a template like Twig or Blade which would make the syntax a lot cleaner, while retaining most of all the controls. Also, remember that the view is for showing data, try to maintain as much logic OUT of the view as possible.

Here's an untested version of your code with Blade:

<ul class='content_list'>
    @foreach($list as $l)
    <li>
        @if($l['flag_count'] > 0)
            <span class='flag_count'>{{ $l['flag_count'] }}</span>
        @else
            <span class='zero_flags'>{{ $l['flag_count'] }}</span>
        @endif

        {{ $this->Html->link("Edit", $l['edit_route']) }}

        <span class='english'>
            {{ $l['translation']['english'][ $l['Content']['content_type'] . "_name" ] }}
        </span>

        @if($l['translation']['english'] != $current_language_id)
            <span class='translate_to'>
                {{ $l['translation']['translate_to'][ $l['Content']['content_type'] . "_name" ] }}
            </span> 
        @endif

        @if('range' === $l['Content']['content_type'])
            <div class='js_slider_toggle' data-content-id='{{ $l['Content']['id'] }}'>
                <span class='slider_label {{ $l['Content']['slider'] or "on_home" }}'>display on home?</span>
                <span class='js_icon slider_toggle {{ $l['Content']['slider'] or "in_slider" }}'></span>
            </div>
        @endif
    </li>   
    @endforeach
</ul>
Improve this answer
\$\endgroup\$
  • 1
    \$\begingroup\$ that looks really nice and clean. I like that! \$\endgroup\$ – Malachi♦ Aug 7 '14 at 15:54
  • \$\begingroup\$ this kind of misses the point of the question, which is about interleaving rather than templating choice - the HTML and PHP in this answer could still be interleaved, it's just the templating technique which is different \$\endgroup\$ – Toni Leigh Dec 1 '15 at 20:41
1
\$\begingroup\$

One thing that I like to say about PHP code that looks like this is to go all PHP.

Echo your HTML tags and do all of your fun PHP stuff and get rid of all those pesky PHP tags.

I tried to re-write your code, but I don't code it often so please forgive me if the syntax and such is a little off

<?php 
    echo '<ul class=\'content_list\'>';
    foreach ($list as $l): 
        $english=$l['translation']['english']; 
        $translate_to=$l['translation']['translate_to']; 

        $content=$l['Content']; 
        echo '<li>';

        if($l['flag_count']>0): 
            echo '<span class=\'flag_count\'>' . $l['flag_count'] . '</span>';
        else: 
            echo '<span class=\'zero_flags\'>' . $l['flag_count'] . '</span>';
        endif; 
        echo $this->Html->link("Edit",$l['edit_route']);
        echo '<span class=\'english\'>' . $english[$content['content_type']."_name"] . '</span>';

        if ($english_id!=$current_language_id): 
            echo '<span class=\'translate_to\'>' . $translate_to[$content['content_type']."_name"] . '</span>';
        endif;

        if ('range'==$content['content_type']): 
            $slider_class=(1==$content['slider']) ? "in_slider" : ""; 
            $slider_text=(1==$content['slider']) ? "on home" : ""; 
            echo '<div class=\'js_slider_toggle\' data-content-id=' . $content['id'] . \'>
                <span class=\'slider_label\'>display on home?</span>
                <span class=\'js_icon slider_toggle ' . $slider_class . '>                              
                </span></div>';
        endif; 
        echo '</li>';
    endforeach;
    echo '</ul>'
?>

Doesn't that look a lot cleaner?

You also don't have to worry about the fun indentation to keep everything straight (unless you really want to) you can combine your tags into echo statements.

Improve this answer
\$\endgroup\$
  • \$\begingroup\$ Ahhhhhh... this code is just begging for string interpolation :P \$\endgroup\$ – Schism Aug 6 '14 at 20:34
  • \$\begingroup\$ thank you @Schism. I don't code PHP well, let's put it that way \$\endgroup\$ – Malachi♦ Aug 6 '14 at 20:35
  • \$\begingroup\$ the problem I have with this is that the HTML structure is completely lost in the PHP, which was the problem I have been trying to over-come by writing things as I have been \$\endgroup\$ – Toni Leigh Aug 6 '14 at 20:35
  • \$\begingroup\$ @ColinSharpe, The HTML structure is there only for reading, you can have an HTML document of one line as long as all the tags are in the right place. this is the cleanest way to write your code. \$\endgroup\$ – Malachi♦ Aug 6 '14 at 20:36
  • \$\begingroup\$ @Malachi - reading it is important though, I need to come back to it in a few months, or another developer needs to come into the project, putting it all on one line, while valid, would be much harder to maintain \$\endgroup\$ – Toni Leigh Aug 6 '14 at 20:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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