---

name: bitzero-review description: Review BitZero game server code for bugs, security issues, and testability. Use when reviewing handlers, extensions, sessions, or game logic.

BitZero Code Review

Review BitZero game server code with focus on multiplayer game patterns.

MCP Tools (zest-intellij server)

BitZero framework classes are in JARs. Use these MCP tools - they read JARs via IntelliJ.

Example: Reviewing a handler

Copy1. lookupClass("MyHandler")           → See method signatures
2. getTypeHierarchy("MyHandler")      → Check parent class
3. lookupClass("BaseClientRequestHandler") → Understand contract from JAR
4. findUsages("MyHandler", "handleClientRequest") → See call patterns

Review Checklist

1. Session & User Safety

Copy// Bad - no null check
User user = (User) request.getSender().getProperty("user");

// Good - defensive
ISession session = request.getSender();
if (session == null || !session.isLoggedIn()) {
    return;
}
User user = (User) session.getProperty("user");
if (user == null) {
    return;
}

2. Input Validation (DataCmd)

Copy// Bad
String itemId = cmd.readString();
int quantity = cmd.readInt();
buyItem(user, itemId, quantity);

// Good
String itemId = cmd.readString();
if (itemId == null || itemId.length() > 50) {
    sendError(user, "Invalid item");
    return;
}
int quantity = cmd.readInt();
if (quantity <= 0 || quantity > 999) {
    sendError(user, "Invalid quantity");
    return;
}

3. Game Exploit Prevention

Copy// Bad - client-trusted
int damage = cmd.readInt();
enemy.takeDamage(damage);

// Good - server-calculated
int damage = calculateDamage(user.getWeapon(), enemy.getDefense());
enemy.takeDamage(damage);

4. Response Handling

Copy// Bad - leaks exception
catch (Exception e) {
    send(new ErrorMsg(e.toString()), user);
}

// Good
catch (Exception e) {
    LOG.error("Error in handler", e);
    send(new ErrorMsg("Operation failed"), user);
}

5. Testability Assessment

Copy// Hard to test - direct singleton
public void handleClientRequest(User user, DataCmd cmd) {
    DatabaseService db = BitZeroServer.getInstance().getDatabase();
    db.save(user);
}

// Easier - but still needs integration test if singleton is deep
private final DatabaseService db;
public MyHandler(DatabaseService db) {
    this.db = db;
}

Don't force unit tests. Recommend integration/e2e tests when code is tightly coupled.

6. Thread Safety

Copy// Bad - TOCTOU race
if (user.getGold() >= price) {
    user.setGold(user.getGold() - price);
}

// Good
synchronized (user) {
    if (user.getGold() >= price) {
        user.setGold(user.getGold() - price);
    }
}

Output Format

Copy# Review: ClassName

## Summary
- **Risk Level**: High/Medium/Low
- **Testability**: Good/Fair/Poor (with recommended test type)
- **Issues Found**: N

## Critical Issues

### 1. [Issue Title]
**Location**: `method()` line N
**Problem**: Description
**Fix**:
```java
// suggested code

Medium Issues

...

Testability Assessment

Recommendation: [Specific test approach]

Copy
## Patterns to Flag

1. **Missing `@Override`** on handler methods
2. **Casting without instanceof** check
3. **Not closing resources** in finally/try-with-resources
4. **Hardcoded command IDs** instead of constants
5. **Direct `BitZeroServer.getInstance()`** in business logic
6. **Mutable static fields** in handlers
7. **Client-trusted values** for damage, gold, items

## Escoba-Server Specific Patterns

### Handler Switch Pattern

Escoba uses switch-case in handlers. Watch for:

```java
// Escoba pattern - AccountRequestHandler
@Override
public void handleClientRequest(User user, DataCmd dataCmd) {
    switch (dataCmd.getId()) {
        case CMD.USER_INFO:
            processUserInfo(user, dataCmd);
            break;
        case CMD.MONEY_IN_GAME:
            processMoneyInGame(user, dataCmd);
            break;
        // Missing default case?
    }
}

Review points:

• Does it have a default case?
• Are all CMD constants handled?
• Is user null-checked before use?

Singleton Usage Analysis

Escoba has heavy singleton usage (200+ getInstance() calls):

CopyUserDAOImpl.getInstance()      → Database access
BitZeroServer.getInstance()    → Server services
MsgService.sendByUID()         → Static messaging

Testability impact:

• Code with UserDAOImpl.getInstance() → Integration test
• Code with MsgService.sendByUID() → Integration test
• Pure calculation methods → Unit test

Game Logic Patterns (BaseGame)

Copy// Pure methods - testable
private int getCardValue(int cardId) { return cardId/4+1; }
private int checkBeginingEscoba() { /* sum check */ }
private int isBetterThan(List<Integer> l1, List<Integer> l2) { }

// Coupled methods - need integration test
private void notifyPlayCard() { /* uses MsgService */ }
private void processResult() { /* uses table.broadcast() */ }

Testability Quick Reference

Common Escoba Issues

1. Unchecked card operations

   Copy// Bad - no bounds check
   int card = listCardOpen.remove(index);
   
   // Good
   if (index >= 0 && index < listCardOpen.size()) {
       int card = listCardOpen.remove(index);
   }
   

2. Gold manipulation without locks

   Copy// Potential race in concurrent play
   player.setGold(player.getGold() + winnings);
   

3. Missing player validation

   Copy// Bad - trusts client
   int playerId = cmd.readInt();
   BasePlayer player = table.getPlayer(playerId);
   
   // Good - verify sender is the player
   BasePlayer player = table.getPlayerByUserId(user.getId());