Stages 2,3,4,5 by kostylevv · Pull Request #2 · hyperskill/json-database

kostylevv · 2018-12-29T07:32:53Z

No description provided.

kforkartoshka

Hyperskill review

Good use of try-with-resources, but missed socket in Server.
Ok overall, the main issue is only with one-at-a-time connections

kforkartoshka · 2019-05-14T16:30:26Z

server/src/main/java/org/hyperskill/database/serverside/Server.java

+        try (ServerSocket server = new ServerSocket(PORT)) {
+            while (true) {
+                Session session = new Session(server.accept());
+                session.run(); // it does not block this thread


Only it does block it. Well, not really blocks, cause execution still continues, but it only serves one client.
You have another infinite loop in Session::run. So by calling it inside the first infinite loop, its iteration won't end until a client disconnects.

You can fix it by making Session into Thread

kforkartoshka · 2019-05-14T16:31:00Z

server/src/main/java/org/hyperskill/database/serverside/Server.java

+}
+
+class Session {
+    private final Socket socket;


Socket is never closed

kforkartoshka · 2019-05-14T16:31:25Z

server/src/main/java/org/hyperskill/database/serverside/Server.java

+
+                  System.out.println("Recieved: " + msg);
+                  output.writeUTF(msg);
+                  System.out.println("Sent: " + msg);


Wrong input and output messages format.
You only send integers, but the task is to send "Give me a record # N" from Client and "A record # N was sent!" from Server

kforkartoshka · 2019-05-14T16:31:46Z

client/src/main/java/org/hyperskill/database/clientside/Client.java

+
+                output.writeUTF(msg); // sending message to the server
+                System.out.println("Sent: " + msg);
+                String receivedMsg = input.readUTF(); // response message


Redundant comments, it's clear that you're sending and receiving something from the method names

kforkartoshka

It's OK but could be simplified and decoupled more
Try to write methods with single-responsibility in mind – each method does one thing. Now your main method does everything - parameters validation, client connection, command handling, reading and writing to socket
It's way harder to read such code and so you start to rely on comments that explain obvious stuff. But if you decouple program behavior, you'll find it easier to reason with.

kforkartoshka · 2019-05-15T13:34:44Z

client/src/main/java/org/hyperskill/database/clientside/Client.java

+                DataInputStream input = new DataInputStream(socket.getInputStream());
+                DataOutputStream output = new DataOutputStream(socket.getOutputStream())
+        ) {
+            if (client.command.equals("set")) {


In Commands validator you use value.trim().toLowerCase(), but here you don't preprocess command, can lead to a potential bug

Also, when checking strings against hardcoded values, better to switch their places, e.g. "set".equals(client.command), or use Objects.equals(client.command, "set"), to avoid NullPointerException
Another approach would be to use switch, e.g. switch (client.command.trim().toLowerCase()) { case "get": ... }

kforkartoshka · 2019-05-15T13:37:09Z

client/src/main/java/org/hyperskill/database/clientside/Client.java

+                output.writeUTF(client.command + " " + client.index);
+                System.out.println("Sent: " + client.command + " " + client.index);
+            }
+            String receivedMsg = input.readUTF(); // response message


Redundant comment
You can just name the variable response

kforkartoshka · 2019-05-15T13:40:40Z

server/src/main/java/org/hyperskill/database/serverside/Server.java

-                  System.out.println("Sent: " + msg);

+        int portNumber = Integer.parseInt(args[0]);
+        Protocol protocol = new Protocol();


Protocol class is missing from the source code

kforkartoshka · 2019-05-15T13:41:23Z

server/src/main/java/org/hyperskill/database/serverside/Server.java

+                        DataOutputStream output = new DataOutputStream(socket.getOutputStream())
+                ) {
+                    String inputLine, outputLine;
+                    inputLine = input.readUTF(); // reading a message


Redundant comments duplicating method names

kforkartoshka · 2019-05-15T13:44:50Z

client/src/main/java/org/hyperskill/database/clientside/Commands.java

+public class Commands implements IParameterValidator {
+    public void validate(String name, String value)
+            throws ParameterException {
+        value = value.trim().toLowerCase();


It's a bad practice to reassign input parameter
Better treat and mark them as final
More and more languages do that by default

kforkartoshka · 2019-05-15T13:48:38Z

client/src/main/java/org/hyperskill/database/clientside/Commands.java

+    public void validate(String name, String value)
+            throws ParameterException {
+        value = value.trim().toLowerCase();
+        if (!(value.equals("set") || value.equals("get") || value.equals("delete"))) {


That's fine, except for potential NPE, which I described in Client class
But could be simplified by creating a Set of allowed commands and checking if passed value is in this set
The set could also be used in exception message to create a string of allowed commands allowed.stream().collect(Collectors.joining("/"))

kforkartoshka

Ok overall, but try to separate logical parts more (with new lines or by extracting into separate methods)
Cause now your flow looks like this – createrequestparseittojsonsenditlogitreceiveresponselogit
It's just one indistinguishable blob of words

kforkartoshka · 2019-05-15T15:23:17Z

common/src/main/java/org/hyperskill/database/common/Request.java

+import java.util.Objects;
+
+public class Request {
+    public enum TYPE {SET, GET, DELETE}


For now it's okay, cause you use it only inside the Request class, but if you'd try to pass TYPE as a parameter somewhere else, you'll have to create an instance of 'Request' to access the enum. To avoid it make it static

Also, enum is just another class, and so follows the same naming convention (upper camel case, instead of all-caps). The values of enum are constants, and so they are written in caps, but not the enum itself

kforkartoshka · 2019-05-15T15:23:50Z

common/src/main/java/org/hyperskill/database/common/Response.java

+package org.hyperskill.database.common;
+
+public class Response {
+    public enum TYPE {OK, FAIL}


Same here as in Request

kforkartoshka · 2019-05-15T15:33:07Z

server/src/main/java/org/hyperskill/database/serverside/JsonProtocol.java

+import java.util.Objects;
+
+public class JsonProtocol {
+    JsonStorage storage = new JsonStorage();


Would be nicer and cleaner to introduce inversion of control here by creating and using Storage interface instead of concrete implementation

kforkartoshka · 2019-05-15T15:38:37Z

client/src/main/java/org/hyperskill/database/clientside/Commands.java

+import com.beust.jcommander.IParameterValidator;
+import com.beust.jcommander.ParameterException;
+
+public class Commands implements IParameterValidator {


Better name it CommandValidator, cause just Commands is ambiguous
The name of a class should reflect on what it does

kforkartoshka · 2019-05-15T15:38:59Z

client/src/main/java/org/hyperskill/database/clientside/PositiveInteger.java

+import com.beust.jcommander.IParameterValidator;
+import com.beust.jcommander.ParameterException;
+
+public class PositiveInteger implements IParameterValidator {


Same with naming as with Commands

kforkartoshka

Mostly okay again, with the same problems about code readability and structure as in previous stages
Nice thought of input parameters validation in methods

kforkartoshka · 2019-05-16T12:30:33Z

common/src/main/java/org/hyperskill/database/common/Util.java

+        String command = null;
+        String key = null;
+        StringBuilder value = new StringBuilder();
+        try (InputStream in = Files.newInputStream(file);


This whole thing could have been implemented in a couple of lines, using Files.lines(file)
But if you don't know about streams, it's ok

kforkartoshka · 2019-05-16T12:31:59Z

common/src/main/java/org/hyperskill/database/common/Util.java

+import java.nio.file.Paths;
+import java.util.Objects;
+
+public class Util {


Creating an "utils" class is tempting but actually is a bad practice. It tempts you to put unrelated stuff together into one class
Here you do IO operations, so the name should convey this

kforkartoshka · 2019-05-16T12:34:18Z

server/src/main/java/org/hyperskill/database/serverside/Server.java

        }
+        */
+    }
+/*


Don't commit commented-out code
It's just a noise
If you want to preserve it for potential future reference – git already did it for you, you can check file history for that

kforkartoshka · 2019-05-16T12:38:35Z

common/src/main/java/org/hyperskill/database/common/Request.java

        return value;
    }
+
+    public static TYPE validateCommand(String word) {


It's not validation but parsing
Also, it could be done with Enum::valueOf method, which will also throw IllegalArgumentException on unknown parameter

kforkartoshka · 2019-05-16T12:40:06Z

common/src/main/java/org/hyperskill/database/common/Response.java

    public enum TYPE {OK, FAIL}
+
+    public boolean isOk() {
+        if (this.type == TYPE.OK) {


Again, when your method returns boolean, you can just return boolean expression used in if clause

kforkartoshka · 2019-05-16T12:45:21Z

common/src/main/java/org/hyperskill/database/common/Response.java

+        return false;
+    }
+
+    public TYPE getType() {


Better have class parameters at the top of its definition. This way you and everyone else can clearly see what this class is constructed of
Your whole structure here is upside-down
It usually goes as follows:
– parameters
– code blocks
– constructors
– getters/setters
– methods
– utility

kforkartoshka · 2019-05-16T12:50:12Z

server/src/main/java/org/hyperskill/database/serverside/NetworkService.java

+        storage = JsonStorage.getINSTANCE();
+    }
+
+    public void run() { // run the service


Again, what's the purpose of such comments?
It's like writing import java.net.Socket; //import Socket class from java.net package

kforkartoshka · 2019-05-16T12:51:57Z

server/src/main/java/org/hyperskill/database/serverside/JsonStorage.java

+        writeLock = lock.writeLock();
+    }
+
+    public static JsonStorage getINSTANCE() {


No need to caps "instance" in getter name, it still follows camel-case in this situation

kforkartoshka · 2019-05-16T12:58:34Z

common/src/main/java/org/hyperskill/database/common/Request.java

    private String value;

-    public Request(String type, String key, String value) {
+    public Request(String type, String key, String value) throws IllegalArgumentException {


Why not use TYPE enum as type parameter type instead of String
Usage of String here only introduces potential failure and complicates logic where you additionally need to parse it

kforkartoshka · 2019-05-16T13:02:11Z

server/src/main/java/org/hyperskill/database/serverside/NetworkService.java

+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+class NetworkService implements Runnable {


Why making it Runnable if you're not using it as runnable
Calling nws.run(); in main doesn't execute it in a separate thread. You need to call start for that

kostylevv added 2 commits December 29, 2018 10:31

Stage 2

494e0f9

Stage hyperskill#3 implemented

11b3117

kostylevv changed the title ~~Stage 2~~ Stage 2,3 Jan 8, 2019

kostylevv changed the title ~~Stage 2,3~~ Stages 2,3 Jan 8, 2019

Stage #4 implemented

cea29ba

kostylevv changed the title ~~Stages 2,3~~ Stages 2,3,4 Jan 10, 2019

Stage #5 implemented

0abf2f5

kostylevv changed the title ~~Stages 2,3,4~~ Stages 2,3,4,5 Jan 29, 2019

kforkartoshka reviewed May 14, 2019

View reviewed changes

kforkartoshka reviewed May 15, 2019

View reviewed changes

kforkartoshka reviewed May 16, 2019

View reviewed changes

Conversation

kostylevv commented Dec 29, 2018

Uh oh!

kforkartoshka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kforkartoshka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kforkartoshka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kforkartoshka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants