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

LotterySolution #1293

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

LotterySolution #1293

wants to merge 2 commits into from

Conversation

VitaliyProgrammer
Copy link

No description provided.

Rommelua
Rommelua suggested changes Dec 17, 2023
View reviewed changes
Copy link

@Rommelua Rommelua left a comment

Choose a reason for hiding this comment

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

Java Cl checks failed. Before making a Pull Request (or a new commit to an already existing PR), you need to write the mvn clean package command in the terminal and make sure that all checks pass. After that, the mentor will be able to review the homework

image

@VitaliyProgrammer VitaliyProgrammer force-pushed the new_branch branch 2 times, most recently from 94def62 to 49fae00 Compare December 18, 2023 13:18
@VitaliyProgrammer
Copy link
Author

Please, check my new commit "LotteryRemodeled"

sokimaaa
sokimaaa suggested changes Dec 18, 2023
View reviewed changes
src/main/java/core/basesyntax/ColorSupplier.java Outdated
//Creating type of color
public static Color getRandomColor() {

Random random = new Random();
Copy link

Choose a reason for hiding this comment

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

random should be instantiated once and reused multiple times,

nor creating each time the getRandomColor method is invoked

src/main/java/core/basesyntax/ColorSupplier.java Outdated
return null;

//Creating type of color
public static Color getRandomColor() {
Copy link

Choose a reason for hiding this comment

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

Dont use static

Suggested change
public static Color getRandomColor() {
public Color getRandomColor() {

src/main/java/core/basesyntax/Lottery.java Outdated
public class Lottery {
public static Ball getRandomBall() {
String colorRandom = ColorSupplier.getRandomColor().toString(); //1
Random random = new Random();
Copy link

Choose a reason for hiding this comment

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

random should be instantiated once

src/main/java/core/basesyntax/Lottery.java Outdated
public static Ball getRandomBall() {
String colorRandom = ColorSupplier.getRandomColor().toString(); //1
Random random = new Random();
int numberRandom = random.nextInt(100) + 1; //2
Copy link

Choose a reason for hiding this comment

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

100 is a magic number, avoid using magic numbers

should be constant with proper name

Suggested change
int numberRandom = random.nextInt(100) + 1; //2
int numberRandom = random.nextInt(100) + 1; //2

src/main/java/core/basesyntax/Lottery.java Outdated
import java.util.Random;

public class Lottery {
public static Ball getRandomBall() {
Copy link

Choose a reason for hiding this comment

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

Dont use static

Suggested change
public static Ball getRandomBall() {
public Ball getRandomBall() {

@VitaliyProgrammer
Copy link
Author

VitaliyProgrammer commented Jan 1, 2024 •
edited
Loading

Please, check my fixed commit "Fixed task balls"..

@VitaliyProgrammer VitaliyProgrammer force-pushed the new_branch branch 7 times, most recently from 6a3b9fe to 3e32932 Compare January 1, 2024 16:46
Rommelua
Rommelua suggested changes Jan 2, 2024
View reviewed changes
src/main/java/core/basesyntax/Ball.java Outdated
Comment on lines 3 to 4
public class Ball {

private Color color;
Copy link

Choose a reason for hiding this comment

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

image

src/main/java/core/basesyntax/Ball.java Outdated
Comment on lines 20 to 18
+ '}';

}
Copy link

Choose a reason for hiding this comment

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

Suggested change
+ '}';
}
+ '}';
}

src/main/java/core/basesyntax/Ball.java Outdated
public Ball(Color color, int number) {

this.color = color;
Copy link

Choose a reason for hiding this comment

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

image

src/main/java/core/basesyntax/ColorSupplier.java Outdated
Comment on lines 6 to 12

private final Color[] colors = Color.values();
private final Random random = new Random();

public Color getRandomColor() {

return colors[random.nextInt(colors.length)];
Copy link

Choose a reason for hiding this comment

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

image

@VitaliyProgrammer VitaliyProgrammer force-pushed the new_branch branch 6 times, most recently from 7792ffa to 3e32932 Compare January 2, 2024 13:52
@VitaliyProgrammer VitaliyProgrammer force-pushed the new_branch branch 2 times, most recently from d1b3ea3 to 3e32932 Compare January 2, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olekskov olekskov olekskov approved these changes

@sokimaaa sokimaaa Awaiting requested review from sokimaaa

@Rommelua Rommelua Awaiting requested review from Rommelua

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VitaliyProgrammer @Rommelua @olekskov @sokimaaa