Assigning Objects vs. Using directly

[Donald Gilbert]

https://github.com/joomla/joomla-platform/blob/staging/libraries/legacy/model/legacy.php#L511

This is one example of what I think is mixed coding styles. At the top of the function, we assign the dispatcher and get the config to vars (which consumes memory), and then each item is only used once. These could easily be replaced with calls directly on the returned object.

For example, `$dispatcher->trigger` could be replaced with `JEventDispatcher::getInstance()->trigger` and save memory. We could do the same with `JFactory::getConfig()->get` instead of assigning to `$conf` and then using that var once. You can see this suggested style was followed on line 517 within the same method with the call to `JFactory::getInstance()->input->get`

What's the 'right' way to do things when it comes to the Joomla Platform? It's always been my thought that if an object would only be used once, to bypass assigning it to a var, and just use it directly. If it's used more than once, we should assign it to a var. Would that be the general consensus?

[Nils Rückmann]

I think it's all about readability. So in this case i would prefer to assign the app or even the input to a variable.

[David Swain]

Since objects are assigned by reference, not copy, does this really consume any more memory? Sure there will be an extra entry in the symbol table until it gets garbage collected, but thats got to be all of 4 bytes* or so, which doesn't seem worth worrying about.

With such a small possible saving is it really worth trading the readability?

Also, I'm not sure, but there may be some issues if the trigger function takes the argument by reference and the getInstance function isn't defined as giving a reference... but that's a bit foggy for me.

:-Dave.

* this is a total guess, but it surely can't be far off?

[Donald Gilbert]

I am for readability as well, but I want to be able to read as well as follow the code logically. Why is $dispatcher assigned at the top, but then not used until the bottom of the method. Surely JEventDispatcher::getInstance()->trigger is just as readable as the current implementation; and IMHO much easier to follow the logic.

[Nils Rückmann]

It's about the line lenght, which increases the readibility:

'defaultgroup' => ($group) ? $group : (isset($this->option) ? $this->option : JFactory::getApplication()->input->get('option'))
vs
'defaultgroup' => ($group) ? $group : (isset($this->option) ? $this->option : $input->get('option'))

[Donald Gilbert]

Well, nested ternary is not readable no matter how you look at it. :)

[Adam Stephen Docherty]

I agree with Donald and not just with nested ternary, but ternary operators in general.

[Andrew Eddie]

Ternaries are like all things, good when used appropriately. The
question to ask is "is it immediately obvious to the reader what the
ternary is doing?". A nested ternary is going to fail that test
pretty much every time (so do regular expressions for that matter).
It would be more readable to blow that array() statement up and
construct it piece by piece. Ironically, in doing that you could use
the JFactory::getConfig() directly if you wanted.

I agree that $dispatcher and $cache are unnecessary in this case (and
if you rebuilt the array, so is $conf). So is the comment "Trigger
the onContentCleanCache event" for that matter (Anthony Ferrera has a
good post on keeping your inline comments to a minimum, preferably
zero, providing you write your code so there is no confusion about
what it's doing - moderation is still required but there are some good
rules of thumb there).

Regards,
Andrew Eddie
http://learn.theartofjoomla.com - training videos for Joomla developers