Clever Geek Handbook

Comprobando el código de XMage y por qué no están disponibles las cartas raras especiales para la colección Dragon's Maze

image1.png


XMage es una aplicación cliente / servidor para jugar Magic: The Gathering (MTG). XMage comenzó a evolucionar a principios de 2010. Durante este tiempo, se lanzaron 182 lanzamientos, se reunió todo un ejército de contribuyentes y el proyecto aún se está desarrollando activamente. ¡Una excelente oportunidad para participar también en su desarrollo! Por lo tanto, hoy el unicornio de PVS-Studio verificará el código base de XMage y, quién sabe, podría chocar con alguien en la batalla.



Brevemente sobre el proyecto



XMage se ha estado desarrollando activamente durante 10 años. Su objetivo es crear una versión en línea gratuita y de código abierto del juego de cartas original Magic: the Gathering .



Características de la aplicación:



  • acceso a ~ 19,000 tarjetas únicas emitidas durante los 20 años de historia de MTG;
  • control y aplicación automáticos de todas las reglas del juego existentes;
  • ;
  • (AI);
  • (Standard, Modern, Vintage, Commander );
  • , .




Tropecé con el trabajo de estudiantes de Delft University of Technology 2018 (Maestría en Arquitectura de Software ). Consistía en el hecho de que los chicos participaban activamente en proyectos de código abierto, que tenían que ser bastante complejos y desarrollarse activamente. Durante un período de ocho semanas, los estudiantes estudiaron el curso y los proyectos de código abierto para comprender y describir la arquitectura del software seleccionado.



Eso es todo. En este trabajo, los chicos analizaron el proyecto XMage, y uno de los aspectos de su trabajo fue obtener varias métricas usando SonarQube (número de líneas de código, complejidad ciclomática, duplicación de código, olores de código, errores, vulnerabilidades, etc.).



Me llamó la atención el hecho de que, en el momento del 2018, el escaneo de SonarQube mostraba 700 defectos (errores, vulnerabilidades) por cada 1.000.000 de líneas de código.



Después de indagar en la historia de los chicos que contribuyeron, descubrí que a partir del informe recibido con advertencias, hicieron una solicitud de extracción para corregir unos 30 defectos de la categoría "Bloqueador" o "Crítico". Se desconoce el resto de las advertencias, pero espero que no se hayan pasado por alto.



Han pasado 2 años desde entonces y la base de código ha crecido en unas 250.000 líneas de código, una buena razón para ver cómo van las cosas.



Sobre el análisis



Para el análisis, tomé el lanzamiento de XMage - 1.4.44V0 .



Tuve mucha suerte con el proyecto. La construcción de XMage usando Maven resultó ser muy simple (como estaba escrito en la documentación): 



mvn clean install -DskipTests


No se me pidió nada más. ¿Frio?



Tampoco hubo problemas con la integración del complemento PVS-Studio en Maven: todo está como en la documentación .



Después del análisis, se recibieron avisos al 911, de los cuales 674 fueron para avisos de 1 y 2 niveles de confianza. Para los propósitos de este artículo, no he considerado las advertencias de nivel 3, ya que suele haber un alto porcentaje de falsos positivos. Me gustaría llamar su atención sobre el hecho de que cuando se usa un analizador estático en una batalla real, no se pueden ignorar tales advertencias, ya que también pueden indicar defectos significativos en el código.



Además, no consideré las advertencias de algunas de las reglas por el motivo de que son mejor consideradas por quienes están familiarizados con el proyecto que yo:



  • V6022, /. 336 .
  • V6014, , . 73 .
  • V6021, , . 36 .
  • V6048, , . 17 .


Además, varias reglas de diagnóstico produjeron alrededor de 20 falsos positivos obvios del mismo tipo. Grabado en todo!



Como resultado, si restamos todo, entonces recibí alrededor de 190 positivos para considerarlos.



Al revisar los desencadenantes, se identificaron muchos defectos menores del mismo tipo, que estaban relacionados con la depuración o la verificación u operación sin sentido. Además, muchos aspectos positivos se asociaron con una pieza de código muy extraña que pedía una refactorización.



Como resultado, para este artículo he identificado 11 reglas de diagnóstico y analizado uno de los desencadenantes más interesantes.



Echemos un vistazo a lo que pasó.



Advertencia N1



V6003 Se detectó el uso del patrón 'if (card! = Null) {...} else if (card! = Null) {...}'. Existe una probabilidad de presencia de errores lógicos. TorrentialGearhulk.java (90), TorrentialGearhulk.java (102) 



