Skip to content
This repository was archived by the owner on Oct 25, 2021. It is now read-only.

Dima_Troshkin#14

Open
Allzza wants to merge 48 commits intomasterfrom
feature/DimaTroshkin
Open

Dima_Troshkin#14
Allzza wants to merge 48 commits intomasterfrom
feature/DimaTroshkin

Conversation

@Allzza
Copy link
Collaborator

@Allzza Allzza commented Jul 7, 2021

1st HW + README MODIFIED

@NikolaevArtem NikolaevArtem added the bug Code fix needed label Jul 9, 2021
@NikolaevArtem NikolaevArtem changed the title Feature/dima troshkin Dima_Troshkin Jul 11, 2021
@Allzza Allzza added the readyForReview Sign for Artem to take a look label Jul 12, 2021
@NikolaevArtem NikolaevArtem removed the readyForReview Sign for Artem to take a look label Jul 13, 2021
Allzza and others added 2 commits July 14, 2021 21:09
Co-authored-by: Artem Nikolaev <Nikolaev.artem.work@yandex.ru>
@Allzza Allzza added the readyForReview Sign for Artem to take a look label Jul 14, 2021
@NikolaevArtem NikolaevArtem removed the readyForReview Sign for Artem to take a look label Jul 15, 2021
@Allzza Allzza added the readyForReview Sign for Artem to take a look label Jul 19, 2021
@Allzza Allzza removed the readyForReview Sign for Artem to take a look label Jul 19, 2021
@NikolaevArtem NikolaevArtem removed readyForReview Sign for Artem to take a look readyForStudentReview labels Sep 20, 2021


public static void main(String[] args) throws IOException {
Battle.run();
Copy link
Owner

Choose a reason for hiding this comment

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

Quite good! You did PvP seabattle. Looks nice, interface is comfortable. Opt: add delays/enter between players, so they will not see each other fields. Also if ship is sunk, near cells can be marked as hit. But the game is good as-is.

Copy link
Owner

Choose a reason for hiding this comment

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

Nice abstractions, model/service segregation, the code is easy to read in general, I didn't find any bugs. Approved!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it was planned, but had no time left. Thank you for checking. Will work on it later and have a whale of a time with my mates, encouraging them go programming)

public class Main {


public static void main(String[] args) throws IOException {
Copy link
Owner

Choose a reason for hiding this comment

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

Opt: remove static

public class Main {
public static void main(String[] args) {

new FirstProgram(); // with default value given by annotation
Copy link
Owner

Choose a reason for hiding this comment

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

Good!

Comment on lines +8 to +14
private static class SingletonHolder {
public static final Singleton INSTANCE_HOLDER = new Singleton();
}

public static Singleton getInstance() {
return SingletonHolder.INSTANCE_HOLDER;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Opt: why do you need static class, why not to have just static variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, agree

Comment on lines 4 to 6
public interface KittenToCatFunction<Kitten> {
Cat grow(Kitten kitten);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Opt: Usually generics are letters, like K or C. Also I don't understand why you have only one generic - for my understanding, here should be 2 or none

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I should go through that topic again

Copy link
Owner

@NikolaevArtem NikolaevArtem left a comment

Choose a reason for hiding this comment

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

Nice work on SeaBattle, looks good, both interface and code. Some fixes needed in HW_3, HW_4 and HW_6. Also, please remove any Java11 code from repository.

@NikolaevArtem NikolaevArtem added the bug Code fix needed label Sep 20, 2021
@Allzza Allzza added the readyForReview Sign for Artem to take a look label Sep 22, 2021
Comment on lines +4 to +5
public interface KittenToCatFunction<K extends Kitten> {
Cat grow(K k);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
public interface KittenToCatFunction<K extends Kitten> {
Cat grow(K k);
public interface KittenToCatFunction<K extends Kitten, C extends Cat> {
C grow(K kitten);

Opt: That's what I meant

@NikolaevArtem NikolaevArtem added HW_3 HW_4 HW_6 and removed readyForReview Sign for Artem to take a look bug Code fix needed labels Sep 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants