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

Delete NumberUtils #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dhughes-xumak
Copy link
Member

@dhughes-xumak dhughes-xumak commented Sep 5, 2017

@dhughes-xumak
Copy link
Member Author

dhughes-xumak commented Sep 5, 2017

Blocked until DantaFramework/JahiaDF#6 is accepted.

@joransky-xk
Copy link
Member

So this class has some use... basically you can do a bunch of useful number stuff.
It can easily get numbers within a certain range, calculate primes, etc.
Primes are useful when trying to create cycles that rarely or never overlap (good for concurrent operations), as well as computationally unlikely numbers (good for priorities).
It's not critical, but could certainly be something that could be extended with more features, and is worth keeping.

@dhughes-xumak
Copy link
Member Author

Within the Danta projects, there was only a single usage of this class, and it was simply to set a particular CP's priority to a prime. That can easily be captured as a literal.

Primes are useful when trying to create cycles that rarely or never overlap (good for concurrent operations), as well as computationally unlikely numbers (good for priorities).

I don't follow the logic here. By using a system which generates primes, the primes are made statistically likely, since there is a limited pool of options to choose from. If we want to generate statistically unlikely numbers, a random integer would have a significantly lower collision rate than a random prime.

Finally, there are plenty of Number and Prime libraries available on the web already. Why roll our own when we can use something like Commons-Math Primes (which at least part of this class appears to have been copied from).

@nimsothea
Copy link
Contributor

Guys, we need to come to agreement on this as it's a blocker for moving to 1.0 release.

@dhughes-xumak
Copy link
Member Author

I wouldn't consider this a blocker. Version 1.0 could certainly be released without this cleanup, however I do feel pretty strongly about removing it.

@nimsothea
Copy link
Contributor

Well, in https://github.com/DantaFramework/AEM, there's a pull request to merge develop to master and it contained "Delete NumberUtils".

@dhughes-xumak
Copy link
Member Author

While that is true, DantaFramework/AEM#9 has already been merged to develop, and DantaFramework/AEM#10 merged develop to master.

AEM depends on this project, not the other way around. Even if all usages of NumberUtils (DantaFramework/AEM#9 and DantaFramework/JahiaDF#6) have been removed, NumberUtils (this PR) could certainly be left in the project for the initial release; though it may raise questions as to why a utility with no usages has been included in the codebase.

@nimsothea
Copy link
Contributor

@dhughes-xumak @jbarrera-xumak @neozilon I recommend that we leave this out of 1.0 release for now.

Copy link
Member

@jbarrera-xumak jbarrera-xumak left a comment

Choose a reason for hiding this comment

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

I agree with remove it.

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.

4 participants