@Override
public boolean apply(Game game, Ability source) {
  ....
  Card card = game.getCard(....);
  if (card != null) {
      ....
  } else if (card != null) {
      ....
  }
  ....
}


Aquí todo es simple: el cuerpo de la segunda declaración condicional if (card! = Null) en la construcción if-else-if nunca se ejecutará, ya que el programa no llegará a este punto, o card! = Null siempre será falso .



Advertencia N2



V6004 La instrucción 'then' es equivalente a la instrucción 'else'. AsThoughEffectImpl.java (35), AsThoughEffectImpl.java (37) 



@Override
public boolean applies(....) {
  // affectedControllerId = player to check
  if (getAsThoughEffectType().equals(AsThoughEffectType.LOOK_AT_FACE_DOWN)) {
    return applies(objectId, source, playerId, game);
  } else {
    return applies(objectId, source, playerId, game);
  }
}


Un error común que a menudo ocurría en mi práctica de verificar proyectos de código abierto. ¿Copiar pegar? ¿O me estoy perdiendo algo? Asumiré que aún necesita devolver falso en la rama else . PD En todo caso, no se aplica ninguna llamada recursiva (....) , ya que estos son métodos diferentes. Activación similar:











  • V6004 La instrucción 'then' es equivalente a la instrucción 'else'. GuiDisplayUtil.java (194), GuiDisplayUtil.java (198)


Advertencia N3



V6007 Expression 'filter.getMessage (). ToLowerCase (Locale.ENGLISH) .startsWith ("Each")' siempre es falsa. SetPowerToughnessAllEffect.java (107) 



@Override
public String getText(Mode mode) {
  StringBuilder sb = new StringBuilder();
  ....
  if (filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")) {
    sb.append(" has base power and toughness ");
  } else {
    sb.append(" have base power and toughness ");
  }
  ....
  return sb.toString();
}


Los activadores de la regla de diagnóstico V6007 son bastante populares para todos los proyectos que se verifican. XMage no es una excepción (79 piezas). La regla se activa, en principio, todo está en el caso, pero muchos casos caen en depuración, luego en reaseguro y luego en otra cosa. En general, es mejor para el autor del código ver esos aspectos positivos que para mí.



Esta operación, sin embargo, es definitivamente un error. Dependiendo del comienzo de la línea filter.getMessage () to sbse añade el texto "tiene ..." o "tiene ...". Pero el error es que los desarrolladores comprueban que la cadena comienza con una letra mayúscula, habiendo convertido esta misma cadena a minúscula antes. ¡Ups! Como resultado, la línea agregada siempre será "have ...". El resultado del defecto no es crítico, sino también desagradable: en alguna parte aparecerá un texto redactado de forma analfabeta.



Los aspectos positivos que encontré más interesantes:



  • V6007 La expresión 't.startsWith ("-")' siempre es falsa. BoostSourceEffect.java (103)
  • V6007 La expresión 'setNames.isEmpty ()' siempre es falsa. DownloadPicturesService.java (300)
  • La expresión V6007 'existingBucketName == null' es siempre falsa. S3Uploader.java (23)
  • V6007 Expression '! LastRule.endsWith (".")' Siempre es cierto. Effects.java (76)
  • V6007 La expresión 'subtypesToIgnore :: contains' siempre es falsa. VerifyCardDataTest.java (893)
  • La expresión V6007 'notStartedTables == 1' siempre es falsa. MageServerImpl.java (1330)


Advertencia N4



V6008 Desreferencia nula de 'SavedSpecialRares'. DragonsMaze.java (230) 



public final class DragonsMaze extends ExpansionSet {
  ....
  private List<CardInfo> savedSpecialRares = new ArrayList<>();
  ....
  @Override
  public List<CardInfo> getSpecialRare() {
    if (savedSpecialRares == null) {                    // <=
      CardCriteria criteria = new CardCriteria();
      criteria.setCodes("GTC").name("Breeding Pool");
      savedSpecialRares.addAll(....);                   // <=
      criteria = new CardCriteria();
      criteria.setCodes("GTC").name("Godless Shrine");
      savedSpecialRares.addAll(....);
      ....
    }
    return new ArrayList<>(savedSpecialRares);
  }
}


El analizador se queja de que se elimina la referencia a la referencia nula SavedSpecialRares cuando la ejecución alcanza el primer llenado de la colección.



