Code review
Opened this issue · 0 comments
Code Review 1
Reviewing version published on GitHub on 20.08. at 1637.
commit: 40f2488 (Fri Aug 20 16:37:35 2021 +0300)
Running fails on OS X
Running gradle run fails on OS X.
I tried running it with multiple different versions of java, gradle etc.
After replacing project.ext.lwjglNatives = "natives-linux" with project.ext.lwjglNatives = "natives-macos" I managed to run the application, but it did not provide any output during approximately 10 minutes.
tuure@Tuures-MacBook-Pro tiralabra-chzesa % java -version; gradle --version
java version "16.0.2" 2021-07-20
Java(TM) SE Runtime Environment (build 16.0.2+7-67)
Java HotSpot(TM) 64-Bit Server VM (build 16.0.2+7-67, mixed mode, sharing)
------------------------------------------------------------
Gradle 7.1.1
------------------------------------------------------------
Build time: 2021-07-02 12:16:43 UTC
Revision: 774525a055494e0ece39f522ac7ad17498ce032c
Kotlin: 1.4.31
Groovy: 3.0.7
Ant: Apache Ant(TM) version 1.10.9 compiled on September 27 2020
JVM: 16.0.1 (Homebrew 16.0.1+0)
OS: Mac OS X 11.5.1 x86_64
And in NetBeans 12.4:
AVA_HOME="/Library/Java/JavaVirtualMachines/jdk-16.0.2.jdk/Contents/Home"
cd /Users/tuure/tiralabra-chzesa; /Users/tuure/.gradle/wrapper/dists/gradle-7.0-bin/2p9ebqfz6ilrfozi676ogco7n/gradle-7.0/bin/gradle --configure-on-demand -x check run
Configuration on demand is an incubating feature.
> Task :compileJava
Note: /Users/tuure/tiralabra-chzesa/src/main/java/app/pq/PriorityQueue.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
> Task :processResources NO-SOURCE
> Task :classes
> Task :run FAILED
[LWJGL] Failed to load a library. Possible solutions:
a) Add the directory that contains the shared library to -Djava.library.path or -Dorg.lwjgl.librarypath.
b) Add the JAR that contains the shared library to the classpath.
[LWJGL] Enable debug mode with -Dorg.lwjgl.util.Debug=true for better diagnostics.
[LWJGL] Enable the SharedLibraryLoader debug mode with -Dorg.lwjgl.util.DebugLoader=true for better diagnostics.
Exception in thread "main" java.lang.UnsatisfiedLinkError: Failed to locate library: liblwjgl.dylib
at org.lwjgl.system.Library.loadSystem(Library.java:162)
at org.lwjgl.system.Library.loadSystem(Library.java:62)
at org.lwjgl.system.Library.<clinit>(Library.java:50)
at org.lwjgl.system.MemoryUtil.<clinit>(MemoryUtil.java:97)
at org.lwjgl.system.Pointer$Default.<clinit>(Pointer.java:67)
at org.lwjgl.system.Callback.<clinit>(Callback.java:41)
at app.App.initWindow(App.java:91)
at app.App.run(App.java:78)
at app.App.main(App.java:665)
FAILURE: Build failed with an exception.
Consider replacing a bunch of if-statements with a single switch-statement in App.java
Maybe this long list of if-statements at glfwSetKeyCallback (lines 108-160) could be replaced with a switch statement?
...
if (key == GLFW_KEY_1 && action == GLFW_RELEASE)
numSites = 1;
if (key == GLFW_KEY_2 && action == GLFW_RELEASE)
numSites = 2;
if (key == GLFW_KEY_3 && action == GLFW_RELEASE)
numSites = 3;
if (key == GLFW_KEY_4 && action == GLFW_RELEASE)
numSites = 4;
if (key == GLFW_KEY_5 && action == GLFW_RELEASE)
numSites = 5;
if (key == GLFW_KEY_6 && action == GLFW_RELEASE)
numSites = 6;
if (key == GLFW_KEY_7 && action == GLFW_RELEASE)
numSites = 7;
if (key == GLFW_KEY_8 && action == GLFW_RELEASE)
numSites = 8;
if (key == GLFW_KEY_9 && action == GLFW_RELEASE)
numSites = 9;
if (key == GLFW_KEY_0 && action == GLFW_RELEASE)
numSites = 10;
PriorityQueue.java
Is the best practice in Java for creating a generic array to create an array of Objects and then just casting them to desired type?
I feel like it's asking for weird and hard to debug runtime exceptions.
ArrayList apparently provides an easier way to implement a generic array.
Maybe something like this: private List<T> items; and this.items = new ArrayList<>(this.capacity) would be an easier solution?
And even if use of an ArrayList is off limits, type casting could be used only once by doing it while initializing of the array. For example something like this: items=(T[]) Array.newInstance(classTypeT, this.capacity) or this: T[] arr = (T[])new Object[capacity];
See also: https://www.baeldung.com/java-generic-array
Tests
No messages in assertions.
Maybe you could add a message to assertions as it makes it easier to grasp why the tests are suddenly failing. I didn't come across any test that would provide a message that is simple and easy to understand.
Excessive use of assertTrue instead of assertEquals
You should use assertEquals(objA, objB) instead of assertTrue(objA.equals(objB)) as assertEquals provides an informative message instead of just failing.
For example, I replaced a failing assertTrue on line 23 of ParseTest.java with assertEquals and now I have a way more informative error message.
(assertTrue(list.equals(list2)); -> assertEquals(list, list2);). Message: java.lang.AssertionError: expected:<[(1,000, 0,000), (0,000, 1,000)]> but was:<[]>.