Codequality #117
Codequality #117
Conversation
…ld executor from a non static method
… on both client and serverside
…ed fields called in consoleclient and server constructor statically
|
I canceled the workflow because testing already took 1 hour and 50 Minutes. So I think there is something wrong... |
| static { | ||
| socket = null; | ||
| client = null; | ||
| menu = null; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is not the solution you are looking for. ✋
Static blocks are only seldom useful (as "constructors" for static classes, for example).
The real issue here is that the variables are static, but they are assigned in a non-static context (that is the constructor). You should think about which makes more sense: Having multiple ConsoleClientLauncher objects with different values for these variables, or having one single ConsoleClientLauncher that uses only static methods. (Or option 3: Use the singleton pattern.)
There was a problem hiding this comment.
You are absolutely right, I was insecure about supporting more clients at the same time, so i was trying to find a proper solution. Because if there is only one Launcher with static methods, multiclients are not possible or am i mixing things up in this case?
| public static void setExecutorPool() { | ||
| executor = Executors.newFixedThreadPool(2); | ||
| } |
There was a problem hiding this comment.
I am confused why this is required by SonarQube. This looks like an unnecessary method that could be replaced by the one line of code that it contains.
There was a problem hiding this comment.
@CptKaNe Can you tell me what the error is that you tried to fix here?
There was a problem hiding this comment.
yes, it also was referring of a static field (executor) in a non static context, but i guess you already find out because of the further feedback below. Lots of work to do for me ;-)
| static { | ||
| serverSocket = null; | ||
| server = null; | ||
| configObject = null; | ||
| } | ||
|
|
There was a problem hiding this comment.
Again, this is not what you want here. Decide how the class should be used: As instance, as static class, or as singleton, and refactor it accordingly.
|
|
||
| public void launchServer() { | ||
|
|
||
| System.setProperty(Log4jLoggerAdapter.ROOT_LOGGER_NAME, "TRACE"); | ||
|
|
||
| logger.info("Server socket listening on port " + configObject.port ); | ||
| System.out.flush(); | ||
| executor = Executors.newCachedThreadPool(); | ||
| //executor = Executors.newCachedThreadPool(); |
There was a problem hiding this comment.
There is no need to keep deleted code as comments in a git repository. That's what version control is for.
There was a problem hiding this comment.
Also, creating the executor in main() instead of launchServer() means that the tests now fail with a null pointer exception, since the ServerIntegrationTest calls launchServer() but not main().
There was a problem hiding this comment.
Well this is probably because i was not into the project for weeks and do not know every change since then, but yesterday I already had the feeling, theres something wrong with these changes. I will appreciate it.
| @@ -55,6 +57,9 @@ public void didChangeWatchedFiles(DidChangeWatchedFilesParams didChangeWatchedFi | |||
| case "Version": | |||
| result = modelicaService.getCompilerVersion(); | |||
| break; | |||
| default: | |||
| result = CompletableFuture.completedFuture(error); | |||
There was a problem hiding this comment.
Is this really what you want? It is a design choice, but inside a Java environment I would opt for a more rigid approach: When you get an unexpected input, you throw a InputMismatchException.
I have found the culprit: It is actually a |
|
Switched target branch to dev, so that we can actually use this pull request. |
- added exceptions for workspaceservice - updated syntax of commands, now start with lower letter - Tried to implement the singleton pattern, but removed this changes because they were wrong - ignored some unneccessary code smells from sonarqube
I created this pull request prematurely in order to be able to provide a comprehensive code review. This branch is still work in progress.