Lo primero que me viene a la mente es simplemente confundir SavedSpecialRares == null con SavedSpecialRares! = Null. Pero en tal caso, NPE puede ocurrir en el constructor ArrayList cuando la colección se devuelve desde el método, ya que SavedSpecialRares == null todavía es posible. Arreglar el código con la primera solución que se te ocurra no es una buena opción. Después de comprender un poco el código, descubrí que s avedSpecialRares se define inmediatamente mediante una colección vacía cuando se declara y no se reasigna en ningún otro lugar. Esto nos dice queSavedSpecialRares nunca será nulo, y la desreferenciación de una referencia nula, sobre la que advierte el analizador, nunca ocurrirá, ya que nunca llegará a la colección. Como resultado, el método siempre devolverá una colección vacía.



PD: Para solucionarlo, debe reemplazar SavedSpecialRares == null con SavedSpecialRares.isEmpty () .



PPS Por desgracia, mientras juegas a XMage, no podrás obtener cartas raras especiales para la colección Dragon's Maze .



Otro caso de desreferenciar una referencia nula:



  • V6008 Desreferencia nula de "coincidencia". TableController.java (973)


Advertencia N5



V6012 El operador '?:', Independientemente de su expresión condicional, siempre devuelve el mismo valor 'table.getCreateTime ()'. TableManager.java (418), TableManager.java (418) 



private void checkTableHealthState() {
  ....
  logger.debug(.... + formatter.format(table.getStartTime() == null
                                        ? table.getCreateTime()
                                        : table.getCreateTime()) + ....);
  ....
}


Aquí el operador ternario ?: Devuelve el mismo valor independientemente de la condición table.getStartTime () == null . Creo que la finalización del código le ha jugado una broma cruel al desarrollador. Opción de corrección: 



private void checkTableHealthState() {
  ....
  logger.debug(.... + formatter.format(table.getStartTime() == null
                                        ? table.getCreateTime()
                                        : table.getStartTime()) + ....);
  ....
}


Advertencia N6



V6026 Este valor ya está asignado a la variable 'this.loseOther'. BecomesCreatureTypeTargetEffect.java (54) 



public
BecomesCreatureTypeTargetEffect(final BecomesCreatureTypeTargetEffect effect) {
  super(effect);
  this.subtypes.addAll(effect.subtypes);
  this.loseOther = effect.loseOther;
  this.loseOther = effect.loseOther;
}


Cadena de asignación duplicada. Parece que el desarrollador se dejó llevar un poco usando las teclas de acceso rápido y no lo notó. Pero dado que el efecto tiene una gran cantidad de campos, el fragmento merece ser enfocado.



Advertencia N7



V6036 Se utiliza el valor del opcional 'selectUser' no inicializado. Session.java (227) 



public String connectUserHandling(String userName, String password)
{
  ....
  if (!selectUser.isPresent()) {  // user already exists
      selectUser = UserManager.instance.getUserByName(userName);
      if (selectUser.isPresent()) {
          User user = selectUser.get();
            ....
      }
  }
  User user = selectUser.get(); // <=
  ....
}


De la advertencia del analizador, podemos concluir que selectUser.get () puede lanzar una NoSuchElementException.



Echemos un vistazo más de cerca a lo que está sucediendo aquí.



Si cree que el comentario de que el usuario ya existe, no se lanzará ninguna excepción: 



....
if (!selectUser.isPresent()) {  // user already exists
  ....
}
User user = selectUser.get()
....


En este caso, la ejecución del programa no entrará en el cuerpo de la declaración condicional. Y todo saldrá bien. Pero entonces surge la pregunta: ¿por qué necesitamos un operador condicional con algún tipo de lógica compleja si nunca se ejecuta?



Pero, ¿y si el comentario no es nada? 



....
if (!selectUser.isPresent()) {  // user already exists
    selectUser = UserManager.instance.getUserByName(userName);
    if (selectUser.isPresent()) {
      ....
    }
}
User user = selectUser.get(); // <=
....


Luego, la ejecución ingresa al cuerpo de la declaración condicional y vuelve a obtener al usuario a través de getUserByName (). Se vuelve a comprobar la validez del usuario, lo que sugiere que el selectUser podría no estar inicializado. No hay otra rama para este caso, lo que conducirá a una NoSuchElementException en la línea de código en cuestión.



Advertencia N8



V6042 Se comprueba la compatibilidad de la expresión con el tipo 'A' pero se convierte al tipo 'B'. CheckBoxList.java (586) 



/**
 * sets the model - must be an instance of CheckBoxListModel
 * 
 * @param model the model to use
 * @throws IllegalArgumentException if the model is not an instance of
 *           CheckBoxListModel
 * @see CheckBoxListModel
 */
