[#8544] - Simplified spaghetti code
- Fixed in Code Base
- 1 Sep 2016
- Medium
- Build: staging
- # 8544
- Diff
- demis-palma:patch-8
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
Removed duplicated code
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
brianteeman
- | Category | ⇒ | Code style |
(int) $this->params->get($side, 0) is used twice. Can be refactored as $index.
The if can be more readable if changed to
if (isset($skindirs[$index]))or whole thing as a ternary, maybe!
$side = $app->isAdmin() ? 'skin_admin' : 'skin';
$index = (int) $this->params->get($side, 0);
$skin = isset($skindirs[$index]) ? basename($skindirs[$index]) : 'lightgray';
$skin = 'skin : "' . $skin . '",';Yours is good too. I'm just suggesting. 
What is the status?
Can we test this, if so can we have a Test scenario?
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8544.
IMO, This is most of a code review.
However the current code (without this patch) would break in the case where $app is neither site nor administrator. Try loading TinyMCE editor in the installation application with error display and error reporting on.
Or you can create a third application similar to administrator with client_id = 3 also. But that is too much of work for this small test.
I have tested this item
Opened TinyMCE normally, worked w/o a problem. Added a tinymce instance in installation app manually, worked as well. Code review seems fine, too. @icampus
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8544.
I have tested this item
tested @icampus:
simplification does what it should
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8544.
@demis-palma Could you make the suggested changes please?
rdeutz
- | Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-09-01 17:13:53 |
| Closed_By | ⇒ | rdeutz |
| Category | Code style | ⇒ | Plugins Front End Code style |
Hi @rdeutz I have changed getClientId() with isAdmin() as per request.