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

Done #1733

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

Done #1733

wants to merge 9 commits into from

Conversation

mizuo-san
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Great job on implementing the task requirements! 🎉 Your code is well-structured and meets all the core requirements. While it's suggested to use a loop for creating multiple Ball objects to enhance code maintainability, this is a minor improvement and doesn't affect the functionality. Keep up the good work and consider this suggestion for future projects to make your code even more efficient! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

src/main/java/core/basesyntax/Application.java Outdated Show resolved Hide resolved
Copy link

@okuzan okuzan left a comment

Choose a reason for hiding this comment

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

Well done!

Comment on lines 7 to 12
Ball[] balls = new Ball[3];

for (int i = 0; i < 3; i++) {
balls[i] = lottery.getRandomBall();
System.out.println(balls[i]);
}
Copy link

Choose a reason for hiding this comment

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

we don't need array here, please try to rewrite without it

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

WHITE,
PURPLE
}

Copy link

Choose a reason for hiding this comment

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

Suggested change

let's make sure there's exactly one empty line at the end of each file, you have two here

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

public String getRandomColor() {
return null;
public Color getRandomColor() {
int picker = new Random().nextInt(Color.values().length);
Copy link

Choose a reason for hiding this comment

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

your code creates new Random instance for each method invocation, let's move it to the class level

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

ColorSupplier colorSupplier = new ColorSupplier();

Color color = colorSupplier.getRandomColor();
int number = new Random().nextInt(101);
Copy link

Choose a reason for hiding this comment

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

same for Random here
make 101 a constant

Copy link
Author

Choose a reason for hiding this comment

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

I made number 101 constant but how i can move this int number = new Random().nextInt(101); to the class level?

Copy link
Author

Choose a reason for hiding this comment

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

I guess maybe i fixed it

@mizuo-san mizuo-san requested a review from okuzan December 29, 2024 23:54
Copy link

@liliia-ponomarenko liliia-ponomarenko left a comment

Choose a reason for hiding this comment

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

Good job! A few minor comments ;)

public class ColorSupplier {
public String getRandomColor() {
return null;
private static final Random picker = new Random();

Choose a reason for hiding this comment

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

Suggested change
private static final Random picker = new Random();
private final Random random = new Random();

Don’t make it a static field


public class Lottery {
private static final int maxNum = 101;
private static final Random number = new Random();

Choose a reason for hiding this comment

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

the same about Random

private static final Random number = new Random();

public Ball getRandomBall() {
ColorSupplier colorSupplier = new ColorSupplier();

Choose a reason for hiding this comment

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

ColorSupplier also should make private final class-level variable

import java.util.Random;

public class Lottery {
private static final int maxNum = 101;

Choose a reason for hiding this comment

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

Suggested change
private static final int maxNum = 101;
private static final int MAX_NUMBER = 101;

use the correct constant style

Copy link

@liliia-ponomarenko liliia-ponomarenko left a comment

Choose a reason for hiding this comment

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

Awesome 😎

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