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

Chertov_Nikita#18

Open
Sotheres wants to merge 36 commits intomasterfrom
feature/ChertovNikita
Open

Chertov_Nikita#18
Sotheres wants to merge 36 commits intomasterfrom
feature/ChertovNikita

Conversation

@Sotheres
Copy link
Collaborator

@Sotheres Sotheres commented Jul 8, 2021

No description provided.

@NikolaevArtem NikolaevArtem added the HW_1 Good to go label Jul 9, 2021
@NikolaevArtem NikolaevArtem modified the milestone: homework_1 is done Jul 9, 2021
@NikolaevArtem NikolaevArtem changed the title Feature/chertov nikita Chertov_Nikita Jul 11, 2021
Sotheres added 4 commits July 13, 2021 14:39
PyramidPrinter update
RandomCharsTable update
@Sotheres Sotheres added the readyForReview Sign for Artem to take a look label Jul 14, 2021
Comment on lines 59 to 65
String line = scan.next();
String[] input = line.split(":");
int hh = Integer.parseInt(input[0]);
int mm = Integer.parseInt(input[1]);
int ss = Integer.parseInt(input[2]);
validateInputMode2(hh, mm, ss, input.length);
seconds = ss + mm * 60 + hh * 3600;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can make it much shorter, literally in a line. Check Class java.time.LocalTime. Have a look at parse() and toSecondOfDay() methods. It's better to use a ready-made bicycle than to reinvent it :)

Copy link
Collaborator Author

@Sotheres Sotheres Jul 17, 2021

Choose a reason for hiding this comment

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

Thanks!)
Didn't know about that class' functionality.

You can make it much shorter, literally in a line. Check Class java.time.LocalTime. Have a look at parse() and toSecondOfDay() methods. It's better to use a ready-made bicycle than to reinvent it :)

@NikolaevArtem NikolaevArtem removed the readyForReview Sign for Artem to take a look label Jul 20, 2021
public void printTable() {
charList = new LinkedList<>();
for (int i = 0; i < rows; i++) {
System.out.print("| ");
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove whitespace, it fail my tests. In AC the output is without whitespaces

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@NikolaevArtem
Copy link
Owner

Good, the apps run as expected. I'll continue review when I'll have time

public class Main {

public static void main(String[] args) {
new SeaBattle().playVsComputer();
Copy link
Contributor

@ArtemNikolaev1 ArtemNikolaev1 Sep 20, 2021

Choose a reason for hiding this comment

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

Good use of abstractions, thorough knowledge of Java Core aspects and libraries. Well done!

To make better: Quite difficult to play, during ship placement the field does not show my placed ships. Also, you could Introduce random ship placement. And void methods are not thread-safe and easy to test, better use return values

Comment on lines +24 to +28
placePlayersShips();
placeComputerShips();
makeMoves();
defineWinner();
closeInput();
Copy link
Contributor

Choose a reason for hiding this comment

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

Opt: these methods should have return values. It helps to test them separately. Better not to use void methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opt: ship placement is difficult to understand (h1 hor on empty field does not work)

Comment on lines +31 to +33
void placePlayersShips() {
new UserShipPlacer(scanner).placeShips(playersField);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Opt: It does not make sense to put oneliner into a separate method

Comment on lines +28 to +30
int result = Integer.hashCode(x);
result = 31 * result + Integer.hashCode(y);
return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int result = Integer.hashCode(x);
result = 31 * result + Integer.hashCode(y);
return result;
return 31 * Integer.hashCode(x)+ Integer.hashCode(y);;

@NikolaevArtem NikolaevArtem added Course Project and removed readyForReview Sign for Artem to take a look labels Sep 20, 2021
* Immutable class is extremely well encapsulated!
*/

public final class Cafe {
Copy link
Owner

Choose a reason for hiding this comment

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

Good job!

import java.util.Scanner;
import java.util.stream.Stream;

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.

Many possible ways, good job!

Comment on lines +46 to +47
if (!tail.equals("")) {
throw new IllegalArgumentException();
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand the usage for this line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just an odd way to check if there is more then just two numbers...
Should simply use regex to validate input.

import homework_7.util.Kitten;

@FunctionalInterface
public interface KittenToCatFunction <R extends Cat, T extends Kitten> {
Copy link
Owner

Choose a reason for hiding this comment

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

Good, looks nice!

private final String errorMsg = "Only 1 non-negative integer is allowed as passed parameter";

@Test
void givenThree_whenRun_thenPrintPyramid() {
Copy link
Owner

@NikolaevArtem NikolaevArtem Sep 20, 2021

Choose a reason for hiding this comment

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

Special thanks for informative test cases names, it really helps

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.

Minor issues/code style fixes.
Except that, homework is accepted!

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.

4 participants