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

Tom_Shandrikov#51

Open
NikolaevArtem wants to merge 36 commits intomasterfrom
feature/TomShandrikov
Open

Tom_Shandrikov#51
NikolaevArtem wants to merge 36 commits intomasterfrom
feature/TomShandrikov

Conversation

@NikolaevArtem
Copy link
Owner

No description provided.

@NikolaevArtem
Copy link
Owner Author

I created a new PR on your behalf to resolve conflicts. You still need to add readme and fix code

@NikolaevArtem NikolaevArtem added the bug Code fix needed label Jul 15, 2021
@ashandrikov ashandrikov force-pushed the feature/TomShandrikov branch from 63586b4 to 6f86142 Compare July 20, 2021 11:37
e.printStackTrace();
}
// Корректно ли здесь возвращать null или нет?
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case upon returning null and assigning it to inputFull in line 23 you risk getting NullPointerExc in line 76 since you haven't checked s for null.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It'll be better to return an empty string. In that way when you will parse it further, in the parseInput() method, you get an exception you handle. Returning null it's not a good manner.

int x = (int) (Math.random() * 26 + 65);
table[i][j] = (char) x;
if (x % 2 == 0) {
strategyEven = strategyEven + " " + table[i][j] + ",";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because of frequent concatenation it's better to use StringBuilder type for this variables (there would be no new object creation for each concatenation).
Also there is no need to announce this variables as global because you pass reference each time you call methods to operate on them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, using StringBuilder allows you to remove the deleteLastCommaInStrategy() method because StringBuilder has a method deleteCharAt(int ind).

Comment on lines 56 to 58
}

if (strategy.equals("odd")) {
Copy link
Collaborator

@Sotheres Sotheres Jul 21, 2021

Choose a reason for hiding this comment

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

May just replace with:

} else {
    System.out.print(strategyOdd);
}

and do not check condition and call extra method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Try to use "text".equals(string) instead of string.equals("text"). Because the second form can throw NPE in some cases. Not in your case, but it's better to get used right form.
About whole code: I'm not sure about empty lines between strings of code but spaces and code style have been written correctly according to the code convention.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you guys for your remarks. Fixed it!

Comment on lines 98 to 101
if (str.endsWith(",")) {
return str.substring(0, str.length() - 1);
} else {
return str;
Copy link
Collaborator

@Sotheres Sotheres Jul 21, 2021

Choose a reason for hiding this comment

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

If you use StringBuilder instead there would be just
str.deleteCharAt(str.length() - 1);
and no need to return anything.

Then it's not necessary to add special method for this. Just do it in one line after filling string of letters.

@ashandrikov ashandrikov added the readyForReview Sign for Artem to take a look label Jul 21, 2021
ashandrikov
ashandrikov previously approved these changes Jul 21, 2021
@ashandrikov ashandrikov dismissed their stale review July 21, 2021 14:41

mistaked

# Conflicts:
#	README.md
#	src/main/java/homework_1/Main.java
@NikolaevArtem NikolaevArtem added HW_1 Good to go and removed bug Code fix needed labels Jul 22, 2021
@ashandrikov ashandrikov added readyForStudentReview and removed readyForReview Sign for Artem to take a look labels Jul 28, 2021
@NikolaevArtem NikolaevArtem removed the readyForReview Sign for Artem to take a look label Sep 21, 2021

import java.util.Scanner;

public class Game extends Thread {
Copy link
Owner Author

Choose a reason for hiding this comment

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

The fields look good, except that indentation is shifted
image

Copy link
Owner Author

@NikolaevArtem NikolaevArtem Sep 21, 2021

Choose a reason for hiding this comment

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

Opt: Also, please introduce automatic ship placement

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good job! Nice interface, well-chosen abstractions, architecture also looks fine. Good skills in Java Core and Java libraries. Special thanks for the description file and png, it helps.

To make better(opt): introduce automatic ship placement and PvE gameplay.

Approved!

Copy link
Collaborator

Choose a reason for hiding this comment

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

2021-09-22_10-03-10

I don't see any shifting. Maybe the reason is that I have windows, I don't know. How did you get in? In gameplay or in tests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Automatic ship placement: Done!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Player vs Computer(easy) mode: Done

public void run() {
setUpNames();
setUpShips();
startGame();
Copy link
Owner Author

Choose a reason for hiding this comment

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

it's more like play();

}
}

// You can uncomment this block of code instead above for() (lines: 28-42) to place only 2 ships for every player.
Copy link
Owner Author

Choose a reason for hiding this comment

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

That's a code smell, please remove

Comment on lines 3 to 4
public class MyShots extends Board {
}
Copy link
Owner Author

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 purpose of this class

Comment on lines 5 to 15
public abstract class InputReader {
protected Scanner scanner;

public String readLine() {
return scanner.nextLine();
}

public Scanner getScanner() {
return scanner;
}
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

The purpose of this class in also inclear

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the only question I have: how could I avoid this abstraction and use tests in this way:
String input = "a1 h\n" +
"a3 h\n" +
...
"\n";
setInput(input);
Because if I have Scanner.in inside class I am testing I have the same exception all the time: no line found. So I solved it this way. Solution for tests...

Parameterized constructor must perform Deep Copy.
*/

public final class ImmutableClass {
Copy link
Owner Author

Choose a reason for hiding this comment

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

The task description is not followed:
несколько конструкторов
метод, который возвращает модифицированный (новый) обьект

@Retention(RetentionPolicy.RUNTIME)
@Target(TYPE)
@Inherited
public @interface ClassCoffeeAnnotation {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Nice, good examples!

import java.nio.file.Paths;
import java.util.Scanner;

public class CustomFileReader {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Good!


//Basic Singleton

public class Singleton {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Nice!

package homework_7.kittenToCatFunction;

@FunctionalInterface
public interface KittenToCatFunction<C extends Cat, K extends Kitten>{
Copy link
Owner Author

Choose a reason for hiding this comment

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

Why do you need generics if you don't use them?

public static void main(String[] args) {

Kitten smallCat = new Kitten(1, "Push");
KittenToCatFunction function = x -> new Cat(x.getAge() + 2, x.getName(), smallCat);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Good example with Cat having Kitten as a field. I just don't understand why it always have smallCat, but not the provided one...

private String filename = "src/main/resources/custom_file_reader/file.txt";

@Test
void givenTextFile_whenRun1_thenEquals() throws IOException {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Nice names!

Copy link
Owner Author

@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.

Well-done! I'll approve your homework now, but you'll need to fix HW_3. In general - pretty good! I like that for many homework you experimented and did smth extra, not just the min requirement

Approved!

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.

6 participants