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.

Sign up for GitHub

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

Jump to bottom

Add RandGen for project-centric RNG functionality #59

Merged
merged 20 commits into from Sep 3, 2019
Merged

Add RandGen for project-centric RNG functionality #59

merged 20 commits into from Sep 3, 2019

Conversation

Plat251
Copy link
Contributor

@Plat251 Plat251 commented Aug 15, 2019

This should allow the developers to rely on a single common implementation of random number generation, with features like seed recording as well as strings as seeds.

@Plat251 Plat251 requested review from a user, orhanar and JohnnyIrvin August 15, 2019 20:36
@codecov-io
Copy link

codecov-io commented Aug 15, 2019 •
edited

Codecov Report

Merging #59 into development will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           development   #59   +/-   ##
=========================================
  Coverage            0%    0%           
=========================================
  Files               17    18    +1     
  Lines              123   145   +22     
  Branches             2     4    +2     
=========================================
- Misses             123   145   +22
Impacted Files Coverage Δ
...ldwarofants/game/module/utils/RandomGenerator.java 0% <0%> (ø)
src/main/java/com/worldwarofants/game/Main.java 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98bebe3...47a14d3. Read the comment docs.

@Plat251 Plat251 added the enhancement New feature or request label Aug 16, 2019
@Plat251 Plat251 removed the request for review from orhanar August 19, 2019 01:20
@Plat251 Plat251 changed the base branch from master to development August 20, 2019 17:26
@Plat251 Plat251 requested review from a team and root-ansh August 25, 2019 22:35
root-ansh
root-ansh reviewed Aug 26, 2019
View reviewed changes
Copy link

@root-ansh root-ansh left a comment

Choose a reason for hiding this comment

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

Hi. I added a couple of questions in the file below. Please answer. :)

Veradux
Veradux requested changes Aug 26, 2019
View reviewed changes
Copy link
Member

@Veradux Veradux left a comment

Choose a reason for hiding this comment

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

Try to give branches tags from now on. Like feature/random_generator, bugfix/generator_strings.

src/main/java/com/worldwarofants/game/util/RandomGenerator.java Outdated Show resolved Hide resolved
src/main/java/com/worldwarofants/game/util/RandomGenerator.java Outdated Show resolved Hide resolved
src/main/java/com/worldwarofants/game/util/RandomGenerator.java Outdated Show resolved Hide resolved
src/main/java/com/worldwarofants/game/util/RandomGenerator.java Outdated Show resolved Hide resolved
src/main/java/com/worldwarofants/game/util/RandomGenerator.java Outdated Show resolved Hide resolved
src/main/java/com/worldwarofants/game/util/RandomGenerator.java Outdated Show resolved Hide resolved
src/main/java/com/worldwarofants/game/util/RandomGenerator.java Outdated Show resolved Hide resolved
@Plat251 Plat251 requested a review from a team August 26, 2019 11:42
This was referenced Aug 27, 2019
Closed
Closed
alexdqmf
alexdqmf reviewed Aug 27, 2019
View reviewed changes
Copy link
Contributor

@alexdqmf alexdqmf left a comment

Choose a reason for hiding this comment

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

Alright, just some questions...

In the constructor, wouldn't it be better as super.setSeed instead of this.setSeed?

Also, we're going the Java route and doing include start and exclude end? This is something that might be good for us to keep a track of. If too many people keep doing a random with include end there'll be a lot of code with getIntInRange(start, end + 1).

Not something for you to change, though. Just something to keep in mind. I'm also not requesting changes because I'm not entirely sure of what I said before with super.setSeed.

@Plat251
Remove non-informative method javadocs
c8ae8df
@Plat251
Add Exception for range method, refactor overload
244cfff 
Because you're not supposed to give negative ranges to the range method,
it should indicate an issue if it's passed those.

It now does.

Additionally, the single-int overload of the method is now nicer.
@Plat251
Remove @Version from class javadoc
2ce3be5
@Plat251
Clean up javadocs and Random class reference
e3ffbdf
@Plat251
Remove comments for self-evident code
2c586e8
@Plat251
Rename Range method for clarity
899f640
@Plat251
Return end instead of 0 on input error for Range
97f1df4
@Plat251
Refactor exception in getIntInRange
c0d7c3c 
Clean up javadocs
@Plat251
Refactor to use inheritance instead of pet object
0532104
@Plat251
Move package to be compliant with new architecture
d86637a
@Plat251
Refactor redundant this.setSeed into setSeed
2c0628c
@Plat251
Simplify getIntInRange with one argument provided
6d66a06
@Plat251
Reformat for beauty
e53565f
@Plat251
Fix a bug where seedString is be set incorrectly
010d834 
Setting a seedString to the result of parsing it as a Long, as it was before this commit, runs it through a format process which can modify it in unexpected ways, like dropping leading zeroes. This doesn't align with the goal of preserving the original seedString input.
smithtj
smithtj suggested changes Aug 31, 2019
View reviewed changes
src/main/java/com/worldwarofants/game/module/utils/RandomGenerator.java Outdated Show resolved Hide resolved
src/main/java/com/worldwarofants/game/module/utils/RandomGenerator.java Outdated Show resolved Hide resolved
src/main/java/com/worldwarofants/game/module/utils/RandomGenerator.java
if (start > end) {
return end;
}
int randomNumber = this.nextInt(end - start);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a chance this could overflow, namely when (end - start) > Integer.MAX_VALUE. There's not a nice fix for this using the Random class as nextLong() doesn't have a bounded method.

daxtery
daxtery reviewed Aug 31, 2019
View reviewed changes
Copy link

@daxtery daxtery left a comment •
edited

Choose a reason for hiding this comment

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

I think separating the tests to another package might be best. Not sure what exactly should the structure be though

smithtj
smithtj approved these changes Aug 31, 2019
View reviewed changes
Copy link
Contributor

@smithtj smithtj left a comment

Choose a reason for hiding this comment

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

All of my suggestions have now been handled.

@Plat251 Plat251 requested a review from a team August 31, 2019 16:47
@JohnnyIrvin
Copy link
Contributor

@Plat251 Good to merge? Looks like so on my end?

@Plat251
Copy link
Contributor Author

Plat251 commented Sep 1, 2019

@RedRoyalDog
It is absolutely serviceable and has received multiple improvements over the course of its life. It's functional and comes with several unit tests to demonstrate just that.

@JohnnyIrvin JohnnyIrvin merged commit 81ca896 into warofants:development Sep 3, 2019
@JohnnyIrvin
Copy link
Contributor

@Plat251 merged.

@Veradux If there are issues with this, I will do a rollback. As for now, we're moving forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Veradux Veradux Veradux requested changes

@daxtery daxtery daxtery approved these changes

@alexdqmf alexdqmf alexdqmf approved these changes

@blackk100 blackk100 blackk100 approved these changes

@smithtj smithtj smithtj approved these changes

@root-ansh root-ansh root-ansh approved these changes

@NishantAcharya NishantAcharya NishantAcharya approved these changes

@JohnnyIrvin JohnnyIrvin Awaiting requested review from JohnnyIrvin

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@Plat251 @codecov-io @daxtery @Veradux @JohnnyIrvin @alexdqmf @blackk100 @smithtj @root-ansh @NishantAcharya