Conversation
valeryrodina
force-pushed
the
feature/Lera_Rodina
branch
from
f892119 to
103b908
Compare
NikolaevArtem
added
bug
valeryrodina
force-pushed
the
feature/Lera_Rodina
branch
6 times, most recently
from
a69770e to
9d48202
Compare
valeryrodina
force-pushed
the
feature/Lera_Rodina
branch
from
9d48202 to
463cebd
Compare
NikolaevArtem
added
HW_1
Add course project without tests
NikolaevArtem
removed
bug
| @@ -0,0 +1,30 @@ | |||
|
|
|||
| Course project SEA BATTLE | |||
| } | ||
|
|
||
| shoot(); | ||
| return false; |
There was a problem hiding this comment.
What's the case when it will return false? Seems impossible
There was a problem hiding this comment.
let me check and fix if it is needed
| } | ||
|
|
||
| shoot(); | ||
| return false; |
There was a problem hiding this comment.
Same here: What's the case when it will return false? Seems impossible
There was a problem hiding this comment.
let me check and fix if it is needed
| displayer.clearScreen(); | ||
| displayer.displayMap(playerOne.getMap()); | ||
| displayer.displayRadar(playerOne.getRadar()); | ||
| if (playerOne.shoot()) { |
There was a problem hiding this comment.
That doesn't look good... usually methods should follow Single Responsibility Prinsiple - it should do only one thing, the thing it promises. In this case it's unexpected it returns boolean.
Have you tried finishing the game? I'm not sure it will not go to an infinite loop of recursion
There was a problem hiding this comment.
Oh, you're absolutely right. Seems that I forgot to add a condition for loop after last changes for this part:( Let me check
|
|
||
| import java.io.IOException; | ||
|
|
||
| public class Game { |
There was a problem hiding this comment.
Good! You introduced some abstractions, showed knowledge of Java Core and Java libraries. The code in general looks good, but there are a few cases where I'm not sure about the correct behavior. Also, your project lacks abstract classes/interfaces, and some architecture (the simplest is service/model). The interface is cool, I like it. playing is quite comfortable) I also appreciate automatic ship placement and description, it really helps to test.
Approved!
| import java.util.Objects; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class ObjectToJsonConverter { |
There was a problem hiding this comment.
Nice, thorough example, good job!
| import java.util.List; | ||
| import java.util.Scanner; | ||
|
|
||
| public class CustomFileReader { |
There was a problem hiding this comment.
Opt: use try-with-resources, otherwise if you get an exception, stream will not be closed
| String file = "src/main/resources/custom_file_reader/fileTest.txt"; | ||
| Path path = Paths.get("src/main/resources/custom_file_reader/fileTest.txt"); | ||
| new CustomFileReader(path).run3(); |
There was a problem hiding this comment.
Opt: That's quite disturbing, I think Path creation could be done in methods that require it...
|
|
||
|
|
||
|
|
||
| HashMap<MapProblemsMutableGenerator<String>, String> container_mutable_gen = new HashMap<>(); |
There was a problem hiding this comment.
There's no mutable key problem in your example
| public class CustomFileReaderTest extends UnitBase { | ||
|
|
||
| @Test() | ||
| public void givenWrongFile_whenRun1_thenException() { |
NikolaevArtem
left a comment
NikolaevArtem
left a comment
There was a problem hiding this comment.
Good! The code is generally easy to read, thoroughly tested, has nice examples/description. Minor fixes requested for HW_3 and HW_6.
NikolaevArtem
added
HW_3
HW_6
and removed
readyForReview
3 participants
No description provided.