Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JText: Changed conditional to make it easier to read #1839

Closed
wants to merge 1 commit into from

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Aug 28, 2013

I had serious problems understanding this conditional, so I simplified it a bit.

Old behavior:
1a. Check if there is a comma in the string. If not, return false.
2a. Compare that false to "false", resulting in "true".
3a. Negate that "true", resulting in false. Skip this block.

1b. Check if there is a comma in the string. If there is, return true.
2b. Compare that "true" against "false", resulting in "false".
3b. Negate that "false", resulting in true. Executing this block.

New behavior:
1a. Check if there is a comma in the string. If not or the comma is at the first position, return false or 0, which is both falsey. Skip this block.

1b. Check if there is a comma in the string. If there is and the comma is not at the first position, return an int > 0, which evaluates to true. Execute this block.

The only difference to before is, that you can't have a key with a leading comma. As far as I can see, this is never the case. At the same time, this code is a LOT more readable.

@@ -65,7 +65,7 @@ public static function _($string, $jsSafe = false, $interpretBackSlashes = true,
$jsSafe = false;
}
}
if (!(strpos($string, ',') === false))
if (strpos($string, ','))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to do all those comparisons at all. Either its 0 or false, then skip that block, or its a number which evaluates to true. So there is no need to complicate the code by negating comparisons against false.

@brianteeman
Copy link
Contributor

Thanks for your contribution - At this time we are only using github as the place to submit code fixes, the actual reporting of issues and testing fixes is still taking place on Joomlacode.

As it has been some time since you opened this issue can you please confirm that it is still valid with the current Master or Joomla 3.2 beta. If it is no longer valid then please can you close this issue. Otherwise please can you:

  1. Open an item on the Joomlacode tracker in the appropriate area.

CMS Bug Reports: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemBrowse&tracker_id=8103

CMS Feature Requests: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemBrowse&tracker_id=8549

  1. After submitting the item to the Joomlacode tracker, add a link to the Joomlacode tracker item here and make sure that you add a link to this GitHub issue or pull request on the joomlacode tracker item.

@Hackwar
Copy link
Member Author

Hackwar commented Dec 5, 2013

http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32888&start=0

It is still valid.

And with the words of Cato the Elder: Ceterum censeo Joomlacode esse delendam (I believe that Joomlacode has to be destroyed. ;-) )

@Bakual
Copy link
Contributor

Bakual commented Dec 5, 2013

And with the words of Cato the Elder: Ceterum censeo Joomlacode esse delendam (I believe that Joomlacode has to be destroyed. ;-) )

Most agree on that and we're working on it. You can help: https://github.com/joomla/jissues

@phproberto phproberto closed this in e3b3db2 Jun 4, 2014
@Hackwar Hackwar deleted the jtext branch April 27, 2019 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants