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

Add support for IFS() logical function #1442

Merged
merged 6 commits into from
Jun 20, 2020
Merged

Add support for IFS() logical function #1442

merged 6 commits into from
Jun 20, 2020

Conversation

Takitaf
Copy link
Contributor

@Takitaf Takitaf commented Apr 13, 2020

This is:

- [ ] a bugfix
- [x] a new feature

Checklist:

Why this change is needed?

PHPSpreadsheet missing Excel IFS logical function support

@MarkBaker
Copy link
Member

Thank you for this submission; I'l take a look at it this weekend, as it would be good to support the IFS() function

@Takitaf
Copy link
Contributor Author

Takitaf commented May 27, 2020

@MarkBaker Thanks for a response.

There is one detail in my implementation that probably needs to be changed. In theory I want to use existing functions if I can and also want to stop executing on a first condition that returned true. It works that way, but I use here the Functions::DUMMY() for a "false value" placeholder. As far as I can see, although by name it seems to be perfectly fine, it is rather used to mark not-implemented features yet.

Additionally, there is a small chance, that user's "true value" would be exactly the same as Functions::DUMMY() value - and in that case it will not behave correctly.

I could calculate IFS in reverse order - calculate last condition, get previous condition and use last condition value as "false value" of previous condition, and repeat that until I reach the very first condition. It, however, makes this feature calculate always all conditions in each IFS statement.

Maybe good enough solution is just introduce dummy value locally, that is even harder to collide with in real world (php_spreadsheet prefix + hash?)

@PowerKiKi PowerKiKi requested a review from MarkBaker May 31, 2020 13:47
Copy link
Member

@MarkBaker MarkBaker left a comment

Choose a reason for hiding this comment

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

Sorry that it's taken me so long to do a review of your submission; it really is useful to have an implementaton of IFS(); and I can understand your thinking about evaluation in reverse order, but that makes evaluation potentially slower if there are a lot of conditions

$returnIfTrue = ($arguments[$i + 1] === null) ? '' : Functions::flattenSingleValue($arguments[$i + 1]);
$result = self::statementIf($testValue, $returnIfTrue, Functions::DUMMY());

if ($result !== Functions::DUMMY()) {
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I understand your concern that there is a possibility that the returned result may be a valid case of the string '#Not Yet Implemented'; unlikely, but not impossible.
I might be inclined to create something purely PHP detectable, that cannot exist in a spreadsheet, such as an exception. Maybe create a new EvaluateIfFalseException which can be returned as $result rather than thrown; and directly type tested rather than simply value tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. Can I use just simple instance of generic Exception in that case: $falseValueException = new Exception();? Not sure if we need special Exception for that.

Copy link
Member

Choose a reason for hiding this comment

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

Yes you could use a standard Exception, I was simply thinking about readabilityand providing a named exception where the name provided contextual meaning

@MarkBaker MarkBaker merged commit 859bef1 into PHPOffice:master Jun 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants