[#11870] - Code optimization for generating pathway
- Closed
- 1 Sep 2016
- Medium
- Build: staging
- # 11870
- Diff
- nvyush:upd-pathway
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Pending JTracker/HumanTestResults Human Test Results: 1 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
Pull Request for Issue # .
Summary of Changes
Some 'view.html.php' files contain this code to generate pathway (data for breadcrumbs):
$path = array_reverse($path);
foreach ($path as $item)
{
$this->pathway->addItem($item['title'], $item['link']);
}
I suggest to replace that with more efficient code:
for ($i = count($path) - 1; $i > -1; --$i)
{
$this->pathway->addItem($path[$i]['title'], $path[$i]['link']);
}
An array $path is not used after this code fragment, so we can leave it non-reversed and iterate from end to start.
Modifications of code are trivial and should not lead to side effects.
Testing Instructions
On the administration panel:
1) Create if not exists and public a module of type 'Breadcrumbs'.
2) Create test structures of categories for articles, contacts, news feeds.
3) Create test articles, contacts, news feeds and move them to test categories.
4) Create test menu items of types 'Articles - Category list', 'Contacts - List Contacts in a Category', 'News Feeds - List News Feeds in a Category' for appropriate root test categories.
On the site:
1) Open the test pages for categories, articles, contacts, news feeds and make screen shots.
2) Apply the patch.
3) Re-open the test pages for categories, articles, contacts, news feeds and make screen shots. There must be no differences.
Documentation Changes Required
None
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
| Category | ⇒ | Front End Components |
- I have tested functionality and review code. Before Patch
After Patch
-
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11870.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11870.
@Bakual, @rdeutz I thought, that reversed iteration is not a 'hard coding'. It may be commented.
My solution removes unnecessary call of function array_reverse and a cycle inside that function.
UPD:
What you think about this code?
/libraries/cms/html/string.php
// Closing tags need to be in the reverse order of opening tags.
$openedTags = array_reverse($openedTags);
// Close tags
for ($i = 0; $i < $numOpened; $i++)
{
if (!in_array($openedTags[$i], $closedTags))
{
$tmp .= "</" . $openedTags[$i] . ">";
}
else
{
unset($closedTags[array_search($openedTags[$i], $closedTags)]);
}
}
}
I have tested this item
yeah i have tested this item successfully
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11870.
I agree with the above points on the readability. I had to read the change three times to realize you were doing negative iterations on your proposed for loop.
If there's a major performance benefit to be found here, I'd say go forward with the change. But IIRC the array_*() functions are already pretty strongly performant so that one function call isn't a major slowdown at all.
Before we spend more time on this I am going to close it.
Here we are speaking about a max 5 element array and the reverse can be done on low level with a simple pointer change. So I am not really convinced that the proposed code is more efficient.
It is for sure not better readable and finally I think this is more important here.
@nvyush thanks for working on this and proposing a change.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-09-01 13:08:32 |
| Closed_By | ⇒ | rdeutz |
Personally I think the code is harder to understand and the performance gain is likely not measurable at all.