@Override
public void setModel(ListModel model) {
  if (!(model instanceof CheckBoxListModel)) {
    if (model instanceof javax.swing.DefaultListModel) {
       super.setModel((CheckBoxListModel)model);         // <=
    }
    else {
      throw new IllegalArgumentException(
          "Model must be an instance of CheckBoxListModel!");
    }
  }
  else {
    super.setModel(model);
  }
}


El autor del código está confundido acerca de algo aquí: primero se asegura de que el modelo no sea un CheckBoxListModel y luego, como resultado, lanza explícitamente el objeto a este tipo. Debido a esto, el método setModel lanzará inmediatamente una ClassCastException cuando llegue allí.



El archivo CheckBoxList.java se agregó hace 2 años y este error persiste en el código desde entonces. Al parecer, no hay pruebas para parámetros incorrectos, no hay un uso real de este método con objetos de tipos inapropiados, por lo que vive.



Si de repente alguien se conecta a este método y lee el Javadoc, esperará una IllegalArgumentException , no una ClassCastException... No creo que nadie se tope deliberadamente con esta excepción, pero quién sabe.



Dada la documentación, lo más probable es que el código se vea así: 



public void setModel(ListModel model) {
  if (!(model instanceof CheckBoxListModel)) {
     throw new IllegalArgumentException(
        "Model must be an instance of CheckBoxListModel!");  
  }
  else {
    super.setModel(model);
  }
}


Advertencia N9



V6060 La referencia de 'jugador' se utilizó antes de que se verificara con un valor nulo. VigeanIntuition.java (79), VigeanIntuition.java (78) 



@Override
public boolean apply(Game game, Ability source) {
    MageObject sourceObject = game.getObject(source.getSourceId());
    Player player = game.getPlayer(source.getControllerId());
    Library library = player.getLibrary();                           // <=
    if (player != null && sourceObject != null && library != null) { // <=
        ....
    }
}


V6060 advierte al desarrollador que se está accediendo a un objeto antes de que se verifique si es nulo . Los desencadenantes de esta regla se encuentran a menudo en artículos sobre la verificación de proyectos de código abierto: por lo general, la razón de esto es la refactorización fallida o el cambio de contratos de métodos. Si prestas atención a la declaración del método getPlayer () , todo encajará inmediatamente en su lugar: 



// Result must be checked for null.
// Possible errors search pattern: (\S*) = game.getPlayer.+\n(?!.+\1 != null)
Player getPlayer(UUID playerId);


Advertencia N10



V6072 Se encontraron dos fragmentos de código similares. Quizás, esto es un error tipográfico y la variable 'playerB' debería usarse en lugar de 'playerA'. SubTypeChangingEffectsTest.java (162), SubTypeChangingEffectsTest.java (158), SubTypeChangingEffectsTest.java (156), SubTypeChangingEffectsTest.java (160) 



@Test
public void testArcaneAdaptationGiveType() {
    addCard(Zone.HAND, playerA, "Arcane Adaptation", 1); // Enchantment {2}{U}
    addCard(Zone.BATTLEFIELD, playerA, "Island", 3);

    addCard(Zone.HAND, playerA, "Silvercoat Lion");
    addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");
    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=

    addCard(Zone.HAND, playerB, "Silvercoat Lion");
    addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");
    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=

    ....

    for (Card card : playerB.getGraveyard().getCards(currentGame)) {
        if (card.isCreature()) {
            Assert.assertEquals(card.getName() + " should not have ORC type",
                    false, card.getSubtype(currentGame).contains(SubType.ORC));
            Assert.assertEquals(card.getName() + " should have CAT type",
                    true, card.getSubtype(currentGame).contains(SubType.CAT));
        }
    }
}


Habiendo visto que este error está en las pruebas, inmediatamente se puede devaluar el defecto encontrado, pensando: "Bueno, estas son pruebas". Si es así, entonces no estoy de acuerdo contigo. Después de todo, las pruebas juegan un papel bastante importante en el desarrollo (aunque no tan notable como la programación), y cuando aparece un defecto en una versión, inmediatamente comienzan a señalar con el dedo a las pruebas / probadores. Entonces, las pruebas defectuosas son insostenibles. Entonces, ¿por qué se necesitan tales pruebas? ¿Por qué desperdiciar recursos en ellos?



El método testArcaneAdaptationGiveType () prueba la tarjeta "Adaptación Arcana". Cada jugador recibe cartas en un área de juego específica. Y gracias a copiar y pegar, playerA obtuvo 2 cartas idénticas de "Silvercoat Lion" en el área de juego "Cemetery" , y playerBasí que no pasó nada. Luego algo de magia y probándose a sí mismo.



