lstrojny / functional-php

Primitives for functional programming in PHP
MIT License
1.98k stars 204 forks source link

refactor(average): Use array functions instead of foreach loop #171

Closed damianopetrungaro closed 6 years ago

damianopetrungaro commented 6 years ago

Is a more functional approach and for a repo named functional PHP would be beautiful to see it.

Erikvv commented 6 years ago

I don't like this change for 2 reasons:

Erikvv commented 6 years ago

My objection to iterator_to_array goes for all your PR's, I won't comment on them individually.

damianopetrungaro commented 6 years ago

@Erikvv if you care about more memory allocation between each iteration than following functional approaches, then it's fine that the PR won't be merged.

This is an object is the most used when FP comes in place. But I'd also love to see a review from the maintainer of the project because IMHO allocating more memory but following the FP approaches may be a good choice in a wannabe FP repo.

phanan commented 6 years ago

But I'd also love to see a review from the maintainer of the project because IMHO allocating more memory but following the FP approaches may be a good choice in a wannabe FP repo.

When the purpose of the repo is indeed to provide "functional primitives," I don't think it's implied anywhere (and rightfully so), that the implementation underneath must be functional – especially when such an approach is prone to problems (as pointed out by @Erikvv).

damianopetrungaro commented 6 years ago

I never said "MUST" be functional 😄

lstrojny commented 6 years ago

Thanks for taking the time and refactoring the functions to be more functional. The design idea behind the library was to provide functional primitives and to provide them in a way that they can be used in real world applications without sincere performance implications. That’s why the default always was to go with a non-functional implementation, since it saves the overhead of creating a higher order function, the engine calling a function etc. pp. which is relatively slow in PHP. That’s why I don’t want to merge your refactorings as they stand since I don’t want to change the performance characteristics. If an implementation actually becomes faster when using more functional primitives and we have a micro benchmark to back that up, I would be convinced. Other than that: I think the changes you did to replace e.g. is_string with \is_string would make a lot of sense.

damianopetrungaro commented 6 years ago

Cool, thank you for the answer @lstrojny ! I'll try to add a benchamark comparation in future then i'll re-open those PRs.

Looking forward to merge #176