I feel like this code is pretty sloppy and can be accomplished in a foreach
loop. But it's a bit complex using a multi-dimensional array.
Can this be cleaner? You can see I'm repeating much of the same code in each li
, minus some class and array key differences, etc. Should this be a foreach
loop? What's the best approach?
// data example:
{
"user": {
"assigned": 3,
"overdue": 2,
"in_review": 1,
"in_progress": 2
},
"team_avg": {
"overdue": 1,
"in_review": 1,
"in_progress": 1,
"assigned": 1
}
}
<ul class="large-block-grid-4">
<li>
<small>Assigned</small>
<h4 class="assigned">
{{ AbbrNum::convert($taskload['user']['assigned']) }}
@if (count($taskload['team_avg']))
@if ($taskload['user']['assigned'] > $taskload['team_avg']['assigned'])
<i data-tooltip aria-haspopup="true" title="Above Team Avg" class="fa fa-arrow-up above-avg" aria-hidden="true"></i>
@elseif ($taskload['user']['assigned'] == $taskload['team_avg']['assigned'])
<i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-minus" aria-hidden="true"></i>
@else
<i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-arrow-down below-avg" aria-hidden="true"></i>
@endif
@endif
</h4>
</li>
<li>
<small>In Progress</small>
<h4 class="in-progress">
{{ AbbrNum::convert($taskload['user']['in_progress']) }}
@if (count($taskload['team_avg']))
@if ($taskload['user']['in_progress'] > $taskload['team_avg']['in_progress'])
<i data-tooltip aria-haspopup="true" title="Above Team Avg" class="fa fa-arrow-up above-avg" aria-hidden="true"></i>
@elseif ($taskload['user']['in_progress'] == $taskload['team_avg']['in_progress'])
<i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-minus" aria-hidden="true"></i>
@else
<i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-arrow-down below-avg" aria-hidden="true"></i>
@endif
@endif
</h4>
</li>
<li>
<small>In Review</small>
<h4 class="in-review">
{{ AbbrNum::convert($taskload['user']['in_review']) }}
@if (count($taskload['team_avg']))
@if ($taskload['user']['in_review'] > $taskload['team_avg']['in_review'])
<i data-tooltip aria-haspopup="true" title="Above Team Avg" class="fa fa-arrow-up above-avg" aria-hidden="true"></i>
@elseif ($taskload['user']['in_review'] == $taskload['team_avg']['in_review'])
<i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-minus" aria-hidden="true"></i>
@else
<i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-arrow-down below-avg" aria-hidden="true"></i>
@endif
@endif
</h4>
</li>
<li>
<small>Overdue</small>
<h4 class="overdue">
{{ AbbrNum::convert($taskload['user']['overdue']) }}
@if (count($taskload['team_avg']))
@if ($taskload['user']['overdue'] > $taskload['team_avg']['overdue'])
<i data-tooltip aria-haspopup="true" title="Above Team Avg" class="fa fa-arrow-up below-avg" aria-hidden="true"></i>
@elseif ($taskload['user']['overdue'] == $taskload['team_avg']['overdue'])
<i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-minus" aria-hidden="true"></i>
@else
<i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-arrow-down above-avg" aria-hidden="true"></i>
@endif
@endif
</h4>
</li>
</ul>
-
2\$\begingroup\$ Please edit your title to say something about what the code does rather than what you'd like reviewed. \$\endgroup\$Phrancis– Phrancis2016年04月23日 20:46:51 +00:00Commented Apr 23, 2016 at 20:46
-
\$\begingroup\$ The title still doesn't look proper. Please look at the front page for examples on good titles. \$\endgroup\$Jamal– Jamal2016年04月23日 22:05:28 +00:00Commented Apr 23, 2016 at 22:05
1 Answer 1
I agree that a foreach
would be possible, but probably not the best solution here as it would indeed make the code overly complex. There is a lot of repetition though, so there is indeed room for improvement. I think a partial sub view would probably be the cleanest solution here. This is how I would go about it.
in a separate file (partials/list_item.blade.php):
<li>
<small>{{$title}}</small>
<h4 class="{{$class}}">
{{ AbbrNum::convert($taskload['user'][$type]) }}
@if (count($taskload['team_avg']))
@if ($taskload['user'][$type] > $taskload['team_avg'][$type])
<i data-tooltip aria-haspopup="true" title="Above Team Avg" class="fa fa-arrow-up above-avg" aria-hidden="true"></i>
@elseif ($taskload['user'][$type] == $taskload['team_avg'][$type])
<i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-minus" aria-hidden="true"></i>
@else
<i data-tooltip aria-haspopup="true" title="Below Team Avg" class="fa fa-arrow-down below-avg" aria-hidden="true"></i>
@endif
@endif
</h4>
</li>
And in your existing template:
<ul class="large-block-grid-4">
@include('partials.list_item', ['title' => 'Assigned', 'type' => 'assigned', 'class' => 'assigned' ])
@include('partials.list_item', ['title' => 'In progress', 'type' => 'in_progress', 'class' => 'in-progress' ])
...
</ul>
-
\$\begingroup\$ Interesting concept! I didn't think to approach it like this - but makes sense. I wonder if there's any performance differences between this and a
foreach
. That said, if there is, it's likely very low/petty anyways. Thanks for your feedback/help! \$\endgroup\$Mike Barwick– Mike Barwick2016年04月24日 00:03:46 +00:00Commented Apr 24, 2016 at 0:03 -
\$\begingroup\$ What about foreach'ing the partial? \$\endgroup\$Mike Barwick– Mike Barwick2016年04月24日 00:04:44 +00:00Commented Apr 24, 2016 at 0:04
-
\$\begingroup\$ Wait, that's essentially the same thing as my original question lol \$\endgroup\$Mike Barwick– Mike Barwick2016年04月24日 00:08:42 +00:00Commented Apr 24, 2016 at 0:08