Skip to content

⚡ Improve GemExecutor performance by removing redundant DB query #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
acsoto wants to merge 1 commit into
base: main
Choose a base branch
from
Open

⚡ Improve GemExecutor performance by removing redundant DB query #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions modules/Gem/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
</repository>
<repository>
<id>placeholderapi</id>
<url>https://repo.extendedclip.com/content/repositories/placeholderapi/</url>
<url>https://repo.helpch.at/releases/</url>
</repository>
</repositories>

Expand All @@ -78,8 +78,38 @@
<dependency>
<groupId>me.clip</groupId>
<artifactId>placeholderapi</artifactId>
<version>2.11.1</version>
<version>2.11.5</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.13.2</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>5.2.0</version>
<scope>test</scope>
Comment on lines +91 to +94
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid Mockito 5 on Java 8 builds

The module still targets Java 8 (<java.version>1.8</java.version> in this POM), but Mockito 5.x requires Java 11+. With mockito-core/mockito-inline set to 5.2.0, any mvn test or CI that runs under Java 8 will fail to load Mockito classes before tests execute. To keep tests runnable on the project’s declared Java level, use a Mockito 4.x release (or raise the project’s Java version).

Useful? React with 👍 / 👎.

</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
<version>5.2.0</version>
Comment on lines +93 to +99
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mockito 5.2.0 requires Java 11 or higher, but this project targets Java 1.8 (see line 15). This will cause compatibility issues when running tests. Consider using Mockito 3.x (e.g., 3.12.4) which is the last version series that supports Java 8, or update the project's Java version to at least Java 11.

Suggested change
<version>5.2.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
<version>5.2.0</version>
<version>3.12.4</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
<version>3.12.4</version>

Copilot uses AI. Check for mistakes.
<scope>test</scope>
</dependency>
<dependency>
<groupId>net.bytebuddy</groupId>
<artifactId>byte-buddy</artifactId>
<version>1.14.19</version>
<scope>test</scope>
</dependency>
Comment on lines +102 to +107
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description mentions adding Byte Buddy 1.14.19 "for Java 21 compatibility", but the pom.xml specifies Java 1.8 as the target version (line 15: <java.version>1.8</java.version>). This is misleading.

If the project is actually targeting Java 1.8, the Java 21 compatibility justification is incorrect. If the project needs Java 21 compatibility, the java.version property should be updated accordingly. Please clarify the actual Java version target and update either the description or the pom.xml configuration.

Copilot uses AI. Check for mistakes.
<dependency>
<groupId>net.bytebuddy</groupId>
<artifactId>byte-buddy-agent</artifactId>
<version>1.14.19</version>
<scope>test</scope>
</dependency>
Comment on lines +93 to +113
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mockito-inline dependency (version 5.2.0) also requires Java 11 or higher, but this project targets Java 1.8. This will cause compatibility issues. Additionally, mockito-inline is typically only needed for mocking static methods and final classes. If you're using it for static field manipulation (as seen in the test), consider using PowerMock with Mockito 3.x for Java 8 compatibility, or update the project to Java 11+.

Suggested change
<version>5.2.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
<version>5.2.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>net.bytebuddy</groupId>
<artifactId>byte-buddy</artifactId>
<version>1.14.19</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>net.bytebuddy</groupId>
<artifactId>byte-buddy-agent</artifactId>
<version>1.14.19</version>
<scope>test</scope>
</dependency>
<version>3.12.4</version>
<scope>test</scope>
</dependency>

Copilot uses AI. Check for mistakes.
</dependencies>
</project>
2 changes: 1 addition & 1 deletion modules/Gem/src/main/java/com/mcatk/gem/GemExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public Integer getTotalGems(String name) {

public void addGems(String name, int addGems) {
Integer gems = MySQLManager.getInstance().getGems(name);
if (MySQLManager.getInstance().getGems(name) == null) {
if (gems == null) {
MySQLManager.getInstance().insertData(name);
gems = 0;
}
Expand Down
79 changes: 79 additions & 0 deletions modules/Gem/src/test/java/com/mcatk/gem/GemExecutorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package com.mcatk.gem;

import com.mcatk.gem.sql.MySQLManager;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

import java.lang.reflect.Field;
import java.util.logging.Logger;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.*;

@RunWith(MockitoJUnitRunner.class)
public class GemExecutorTest {

@Mock
private MySQLManager mySQLManagerMock;

@Mock
private Gem gemMock;

private GemExecutor gemExecutor;

@Before
public void setUp() throws Exception {
// Mock MySQLManager.instance
setStaticField(MySQLManager.class, "instance", mySQLManagerMock);

// Mock Gem.plugin
setStaticField(Gem.class, "plugin", gemMock);

gemExecutor = new GemExecutor();
}

@After
public void tearDown() throws Exception {
setStaticField(MySQLManager.class, "instance", null);
setStaticField(Gem.class, "plugin", null);
}

private void setStaticField(Class<?> clazz, String fieldName, Object value) throws Exception {
Field field = clazz.getDeclaredField(fieldName);
field.setAccessible(true);
field.set(null, value);
}
Comment on lines +47 to +51
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test uses reflection to set static fields, which is fragile and can break with different JVM configurations or security managers. Since the code uses MySQLManager.getInstance() and Gem.getPlugin() which implement singleton patterns, consider refactoring the production code to use dependency injection instead. This would make testing easier and more robust without requiring reflection hacks.

Alternatively, if reflection must be used for testing singletons, consider adding a reset method to these singleton classes specifically for testing purposes, which would be safer than direct field manipulation.

Copilot uses AI. Check for mistakes.

@Test
public void testAddGems_Performance() {
String playerName = "TestPlayer";
int gemsToAdd = 100;

// Simulate new user: getGems returns null
when(mySQLManagerMock.getGems(playerName)).thenReturn(null);

// Mock getTotal to return a value (e.g. 0)
when(mySQLManagerMock.getTotal(playerName)).thenReturn(0);

gemExecutor.addGems(playerName, gemsToAdd);
Comment on lines +58 to +64
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test verifies that getGems is called only once, which is correct for the performance optimization. However, the test setup mocks getTotal to return 0, which masks a potential NullPointerException bug in the production code. When a new user is created (gems is null), getTotal will also return null, and the code on line 53 of GemExecutor.java attempts to add to this null value, which will throw a NullPointerException.

Consider adding an additional test case that mocks getTotal to return null (matching the actual behavior for new users) to expose this bug and verify it's handled correctly.

Copilot uses AI. Check for mistakes.

// Verify getGems is called exactly ONCE (target behavior)
verify(mySQLManagerMock, times(1)).getGems(playerName);

// Verify insertData is called
verify(mySQLManagerMock).insertData(playerName);

// Verify update calls
verify(mySQLManagerMock).setGems(eq(playerName), eq(gemsToAdd));
verify(mySQLManagerMock).setTotal(eq(playerName), eq(gemsToAdd));

// Verify log
verify(gemMock).log(anyString());
}
}
Loading