Conversation
|
Just want to mention that there were some changes to FilRequestHandler in Issue 19. Including static to non-static methods. (There is a pull request out now for it.) |
| private FileRequestHandler fileRequestHandler; | ||
|
|
||
| public ClientHandler(Socket socket) { | ||
| public ClientHandler(Socket socket, FileRequestHandler fileRequestHandler) { |
There was a problem hiding this comment.
Nice. It looks like you made fileRequestHandler a dependency injection so that you could test ClientHandler more easily.
ToniKaru
left a comment
There was a problem hiding this comment.
Looks good. Static methods made non-static and good to fix the package name to follow good praxis.
|
There is a problem with glf on my computer. I have to fix it before I can propperly review the code. (So someone else might have to review this :) ) |
helenahalldiniths
left a comment
There was a problem hiding this comment.
Look good!
Maybe we shouldn't change the package name in this issue since it is not part of the issue itself(It could be done separately in another issue). However it is a good idea and sould be done.
009b824
I have reverted the package name changes but now the issue needs to be reviewed again 😅 |
# Conflicts: # src/main/java/org/fungover/storm/server/Server.java
|
I went ahead and refactored the ClientHandler to work with the FileRequestHandler injection as well as merged the Server class from main |
Yuriks1
left a comment
There was a problem hiding this comment.
Looks good to me, well done
Closes issue #75.