Ping Pong #9 #94
Ping Pong #9 #94
Conversation
Add simple if statement for simple string reply to ping and removed superflous readline.
Co-authored-by: phteffe <phtefan.karlsson@gmail.com> Co-authored-by: JesperBodin <112554269+jesperbodin@users.noreply.github.com>
Co-authored-by: phteffe <phtefan.karlsson@gmail.com> Co-authored-by: JesperBodin <112554269+jesperbodin@users.noreply.github.com>
# Conflicts: # src/main/java/org/fungover/haze/Main.java
Co-authored-by: saftromo <safstrom.oliver@gmail.com> Co-authored-by: phteffe <phtefan.karlsson@gmail.com> Co-authored-by: JesperBodin <112554269+jesperbodin@users.noreply.github.com>
There was a problem hiding this comment.
The logic in the ping method and the accompanying tests look good, and the unit tests are passing so that's all good.
I'm not sure that I understand 100% how the code in the main method works, but I think passing the value variable as an argument to ping will get us in trouble here. What happens if we receive a command like "PING message\r\n"? I think readInputStream will opt for its else clause and fill the inputList with only two strings, which then would make getValueIfExist return an empty string. That would then be what would be passed into ping as an argument instead of "message" and ping itself would then also return an empty string. I'm speculating here though. It would be nice to get someone else to look at this as well.
I'm also a bit unsure about the actual format of the return from the ping method. Looking at the connected issue, the expected return string for a simple PING command seems to be a string starting with a + sign, and ending with \r\n, and otherwise be in the form of a bulk string. Maybe this is a standard we need to follow?
I think I got your point. The code is modified to be merged with cchriss123's #95 pull request for now. |
There was a problem hiding this comment.
Nicely done, looks awesome!
Yeah, I think we could just let it be as it is now, and then let the code that needs to use the function worry about how to pass in the correct arguments to it and call it.
Maybe the parameter name could be improved a little though, it might be a bit misleading.
|
You did not include the new line code in the HazeDatabase class, which I implemented. |
Merge branch 'main' into 9-ping-pong Co-authored-by: Ahlberg-iths <113943951+Ahlberg-iths@users.noreply.github.com>
Add ping case in executeCommand method Fix off by one err in ping method Add tests for ping in MainTest Co-authored-by: Ahlberg-iths <113943951+Ahlberg-iths@users.noreply.github.com>
|
Kudos, SonarCloud Quality Gate passed! |
Add ping method with tests