20
\$\begingroup\$

I'm currently in the process of creating a online work schedule for internal use. For this, the client needs to show the days of a single week.

I pass a week number and a year. From that I get the first day of that week, by using strtotime() passing an argument in the format [year]-W[weekno]-[dayofweek]. Example: strtotime('2011-48W-1');

With that function I create my table as mentioned earlier:

<table class="weekly">

    <thead> 

        <tr>

            <?php for($i = 1; $i <= 7; $i++): ?>

            <th><?php echo $dage[$i] . " ". date("j/m", strtotime($year.'-W'.$week.'-'.$i)); ?></th>

            <?php endfor; ?>            

        </tr>

    </thead>

    <tbody>         

        <tr>

            <?php for($i = 1; $i <= 7; $i++): ?>

            <td<?php if($i == 7) { echo ' class="last"'; } ?>>

                <?php

                $day = date("Y-m-d", strtotime($year.'-W'.$week.'-'.$i));

                if(isset($entries[$day])):

                foreach($entries[$day] as $entry):

                ?>      

                <div class="workEntry" title="<?php echo $entry->note; ?>">

                    <span>

                        <?php echo date("H:i", strtotime($entry->workStart)); ?>

                        <?php if($entry->workStart != "0000-00-00 00:00:00") { echo " - ".date("H:i", strtotime($entry->workEnd)); } ?>

                    </span>

                    <span>

                    <?php if($entry->userId == 0): ?>

                    Unspecifed

                    <?php else: ?>

                    <?php echo $entry->name; ?>, <?php echo $entry->pgroup; ?>

                    <?php endif; ?>

                    </span>                     

                </div>              

                <?php endforeach;

                endif; ?>

            </td>

            <?php endfor; ?>            

        </tr>   

    </tbody>
</table>

Well, it works, but I'm pretty sure that I'm reinventing the wheel, and doing a pretty bad job at it too. I'd like to know what you think and to hear any suggestions.

\$\endgroup\$
2

3 Answers 3

12
\$\begingroup\$

Three minor notes:

  1. From Steve McConnell, Code Complete 2nd Edition, 31.2 Layout Techniques, p737:

    Using blank lines is a way to indicate how a program is organized. You can use them to divide groups of related statements into paragraphs, to separate routines from one another, and to highlight comments.

    It does not make sense if all groups contain only one line.

  2. strtotime($year.'-W'.$week.'-'.$i) is used and duplicated multiple times in the code. You could extract it out to a function to remove the duplication.

  3. If I'm right you are iterating through the days of the week with this loop:

    <?php for($i = 1; $i <= 7; $i++): ?>
    

    Renaming $i to $weekday or something similar would make it obvious and help readers/maintainers a lot.

\$\endgroup\$
9
+50
\$\begingroup\$

There is a lot of back and forth going on here.

I think that I would just pick PHP and write the whole thing in PHP, you would eliminate a lot of confusion and obscurity by being able to remove the <?php ?> tags from all over the place along with being able to properly indent the code.

You don't really have to worry about indenting the HTML tags inside of the PHP code, because that is just for looks on the developers side

<?php
    echo "<table class='weekly'><thead><tr>";
    for($i = 1; $i <= 7; $i++){
        echo "<th>" . $dage[$i] . " ". date("j/m", strtotime($year.'-W'.$week.'-'.$i)) . "</th>";
    }             

    echo "</tr></thead><tbody><tr>";
    for($i = 1; $i <= 7; $i++) {
        echo "<td"; 
        if($i == 7) { echo " class='last'"; } 
        echo ">";
        $day = date("Y-m-d", strtotime($year.'-W'.$week.'-'.$i));
        if(isset($entries[$day])) {
            foreach($entries[$day] as $entry) {
                echo "<div class='workEntry' title='" . $entry->note . "'><span>";
                echo date("H:i", strtotime($entry->workStart));
                if($entry->workStart != "0000-00-00 00:00:00") 
                { 
                    echo " - " . date("H:i", strtotime($entry->workEnd)); 
                } 
                echo "</span><span>";
                if($entry->userId == 0) {
                    echo "Unspecifed";
                }
                else {
                    echo $entry->name . "," . $entry->pgroup;
                }    
                echo "</span></div>";               
            }
        } 
        echo "</td>";
    }         
    echo "</tr></tbody></table>";
?>

all I did here was translate your HTML/PHP code mix into straight PHP, this should render the same exact page as your code.

Sometimes it is a good idea to mix PHP into HTML, but in this instance it is a better Idea to mix your HTML into your PHP, I think you can see that by looking at this code that it is much cleaner.


@palacsint makes a good observation about strtotime($year.'-W'.$week.'-'.$i) so I have created a function for this purpose even though it will only be called twice.

in this code I also changed $i to $weekdayNo a variation of @palacsint's suggestion, I also removed a lot of whitespace, probably more than @palacsint or you would have done.

<?php

    function dayOfWeek($dayNumber) {
        return strtotime($year . '-W' . $week . '-' . $dayNumber);
    }        

    echo "<table class='weekly'><thead><tr>";
    for($weekdayNo = 1; $weekdayNo <= 7; $weekdayNo++){
        echo "<th>" . $dage[$weekdayNo] . " ". date("j/m", dayOfWeek($weekdayNo)) . "</th>";
    }             

    echo "</tr></thead><tbody><tr>";
    for($weekdayNo = 1; $weekdayNo <= 7; $weekdayNo++) {
        echo "<td"; 
        if($weekdayNo == 7) { echo " class='last'"; } 
        echo ">";
        $day = date("Y-m-d", dayOfWeek($weekdayNo));
        if(isset($entries[$day])) {
            foreach($entries[$day] as $entry) {
                echo "<div class='workEntry' title='" . $entry->note . "'><span>";
                echo date("H:i", strtotime($entry->workStart));
                if($entry->workStart != "0000-00-00 00:00:00") { 
                    echo " - " . date("H:i", strtotime($entry->workEnd)); 
                } 
                echo "</span><span>";
                if($entry->userId == 0) {
                    echo "Unspecifed";
                }
                else {
                    echo $entry->name . "," . $entry->pgroup;
                }    
                echo "</span></div>";               
            }
        } 
        echo "</td>";
    }         
    echo "</tr></tbody></table>";
?>

Note: I don't know where $year and $week are being declared or created so you may encounter scope issues if you don't set those up correctly.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ +50 for addressing the <?php ?> tags all over the place - nice answer :) \$\endgroup\$ Commented Jul 4, 2014 at 15:45
5
\$\begingroup\$

I'm not 100% sure what the rest of your code is doing, but here is the code to get the days of a specified week using date:

<?php 
// Set current date
$date = '06/29/2014';
// Parse into a Unix timestamp
$ts = strtotime($date);
// Find the year and the current week
$year = date('o', $ts);
$week = date('W', $ts);
?>

Output would be like this:

Monday Tuesday Wednesday Thursday Friday Saturday Sunday

<?php for($i = 1; $i <= 7; $i++) { ?>
<th><?php $ts = strtotime($year.'W'.$week.$i); print date("l", $ts) . "\n"; ?></th>
<?php } ?>
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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