Cuando la prueba llega al "cementerio" del jugador B en el dibujo actual, entonces la ejecución de la prueba nunca entra en el ciclo, porque no había nada en el "cementerio". Esto lo descubrí con el viejo System.out.println () al comenzar la prueba .



Copiar y pegar corregido: 



....
addCard(Zone.HAND, playerA, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=

addCard(Zone.HAND, playerB, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerB, "Silvercoat Lion");   // <=
....


Después de ajustar el código, cuando ejecuté la prueba, la búsqueda de criaturas en el cementerio del jugador B comenzó a funcionar. Ave, System.out.println () !



La prueba es verde tanto antes como después de la corrección, lo cual es mucha suerte. Pero en caso de modificaciones que cambien la lógica de la ejecución del programa, dicha prueba no le hará ningún favor y le notificará de la finalización exitosa incluso si hay errores.



Mismo copiar y pegar en otros lugares:



  • V6072 Se encontraron dos fragmentos de código similares. Quizás, esto es un error tipográfico y la variable 'playerB' debería usarse en lugar de 'playerA'. PaintersServantTest.java (33), PaintersServantTest.java (29), PaintersServantTest.java (27), PaintersServantTest.java (31)
  • V6072 Se encontraron dos fragmentos de código similares. Quizás, esto es un error tipográfico y la variable 'playerB' debería usarse en lugar de 'playerA'. SubTypeChangingEffectsTest.java (32), SubTypeChangingEffectsTest.java (28), SubTypeChangingEffectsTest.java (26), SubTypeChangingEffectsTest.java (30)


Advertencia N11



V6086 Formateo de código sospechoso. Probablemente falta la palabra clave 'else'. DeckImporter.java (23) 



public static DeckImporter getDeckImporter(String file) {
  if (file == null) {
    return null;
  } if (file.toLowerCase(Locale.ENGLISH).endsWith("dec")) {   // <=
    return new DecDeckImporter();
  } else if (file.toLowerCase(Locale.ENGLISH).endsWith("mwdeck")) {
    return new MWSDeckImporter();
  } else if (file.toLowerCase(Locale.ENGLISH).endsWith("txt")) {
    return new TxtDeckImporter(haveSideboardSection(file));
  }
  ....
  else {
    return null;
  }
}


La regla de diagnóstico V6086 diagnostica el formato if-else-if incorrecto , lo que implica la omisión de else .



Este fragmento de código demuestra esto. En este caso, debido a la expresión de retorno nula , la inexactitud en el formato no conduce a nada, pero es genial encontrar tales casos, ya que no es necesario.



Consideremos un caso en el que omitir el else puede conducir a un comportamiento inesperado: 



public SomeType smtMethod(SomeType obj) {
  ....
  if (obj == null) {
    obj = getNewObject();
  } if (obj.isSomeObject()) {
    // some logic
  } else if (obj.isOtherSomething()) {
    obj = calulateNewObject(obj);
    // some logic
  } 
  ....
  else {
    // some logic
  }
  return obj;
}


Ahora, en el caso de obj == null , al objeto en cuestión se le asignará algún valor, y el else faltante hará que el objeto recién asignado comience a ser verificado a lo largo de la cadena if-else-if , mientras que se suponía que el objeto regresaría inmediatamente de método.



Conclusión



Verificar XMage es otro artículo que revela las capacidades de los analizadores estáticos modernos. En el desarrollo moderno, la necesidad de ellos solo crece a medida que aumenta la complejidad del software. Y no importa cuántas versiones, pruebas o comentarios de los usuarios tenga: un error siempre encontrará una laguna para ingresar a su base de código. Entonces, ¿por qué no agregar otra barrera a su defensa?



Como comprenderá, los analizadores son propensos a generar falsos positivos (incluido PVS-Studio Java). Esto puede ser el resultado tanto de un defecto obvio como de un código demasiado confuso (por desgracia, el analizador no se dio cuenta). Debe tratarlos con comprensión y darse de baja de inmediato sin dudarlo , pero mientras los falsos positivos esperan su corrección, puede usar uno de los métodossuprimiendo advertencias.



En conclusión, le sugiero que personalmente "toque" el analizador descargándolo de nuestro sitio web.





Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace de traducción: Maxim Stefanov. Comprobando el código de XMage y por qué no podrá obtener las cartas especiales raras de la colección Dragon's Maze .


More articles:


All Articles