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 code snippet for Generate random numbers #116

Closed

Conversation

asilenko
Copy link

Added code snippet for Issue #105 :

  • created two methods:
    -> for random number between x and y
    -> for throwing dice
  • added tests for both methods,
  • updated Readme.md

Copy link
Owner

@iluwatar iluwatar left a comment

Choose a reason for hiding this comment

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

In addition to the review comments, there is a merge conflict that needs to be resolved.

* @param y right bound
* @return random integer between x and y
*/
public static int generateRandomNumberBetweenXAndY(int x, int y) {
Copy link
Owner

Choose a reason for hiding this comment

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

I would suggest generateRandomNumberBetween (leave out the end)

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines 48 to 61
/**
* Throw dice (3d6)
*
* @return random integer between 1 and 6 (inclusive)
*/
public static int throwDice() {
return random.nextInt(5) + 1;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe the function could take parameters of how many dice we have and what kind of dice it is. For example, parameters 3 and 6 would throw 3 times d6 dice giving a result between 3-18.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines 17 to 51
@Test
void testGenerateRandomNumberBetweenXAndY() {
var randomInteger0 = GenerateRandomNumbersSnippet.generateRandomNumberBetweenXAndY(0, 0);
assertEquals(0, randomInteger0);

var randomInteger1 = GenerateRandomNumbersSnippet.generateRandomNumberBetweenXAndY(3, 10);
var listOfPossibleResults1 = List.of(3, 4, 5, 6, 7, 8, 9, 10);
assertTrue(listOfPossibleResults1.contains(randomInteger1));

var randomInteger2 = GenerateRandomNumbersSnippet.generateRandomNumberBetweenXAndY(42, 43);
var listOfPossibleResults2 = List.of(42, 43);
assertTrue(listOfPossibleResults2.contains(randomInteger2));
}

/**
* Tests for {@link GenerateRandomNumbersSnippet#throwDice()}.
*/
@Test
void testThrowDice() {
var number = GenerateRandomNumbersSnippet.throwDice();
var listOfPossibleResults = List.of(1, 2, 3, 4, 5, 6);
assertTrue(listOfPossibleResults.contains(number));
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think parameterized tests would be very useful here to test more thoroughly. See https://www.baeldung.com/parameterized-tests-junit-5

Copy link
Author

Choose a reason for hiding this comment

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

Parameterized tests added. I'm new to junit, thanks for the opportunity to learn.

@asilenko asilenko force-pushed the 105-generate-random-numbers branch from 15cc998 to a01d1b1 Compare November 6, 2021 15:30
@asilenko
Copy link
Author

asilenko commented Nov 6, 2021

Ready for review.

@asilenko asilenko requested a review from iluwatar November 6, 2021 16:59
Copy link
Owner

@iluwatar iluwatar left a comment

Choose a reason for hiding this comment

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

You need to fix the failing build

@asilenko
Copy link
Author

You need to fix the failing build

I had some trouble working with checkstyle.xml It seems to be OK by now.
In addition I added one new test to check if all numbers from given range were generated.

@asilenko asilenko requested a review from iluwatar November 20, 2021 07:41
Copy link
Owner

@iluwatar iluwatar left a comment

Choose a reason for hiding this comment

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

The build is still failing due to Checkstyle issue. Please set up your IDE to work with Checkstyle and run the checks locally before submitting changes for review.

@asilenko
Copy link
Author

asilenko commented Jan 8, 2022

Sorry for the delay. Hope this last commit has fixed the problem with the checkstyle. I'm new to gradle. The project still do not build on my device but the reason is build.gradle file. I'm not sure if I should fix it along with this issue. Let me know if it's ok.

@asilenko asilenko requested a review from iluwatar January 8, 2022 22:39
Copy link
Owner

@iluwatar iluwatar left a comment

Choose a reason for hiding this comment

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

The build is still failing due to the missing license header and Checkstyle issues.

@iluwatar
Copy link
Owner

iluwatar commented Jan 9, 2022

@asilenko I added some more instructions in the PR #119 Maybe you find that helpful.

@asilenko
Copy link
Author

asilenko commented Jan 9, 2022

@asilenko I added some more instructions in the PR #119 Maybe you find that helpful.

Thank you. I fixed the licence, but I don't see any checkstyle warning when I try to build the project. If it still does not work just pass the task to someone else. Sorry for wasting your time.

@asilenko asilenko requested a review from iluwatar January 9, 2022 17:52
@grzesiekkedzior
Copy link
Collaborator

Ok, @asilenko I checked your code very fast and in my Idea everything compile successfully... almost. In class GenerateRandomNumbersSnippetTest change line 67. Line is longer than 100 characters. In checkstyle.xml at line 34 the max line length value equals 100. Line 67 has 102.

```java
public static int calculateLuhnChecksum(long num) {
public static int calculateLuhnChecksum(long num) {
Copy link
Owner

Choose a reason for hiding this comment

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

No need to modify this line

private static final Random random = new Random();

public static int generateRandomNumberBetween(int x, int y) {
int exclusiveRightBound = y - x + 1;
Copy link
Owner

Choose a reason for hiding this comment

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

use var keyword where possible

Comment on lines +60 to +75
@Test
void checkThatAllNumbersInTheSpecifiedRangeHaveBeenGenerated() {
List<Integer> listOfResults = new ArrayList<Integer>();
for (int i = 0; i < 200; i++) {
listOfResults.add(GenerateRandomNumbersSnippet.generateRandomNumberBetween(1, 10));
}
List<Integer> listOfDistinctResults =
listOfResults.stream()
.distinct()
.sorted(Integer::compareTo)
.collect(Collectors.toList());

List<Integer> expectedResult = List.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);

assertEquals(listOfDistinctResults, expectedResult);
}
Copy link
Owner

Choose a reason for hiding this comment

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

This test looks like it can randomly fail. I'm not sure I understand the purpose of the test either.

*
* @param numberOfDice represents number of dice being thrown
* @param typeOfDice represents number of sides of the dice
* @return random integer between 1 and 6 (inclusive)
Copy link
Owner

Choose a reason for hiding this comment

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

Not true, depends on the parameters

Comment on lines +80 to +89
@CsvSource({
"1, 6",
"2, 6",
"3, 6",
"1, 10",
"2, 10",
"10, 6",
"10, 10",
"20, 5"
})
Copy link
Owner

Choose a reason for hiding this comment

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

The test data is lacking 0 and negative numbers

Copy link

@PraveenNanda124 PraveenNanda124 left a comment

Choose a reason for hiding this comment

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

Good work

@stale
Copy link

stale bot commented Dec 31, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the status: stale label Dec 31, 2022
@stale
Copy link

stale bot commented Feb 14, 2023

Closed due to inactivity. Thank you for your contributions.

@stale stale bot closed this Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants