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

Added option to display both absolute and percentage values in pie chart. #85

Closed
wants to merge 1 commit into from

Conversation

MegaChriz
Copy link

Fixes #84.

@MegaChriz
Copy link
Author

I see that the build is failing but I believe that the failure is a false positive.

Line   src/Chart/Pie.php                           

 ------ -------------------------------------------- 

  2353   Variable $Absolute might not be defined.    

  2356   Variable $Percentage might not be defined.  

  2359   Variable $Absolute might not be defined.    

  2359   Variable $Percentage might not be defined.

If you inspect the code clearly, then you can see that $Absolute and $Percentage always will have a value when they are used.

  • $Absolute is defined when $WriteValues is PIE_VALUE_NATURAL or PIE_VALUE_BOTH. The variable isn't used later on if $WriteValues is something else.
  • $Percentage is defined when $WriteValues is PIE_VALUE_PERCENTAGE or PIE_VALUE_BOTH. The variable isn't used later on if $WriteValues is something else.

But perhaps I could write the code differently, if desired. I think I would fix it by defining two additional methods.

@szymach
Copy link
Owner

szymach commented Nov 3, 2022

Hello!
Thank you for your submission. I will have to consider this addition a bit, I think I will be able to do it this weekend. Cheers.

@MegaChriz
Copy link
Author

Thanks for considering! Take the time you need. 🙂

*/
protected function getDisplayValue($Value, $WriteValues, $SerieSum, $Precision, $ValueSuffix)
{
if ($WriteValues == PIE_VALUE_NATURAL || $WriteValues == PIE_VALUE_BOTH) {
Copy link
Owner

Choose a reason for hiding this comment

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

Hello, sorry for late reply. Try the code below, it should silence any static code exceptions.

$ValidWriteValues = [PIE_VALUE_NATURAL, PIE_VALUE_PERCENTAGE, PIE_VALUE_BOTH];
if (false === in_array($WriteValues, $ValidWriteValues, true)) {
    return '';
}

$isBothOrNatural = in_array($WriteValues, [PIE_VALUE_NATURAL, PIE_VALUE_BOTH], true);
$isBothOrPercentage = in_array($WriteValues, [PIE_VALUE_PERCENTAGE, PIE_VALUE_BOTH], true);

$Absolute = true === $isBothOrNatural ? ($Value . $ValueSuffix) : null;
$Percentage = true === $isBothOrPercentage
    ? sprintf('%s%', round((100 / $SerieSum) * $Value, $Precision))
    : null
;

if (null === $Absolute && null === $Percentage) {
    throw new RuntimeException(
        "Neither absolute nor percentage display value was calculated"
        . " for display value \"{$WriteValues}\"."
    );
}

if (null !== $Absolute && null !== $Percentage) {
    $Result = "{$Absolute}\n({$Percentage})";
} elseif (null !== $Absolute) {
    $Result = $Absolute;
} elseif (null !== $Percentage) {
    $Result = $Percentage;
}

return $Result;

Also, can you please write a test case in this unit test for the new option?

@szymach
Copy link
Owner

szymach commented Jun 15, 2023

Hello!
I am closing this PR, since I have incorporated Your code myself into the 3.1 branch. I will make a release sometime soon.

Cheers.

@szymach szymach closed this Jun 15, 2023
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.

2 participants