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

Rog_Elena#35

Open
RogElenaYu wants to merge 65 commits intomasterfrom
feature/RogElena
Open

Rog_Elena#35
RogElenaYu wants to merge 65 commits intomasterfrom
feature/RogElena

Conversation

@RogElenaYu
Copy link
Collaborator

No description provided.

@RogElenaYu RogElenaYu added the readyForReview Sign for Artem to take a look label Jul 10, 2021
@NikolaevArtem NikolaevArtem added HW_1 Good to go and removed readyForReview Sign for Artem to take a look labels Jul 11, 2021
@NikolaevArtem NikolaevArtem changed the title Feature/rog elena Rog_Elena Jul 11, 2021
@RogElenaYu RogElenaYu added readyForReview Sign for Artem to take a look HW_2 labels Jul 26, 2021
@NikolaevArtem NikolaevArtem removed bug Code fix needed readyForReview Sign for Artem to take a look readyForStudentReview labels Sep 23, 2021
import java.util.TreeMap;

@Data
public class SeaBattle {
Copy link
Owner

Choose a reason for hiding this comment

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

I couldn't play the game... it just takes so long to start, to place ships manually for 2 players, especially because I need to input all the cells of a ship... Please refactor, I'll check then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!

@@ -0,0 +1,7 @@
package homework_2.random_chars_table;

public class Main {
Copy link
Owner

Choose a reason for hiding this comment

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

Good HW_2, with enums and inner classes!

@CustomAnnotation(v = 1.2f)
public class Main {
public static void main(String[] args) {
new Main().run();
Copy link
Owner

Choose a reason for hiding this comment

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

Nice example!

import java.nio.channels.FileChannel;
import java.nio.charset.StandardCharsets;

public class CustomFileReader {
Copy link
Owner

Choose a reason for hiding this comment

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

Opt: unnessessary method boxing and inner try-catches... seems not needed

Copy link
Owner

Choose a reason for hiding this comment

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

Looks like the code is hard to read without a reason

Copy link
Owner

Choose a reason for hiding this comment

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

Why all methods are public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tests ((... How can I test them then?

Copy link
Owner

Choose a reason for hiding this comment

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

You shouldn't test all methods. If your app is well-incapsulated, it has a few public methods (aka API of your class) and many private methods

System.out.println(singleGameHero2.hashCode() + ", age: " + singleGameHero2.getAge() + ", name: " + singleGameHero2.getName());
}

private Singleton() {}
Copy link
Owner

Choose a reason for hiding this comment

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

Opt: not needed

}

@Override
public boolean equals(Object obj) {
Copy link
Owner

@NikolaevArtem NikolaevArtem Sep 23, 2021

Choose a reason for hiding this comment

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

I'm really confused why all your methods in most of HW are public. By this you break one of the OOP principles - incapsulation

Copy link
Collaborator Author

@RogElenaYu RogElenaYu Sep 23, 2021

Choose a reason for hiding this comment

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

Oh ((. It's because of the tests. I need to learn to test private methods (or to build the architecture correctly).

Copy link
Owner

Choose a reason for hiding this comment

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

You shouldn't test private methods

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A-a-ah... I didn't know, I wrote all methods as private, but with the writing tests to them I needed to change them to public.

Comment on lines 18 to 19
Random random = new Random();
return random.nextInt();
Copy link
Owner

Choose a reason for hiding this comment

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

We looked at this problem on the lecture, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed (object's hashcode tied to the field's hashcode (date of birth)).

Comment on lines +21 to +23
KittenToCatFunction<Kitten, Cat> function = k -> new Cat(k.getName(), k.getSex(), k.getAgeInMonths() / 12, k.getColour());
Cat[] cats = kittens.stream().map(k -> function.grow(k)).toArray(Cat[]::new);
Arrays.stream(cats).forEach(System.out::println);
Copy link
Owner

Choose a reason for hiding this comment

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

Nice!

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.

Pretty good! You show good knowledge of Java Core and Java libraries, in HW you experimented with its features, and it looks in place. There are some minor issues to fix, and most important is - please fix Cource Project and ping me as soon as done

@RogElenaYu RogElenaYu added the readyForReview Sign for Artem to take a look label Sep 23, 2021
@@ -27,7 +27,8 @@ private enum Section {
private final Calendar yearOfPublishing;
private final int numberOfPages;
Copy link
Owner

Choose a reason for hiding this comment

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

Your class is 100% mutable

Copy link
Collaborator Author

@RogElenaYu RogElenaYu Sep 23, 2021

Choose a reason for hiding this comment

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

Ah, yearOfPublishing is a reference object (Calendar), that's why I have to return not current instance but also it's copy as Author?

Copy link
Owner

Choose a reason for hiding this comment

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

We care about reference type, mutable fields (you have 2 - Author and Calendar)

Copy link
Owner

Choose a reason for hiding this comment

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

Still not correct) Repeat the topic when you have time

Copy link
Collaborator Author

@RogElenaYu RogElenaYu Sep 23, 2021

Choose a reason for hiding this comment

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

I'll look at smarter students, I think, there is something I don't understand but I want very much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I understood, we needn't setters at all and we have to override some getters for reference fields.

import java.util.*;

@Data
public class Field {
Copy link
Owner

Choose a reason for hiding this comment

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

Good! The game is nice to play, the interface is good, playing is easy. 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.

Thanks for the science and advices!

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

Labels

HW_1 Good to go HW_2 HW_4 HW_5 HW_6 HW_7 readyForReview Sign for Artem to take a look

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants