Refactoring - Propuesta de Mejora

[gabordonp]

Hola @jzyong:

Somos un grupo de estudiantes de Diseño de Software de la ESPOL (Ecuador). Como parte de un trabajo del curso sobre Refactoring de Fowler, analizamos game-server y quisiéramos compartir tres hallazgos que parecen defectos reales y no preferencias de estilo, además de algunas limpiezas de menor prioridad. Con gusto abrimos un PR por cualquiera de ellos si le resultan útiles.

1. Creación de hilos sin límite en MailManager.sendTextMailAsync

sendTextMailAsync(...) crea un Thread nuevo en cada invocación. Ante una ráfaga de alertas —que es justamente cuando más probable es que se envíen correos— esto puede agotar los hilos del sistema operativo y provocar un OutOfMemoryError: unable to create new native thread, tumbando el servidor por una funcionalidad que no es crítica.

Un ExecutorService de tamaño fijo como campo acota la concurrencia y reutiliza los hilos:

private final ExecutorService mailExecutor = Executors.newFixedThreadPool(5);

public void sendTextMailAsync(String title, String content, String... recives) {
    mailExecutor.submit(() -> sendTextMail(title, content, recives));
}

2. Casteo sin verificación en HttpHandler.setParameter

setParameter(Object parameter) realiza un casteo directo a HttpResponseImpl. La verificación con instanceof que lo habría evitado existe en el archivo, pero está comentada. Cualquier llamador que pase un tipo incompatible provoca una ClassCastException que se propaga hacia el manejo de la sesión.

@Override
public void setParameter(Object parameter) {
    if (parameter instanceof HttpResponseImpl) {
        this.response = (HttpResponseImpl) parameter;
    }
}

3. Placeholder de log sin argumento en MailManager.initMailConfig

La advertencia usa un placeholder {} de SLF4J pero no recibe ningún argumento, así que la línea de log imprime {} literal en lugar de la ruta que realmente se buscó. Esto vuelve el mensaje inútil para diagnosticar una configuración faltante en producción.

// actual: LOGGER.warn("{}/mailConfig.xml未找到配置文件");
LOGGER.warn("{}/mailConfig.xml no encontrado, usando configuración por defecto", configPath);

Los bloques catch de sendTextMail tienen un problema relacionado: registran una cadena estática sin referencia al mensaje que falló, por lo que un operador no puede saber qué correo se perdió.

Limpiezas de menor prioridad

• HttpHandler.response() / responseWithStatus() duplican la misma estructura de verificación de nulo seguida de session.write(...). Extraer un método privado writeResponseToSession(...) la centraliza (DRY).
• HttpHandler.getParameter() instancia HttpResponseImpl de forma perezosa dentro de un getter, violando el principio de Separación Consulta-Comando (CQS): una lectura muta el estado del objeto de manera silenciosa.
• MailManager verifica mailConfig == null en varios métodos; inicializar el campo en el constructor elimina esas ramas.
• MathUtilTest valida testRandomVector3 y testRandomSeed con System.out.println en lugar de aserciones, por lo que las pruebas no pueden fallar en CI. Además declara PI y HALF_PI como campos de instancia pese a seguir la convención de nombres de constantes, y repite literales de tolerancia (0.01, 0.00488) en línea.
• "utf-8" está incrustado en message.setText(...); StandardCharsets.UTF_8.name() evita errores de tipeo.
• Importación sin uso: com.jzy.game.engine.handler.TcpHandler.

Notas

Tenemos versiones refactorizadas funcionales de MathUtilTest.java, HttpHandler.java y MailManager.java, y podemos dividirlas en PRs separados —uno por cada tema— para que cada uno pueda revisarse o rechazarse de forma independiente. Los puntos 1 a 3 son los que priorizaríamos; el resto queda a su criterio.

Gracias por mantener este proyecto: fue una base de código realmente útil para estudiar.