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 FilterNum() in ArrayHelper.php #43

Open
wants to merge 6 commits into
base: 3.x-dev
Choose a base branch
from

Conversation

korenevskiy
Copy link

@korenevskiy korenevskiy commented Sep 25, 2023

Pull Request for Issue #

A function for filtering only numbers from an array of values. Used to filter values to insert into an SQL query.

$newarray = Array_Helper::filterNum($array =[], $onlyPositive = true));

Testing Instructions

$search = "One, two ,three, four, 5, 6 ,7";
$new_array = Array_Helper::filterNum(explode(',', $search));

return

[5, 6, 7];

unlike the array_filter() function, this function array to return TRIM() strings at the same time. And the digit 0 was also considered a Valid value in the array.
This is necessary to filter indexes for insertion into the database.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@izharaazmi
Copy link
Contributor

I believe this feature is not required as the task is fairly simple when using native PHP. Let me explain:

Filter numbers only

All supported PHP versions

array_filter($array, 'is_numeric')

Filter positive numbers only

Please note that the same can be extended for negative numbers, even numbers, odd numbers and various other conditions easily.

PHP 7.4 onwards

array_filter($array, fn($val) => is_numeric($val) && $val >= 0)

PHP below 7.4

array_filter($array, function($val) {
	return is_numeric($val) && $val >= 0;
});

@korenevskiy
Copy link
Author

I believe this feature is not required as the task is fairly simple when using native PHP. Let me explain:

joomla/joomla-cms#39966 (comment)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@korenevskiy
Copy link
Author

korenevskiy commented Sep 26, 2023

I believe this feature is not required as the task is fairly simple when using native PHP. Let me explain:

The trick is for the array to return TRIM() strings at the same time. And the digit 0 was also considered a Valid value in the array.
This is necessary to filter indexes for insertion into the database.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@izharaazmi
Copy link
Contributor

@korenevskiy I agree to your use case. Still I truly believe that ArrayHelper is not the right place for it.

May be StringHelper::toInitArray($text, $unsigned = false) or something even better.

@izharaazmi
Copy link
Contributor

@korenevskiy
Copy link
Author

Instead of putting it in ArrayHelper I would prefer something like this:

You have combined 2 tasks into 1. 1.Splitting a string into an array. 2. Filtering the array. In principle, combining 2 tasks is useful. But it is useful for internal calculations. And the use of the helper class has a wider use in different places. It may turn out that you just need to filter the array without dividing the string into an array. So I suggested a simple filtering method.

Another question is the intuitiveness of the code. How would an average programmer write code.

KeywordSearch::toIds($str);

or

ArrayHelper::filterIds(explode(',',$str)); //or ArrayHelper::filterNum()

I would use only the second option. Although the first one is shorter, but it's not intuitive for me.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@izharaazmi
Copy link
Contributor

Just in case the team decides to go with the approach in this PR, I would add few suggestions:

  1. Name the method filterNumeric
  2. Make the first argument $array required
  3. Typehint first argument as array
  4. Skip the key $i in foreach loop
  5. Rename $new_array to $result to be consistent with other methods
  6. Instead of using settype use float casting as settype will make no difference here and keep it as string
  7. Invert if

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@korenevskiy
Copy link
Author

korenevskiy commented Sep 27, 2023

I changed the PR according to your recommendations . They are weighty. Except deleting $i. Since the static method removes filters array elements, this should save the array keys. Do you think the array keys are not needed?
Use this should imply saving the type of each element. You should notice this in the code.
We can't just change the type without a requirement, can we?

@izharaazmi
Copy link
Contributor

We can't just change the type without a requirement, can we?

Right, but as the name suggests filterNumeric, it essentially calls upon only numeric values IMHO.

@korenevskiy
Copy link
Author

Right, but as the name suggests filterNumeric, it essentially calls upon only numeric values IMHO.

as if there is still a function is_num(). it accepts a string and the string can be regarded as a number. It is by analogy of this function that I originally called the filterNum() method. If it were C#, I would really make the parameter type more typed. But here we have PHP, where one function serves for all types.

@izharaazmi
Copy link
Contributor

"The is_numeric function serves as a straightforward validation check, returning a Boolean value. On the other hand, I believe that filterNumeric offers a more comprehensive functionality. However, I respect your decision if you prefer to maintain the current data type."

@korenevskiy
Copy link
Author

as if there is still a function is_num(). it accepts a string and the string can be regarded as a number. It is by analogy of this function that I originally called the filterNum() method. If it were C#, I would really make the parameter type more typed. But here we have PHP, where one function serves for all types.

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

2 participants