Skip to content

Conversation

@nuzil
Copy link
Contributor

@nuzil nuzil commented Apr 6, 2018

Description

This PR improves code structure for usage of if / else statements

Fixed Issues (if relevant)

No, code improvements

Manual testing scenarios

No, code improvements

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

$items = array_values($this->_criticalCollection->getItems());
$this->_latestItem = false;
if (count($items)) {
$this->_latestItem = $items[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Ternary operator would fit better here.

} else {
return $this->_groupRepository->getById($groupId)->getCode();
}
return $this->_groupRepository->getById($groupId)->getCode();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ternary operator would fit better here.

} else {
$this->subscriptionHandler->processDisabled();
}
$enabled ? $this->subscriptionHandler->processEnabled() : $this->subscriptionHandler->processDisabled();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ternary operator is NOT really suitable here as it decreases code readability. it is better to be used in assignments only.

@magento-engcom-team magento-engcom-team merged commit 4cf7e8b into magento:2.3-develop Apr 10, 2018
@magento-engcom-team
Copy link
Contributor

Hi @nuzil. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.3.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants