Plugin Code review

[Monsieur Yaourt]

Context
Salut pour mettre un peu de context, je code depuis un bon moment maintenant (c, c++, et un peu kotlin/java) et la je veux m'exercer a 'bien' coder, un code le plus opti, lisible, facile a utiliser et maintenir. Du coup ce que je vais demander la c'est peut-etre un peu du chipotage a certains moments.

Probleme
Dans ma configuration.yml j'ai des messages

messages:
    mon_message: "&esalut %tag%"

je veux pouvoir acceder et envoyer des messages, depuis n'importe ou dans le code, sachant qu'ils peuvent avoir des tags (example %player% qui sera remplacer par le joueur (predefini)). Le message peut-etre destine a la console.
Le message de la configuration peut-etre:

• Un simple string (voir plus haut)
• Une list :

mon_message:
        - "&esalut %tag%"
        - "&eligne 2"

• Rien si l'user a supprimer la ligne..
• Rien si l'user a indiquer un message vide

mon_message: ""

Solution
Pour réaliser ça, j'ai opté pour l'Enum suiavnte

enum class Message(val path: String) {
   TARGET_MSG("path.to_the_message"),
   ...
}

L'Enum contient la fonction static principal suivante (ignorer Component pour le moment)

fun sendMessage(player: Player?, msg: Message, component: ComponentObj?) {
    var messageList: MutableList<String>? = fileManager.getStringListFromFile(FileType.MESSAGE, msg.path) //Cette fonction utilisera getStringList de org.bukkit.configuration.file
    if (messageList.isNullOrEmpty()) //getStringList est null ou vide ?
        messageList = mutableListOf(fileManager.getStringFromFile(FileType.MESSAGE, msg.path) ?: return) //Cette fonction utilisera getString de org.bukkit.configuration.file
    if (messageList.isEmpty()) //Le message est quand meme vide ?
         return
    if (component != null)
        buildMessageComponent(messageList, component)
    messageSender(player, messageList)
}

Et ici, j'ai déjà un problème, au total, j'effectue 3 if (sans Component et en comptant ?: comme un if) à chaque fois que je veux envoyer un message, imaginez que le message est envoyé de maniere global à tout le serveur et que comme par hasard il y a pile 999joueurs sur le (unique) serveur, ça en fait des vérifications pour un pauvre message.. Je suis obligé de vérifier le retour de getStringList, lequel ne récupère pas le contenu si c'est un string simple, donc pour m'assurer que c'est pas un String simple, je dois vérifier et effectuer getString, mais je dois quand même vérifier que le message suite à getString n'est pas vide (si l'utilisateur à supprimer la ligne ou n'a rien mis). Bref, je n'aime pas ce code. La seule alternative que j'ai trouve c'est peut-être de faire une map au start du serveur avec tous les messages enregistrés 1 seule fois au démarrage (avis?).

Ensuite, pour les Component, lesquels gèrent donc les tags j'ai l'enum ainsi que l'objet suivant (dans le meme fichier Message.kt)

enum class ComponentEnum(val tag: String) {
   PLAYER("%player%"),
   ...
}
class ComponentObj(val compoEnum: List<ComponentEnum>, val values: List<String>)

Donc au final, ma fonction sendMessage, en admettant qu'il y a un ou plusieurs component et appele comme ceci:

Message.sendMessage(playerVar, Message.MY_MESSAGE, ComponentObj(listOf(ComponentEnum.PLAYER), listOf(playerVar.name)))

Puis message a un builder (buildMessageComponent) qui effectuera

private fun buildMessageComponent(messageList: MutableList<String>, component: ComponentObj) {
        messageList.indices.forEach { i ->
            if (messageList[i].contains('%')) { //Si la ligne ne contient meme pas un % ce n'est pas la peine de continuer
                component.values.indices.forEach { x ->
                    messageList[i] = messageList[i].replace(component.compoEnum[x].tag, component.values[x])
                }
            }
        }
}

L'utilisation est assez simple, mais je suis quand même sceptique, je sais pas trop pourquoi enfaite, si quelqu'un a une meilleure manière de gérer les choses je suis preneur.

 

[ShE3py]

Bonsoir,

un code le plus opti, lisible, facile a utiliser et maintenir. Du coup ce que je vais demander la c'est peut-etre un peu du chipotage a certains moments.

Ton but premier est d'écrire du code élégant ; c'est à ton compilateur de l'optimiser ensuite. Ne lui vole donc pas son travail !

sachant qu'ils peuvent avoir des tags (example %player% qui sera remplacer par le joueur (predefini))

Le terme approprié serait “placeholder”

Pour réaliser ça, j'ai opté pour l'Enum suiavnte

enum class Message(val path: String) {
    TARGET_MSG("path.to_the_message"),
    ...
}

Idéalement il faudrait générer cette classe à partir du .yml.

L'Enum contient la fonction static principal suivante (ignorer Component pour le moment)

fun sendMessage(player: Player?, msg: Message, component: ComponentObj?) {
    var messageList: MutableList<String>? = fileManager.getStringListFromFile(FileType.MESSAGE, msg.path) //Cette fonction utilisera getStringList de org.bukkit.configuration.file
    if (messageList.isNullOrEmpty()) //getStringList est null ou vide ?
        messageList = mutableListOf(fileManager.getStringFromFile(FileType.MESSAGE, msg.path) ?: return) //Cette fonction utilisera getString de org.bukkit.configuration.file
    if (messageList.isEmpty()) //Le message est quand meme vide ?
         return
    if (component != null)
        buildMessageComponent(messageList, component)
    messageSender(player, messageList)
}

Et ici, j'ai déjà un problème, au total, j'effectue 3 if (sans Component et en comptant ?: comme un if) à chaque fois que je veux envoyer un message, imaginez que le message est envoyé de maniere global à tout le serveur et que comme par hasard il y a pile 999joueurs sur le (unique) serveur, ça en fait des vérifications pour un pauvre message..

Ce n'est pas grave ; un ordinateur peut faire plusieurs milliards de comparaisons par seconde.

La seule alternative que j'ai trouve c'est peut-être de faire une map au start du serveur avec tous les messages enregistrés 1 seule fois au démarrage (avis?).

C'est effectivement ce qu'il faudrait faire ; car là tu crées une liste à chaque fois que tu envoies un message. Vu que la clef est une énumération il faudrait utiliser une EnumMap.

Message.sendMessage(playerVar, Message.MY_MESSAGE, ComponentObj(listOf(ComponentEnum.PLAYER), listOf(playerVar.name)))

Un builder pattern serait plus lisible qu'une ribambelle d'arguments ;

Message.BABAR.create(player).arg("player", player).send();

Et l'idée serait de surcharger arg() pour tous les types que tu veux (String, Player, nombres, etc.).

Cordialement,
ShE3py

 

[Monsieur Yaourt]

Merci du retour !

Idéalement il faudrait générer cette classe à partir du .yml.

Je suis pas sûr d'avoir bien compris ce que tu voulais dire par là ?

Ce n'est pas grave ; un ordinateur peut faire plusieurs milliards de comparaisons par seconde.

Oui, bien sûr, ce n'est pas grave, mais ça rend mon code bien moins élégant je trouve

Un builder pattern serait plus lisible qu'une ribambelle d'arguments ;

J'ai une idée de comment faire mais je suis pas sûr de la manière d'implémenter un tel pattern.

Message.BABAR.create(player).arg("player", player).send();

create serait le première (obligatoire) appel, puis les méthodes, pour finir par le send. Je devrais faire des variables dans mon enum ou alors créer un data class dans mon Enum peut-etre ? Et merci, je savais pas qu'on pouvait faire Message.VALUE directement, ça semble si évident je me sens idiot !!

C'est effectivement ce qu'il faudrait faire ; car là tu crées une liste à chaque fois que tu envoies un message. Vu que la clef est une énumération il faudrait utiliser une EnumMap.

Cette EnumMap elle devrait etre defini ou ? J'ai une class FileManager, peut-etre labas avec un getValue, ou directement dans l'Enum avec une methode setup ?

 

[ShE3py]

Je suis pas sûr d'avoir bien compris ce que tu voulais dire par là ?

Faire un programme Java qui lit toutes les clefs du fichier de traduction, et qui te génère Message.java automatiquement.

---

// TODO: Générer ce fichier automatiquement

/**
 * Représente un message traduisible.
 */
public enum Message {
    BABAR("babar");
    // ^ Générer les valeurs ci-dessus automatiquement,
    //   et juste faire un gros `writer.write("public enum Message {")`
    //   avant/après pour le code constant
   
    public final @NonNls String key;
   
    Message(String key) {
       this.key = key;
    }
   
    public MessageSender create(CommandSender receiver) {
       return new MessageSender.Unicast(receiver, this);
    }
   
    public MessageSender create(Iterable<CommandSender> receivers) {
       return new MessageSender.Multicast(receivers, this);
    }
}

/**
 * Représente des {@linkplain Message} chargés en mémoire (aka une langue).
 */
public final class MessageBundle {
    private final Map<Message, List<String>> messages;
   
    private MessageBundle() {
       this.messages = new EnumMap<>(Message.class);
       
       // FIXME: lire le fichier de configuration
       this.messages.put(Message.BABAR, Arrays.asList("Hewwo", "%user%", "x%user%", "%user%x", "%user%%user%", "%user%x%user%"));
    }
   
    public static MessageBundle forReceiver(CommandSender receiver) {
       // FIXME: probablement renvoyer un singleton `MessageBundler.FRENCH`,
       //        cette fonction permettra de faire du multilangue plus tard si voulu
       throw new RuntimeException("not yet implemented");
    }
   
    public MessageBuilder create(Message key) {
       return new MessageBuilder(this.messages.get(key), Locale.FRENCH);
    }
   
    // pour tester
    public static void main(String[] args) {
       MessageBundle bundle = new MessageBundle();
       System.out.println(bundle.create(Message.BABAR).arg("user", "anon"));
       System.out.println();
       System.out.println(bundle.create(Message.BABAR).arg("user", "⋄"));
    }
}

/**
 * Permet de formatter un {@linkplain Message} d'un {@linkplain MessageBundle}.
 */
public final class MessageBuilder {
    private final List<@Nls String> formats;
    private final Locale locale;
    private final Map<String, String> args;
   
    public MessageBuilder(List<@Nls String> formats, Locale locale) {
       this.formats = formats;
       this.locale = locale;
       this.args = new IdentityHashMap<>();
    }
   
    public MessageBuilder arg(@NonNls String key, @Nls String value) {
       this.args.put(key, value);
       return this;
    }
   
    public MessageBuilder arg(@NonNls String key, float value) {
       this.arg(key, String.format(this.locale, "%.1f", value));
       return this;
    }
   
    public MessageBuilder send(Iterable<CommandSender> receivers) {
       String[] that = this.toArray();
       for(CommandSender r : receivers) {
          r.sendMessage(that);
       }
       return this;
    }
   
    public MessageBuilder send(CommandSender receiver) {
       return this.send(Collections.singleton(receiver));
    }
   
    @Override
    public String toString() {
       return this.formats.stream().map(this::build).collect(Collectors.joining("\n"));
    }
   
    public String[] toArray() {
       return this.formats.stream().map(this::build).toArray(String[]::new);
    }
   
    /**
     * Remplace tous les {@code %placerholder%} de format par ceux dans {@code this.args}.
     */
    private @Nls String build(@Nls String format) {
       // un memcpy serait plus efficace, mais Java ne nous permet pas de travailler par byte[]
       // (ou du moins sans qu'il refasse des conversions/vérification d'encodage derrière),
       // donc c'est plus rapide d'utiliser `.replace()`
       for(Map.Entry<String, String> entry : this.args.entrySet()) {
          format = format.replace('%' + entry.getKey() + '%', entry.getValue());
       }
       
       return format;
    }
}

/**
 * Permet de formatter un {@linkplain Message} pour un groupe de {@linkplain CommandSender}.
 */
public interface MessageSender {
    MessageSender arg(@NonNls String key, @Nls String value);
    MessageSender arg(@NonNls String key, float value);
    void send();
   
    class Unicast implements MessageSender {
       private final MessageBuilder builder;
       private final CommandSender receiver;
       
       public Unicast(CommandSender receiver, Message key) {
          this.builder = MessageBundle.forReceiver(receiver).create(key);
          this.receiver = receiver;
       }
       
       @Override
       public MessageSender arg(String key, @Nls String value) {
          builder.arg(key, value);
          return this;
       }
       
       @Override
       public MessageSender arg(String key, float value) {
          builder.arg(key, value);
          return this;
       }
       
       @Override
       public void send() {
          this.builder.send(receiver);
       }
    }
   
    class Multicast implements MessageSender {
       private final Map<MessageBuilder, List<CommandSender>> localized;
       
       public Multicast(Iterable<CommandSender> receivers, Message key) {
          Map<MessageBundle, List<CommandSender>> perBundle = new IdentityHashMap<>();
          for(CommandSender receiver : receivers) {
             perBundle.computeIfAbsent(MessageBundle.forReceiver(receiver), _bundle -> new ArrayList<>()).add(receiver);
          }
         
          this.localized = new HashMap<>(perBundle.size());
          for(Map.Entry<MessageBundle, List<CommandSender>> entry : perBundle.entrySet()) {
             this.localized.put(entry.getKey().create(key), entry.getValue());
          }
       }
       
       @Override
       public MessageSender arg(String key, @Nls String value) {
          this.localized.keySet().forEach(inner -> inner.arg(key, value));
          return this;
       }
       
       @Override
       public MessageSender arg(String key, float value) {
          this.localized.keySet().forEach(inner -> inner.arg(key, value));
          return this;
       }
       
       @Override
       public void send() {
          this.localized.forEach(MessageBuilder::send);
       }
    }
}

Ce n'est pas forcément la meilleure solution, et de toute façon c'est typiquement le code que tu réusines plusieurs fois car c'est difficile de prévoir ce que tu veux faire dans tes messages.

Ou en plus simple, si tu es prêt à perdre tes placeholders nommés :

public enum Message {
    /**
     * {@code %1$s a remporté %2$.1f € !}
     */
    BABAR("babar");
    // ^ idéalement le code généré inclut la valeur par défaut
   
    public final @NonNls String key;
   
    Message(String key) {
       this.key = key;
    }
   
    public @Nls String format(Object... args) {
       // FIXME: utiliser la traduction du fichier
       String fmt = "%1$s a remporté %2$.1f € ! Est-ce %3$s ?";
       
       // pour modifier certaines classes
       for(int i = 0; i < args.length; ++i) {
          if(args[i] instanceof Boolean b) {
             args[i] = b ? "vrai" : "faux";
          }
          else if(args[i] instanceof CommandSender cs) {
             args[i] = cs.getName();
          }
       }
       
       return fmt.formatted(args);
    }
   
    public static void main(String[] args) {
       // Bernard a remporté 17.2 € ! Est-ce vrai ?
       System.out.println(Message.BABAR.format("Bernard", 17.1824, true));
    }
}

 

[Monsieur Yaourt]

Salut merci pour les reponses c'est tres instructif.

Faire un programme Java qui lit toutes les clefs du fichier de traduction, et qui te génère Message.java automatiquement.

Ça reste un plugin Minecraft, une fois qu'il est compile, il n'y a rien à faire (normalement ?).En soit l'Enum est fix, par exemple de base il y aurait NO_PERMISSION, mais dans le fichier message.yml l'admin peut juste supprimer la ligne (ou rien mettre) s'il veut que ce message ne soit jamais envoyé alors que moi dans le code je fais un Message.NO_PERM.send.... (qui n'enverrait donc rien dans ce cas de figure)

Sinon j'ai juste pas tout à fait compris pourquoi tu avais fait un MessageBuilder et aussi un MessageBundle ?

Voici le code actuel avec différentes modifs en rapport avec ce dont nous avons parlé. (Théorique j'ai pas encore test)(oui j'ai beaucoup aimé le concept de build pattern que tu m'as montré)

class MessageBundle {
    companion object {
        private val messageList: HashMap<String, List<String>> = hashMapOf() //key = path, list = list string from file

        init {
            //open the file, fill the messageList with the content of message.yml, then close the file

        }
    }
    private var message: List<String> = listOf();

    fun sendMessage(player: Player) {
        if (message.isEmpty())
            return
        for (line in message)
            player.sendMessage(line.replace('&', '§'))
    }

    fun withPlaceHolder(placeHolder: String, value: String): MessageBundle {
        message.indices.forEach { i ->
            if (message[i].contains('%'))
                message[i].replace(placeHolder, value)
        }
        return this
    }

    // Conventionnel d'après ce que j'ai pu lire au sujet de builder pattern
    fun createBuilder(messagePath: String): MessageBundle {
        message = messageList.getOrDefault(messagePath, emptyList())
        return this
    }
}

avec cet appel:
MessageBundle().createBuilder("path.msg_example").withPlacerHolder("%time%", "60").sendMessage(player)
ou sans holder
MessageBundle().createBuilder("path.msg_example").sendMessage(player)

Dans le cas de plusieurs PlaceHolder je devrais alors faire plusieurs withPlacerHolder ou simplement une fonction qui prend 2 list plutôt que deux String; à moins qu'il existe une meilleure solution. Et je suis pas super convaincu du companion object, ça fait pas super propre et si je veux implémenter un /reload se sera problématique puisque je n'ai pas de fonction par exemple, initMessageList mais un init directement.

J'ai supprimé les Enum, puisque ça m'apporte plus de flexibilite, d'autant que je pourrais faire un repo maven pour utiliser ce MessageBundle sur plusieurs plugins directement (qui auront donc des message.yml tres differents), mais je ne sais pas si c'est une bonne practice du coup.

 

[ShE3py]

Ça reste un plugin Minecraft, une fois qu'il est compile, il n'y a rien à faire (normalement ?).En soit l'Enum est fix, par exemple de base il y aurait NO_PERMISSION, mais dans le fichier message.yml l'admin peut juste supprimer la ligne (ou rien mettre) s'il veut que ce message ne soit jamais envoyé alors que moi dans le code je fais un Message.NO_PERM.send.... (qui n'enverrait donc rien dans ce cas de figure)

Générer src/Message.java à partir de ressources/config.yml, de telle sorte que la classe contenant toutes les clefs soit synchronisée avec la traduction par défaut. Je peux te faire un plugin exemple avec Maven si tu utilises ce dernier.

Sinon j'ai juste pas tout à fait compris pourquoi tu avais fait un MessageBuilder et aussi un MessageBundle ?

Je suppose que tu as plusieurs langues chargées en même temps ; MessageBundle représente une langue particulière, c.-à-d. les textes avec placeholders. MessagBuilder permet de transformer un texte avec placeholders en texte sans placeholder. Si tu n'as qu'une seule traduction, tu peux enlever cette classe.

player.sendMessage(line.replace('&', '§'))

Tu risques de remplacer incorrectement des Bro & Cie ; utilise ChatColor#translateAlternateColorCodes(char, String).

if (message.contains('%')) message.replace(placeHolder, value)

Tu peux enlever le if (il est déjà inclut dans le .replace()).

fun createBuilder(messagePath: String): MessageBundle { message = messageList.getOrDefault(messagePath, emptyList()) return this }

C'est plutôt un withPath() vu que tu return this;.

MessageBundle().createBuilder("path.msg_example").withPlacerHolder("%time%", "60").sendMessage(player)

Idéalement le paramètre doit être stringifié par la fonction (1 000 s'écrit 1,000 en anglais).

Dans le cas de plusieurs PlaceHolder je devrais alors faire plusieurs withPlacerHolder ou simplement une fonction qui prend 2 list plutôt que deux String; à moins qu'il existe une meilleure solution.

Généralement on chaîne les appels, mais tu peux aussi prendre un Object... en paramètres pour pouvoir écrire style .withPlaceholders("%a%", 17, "%b%", -4).

Et je suis pas super convaincu du companion object, ça fait pas super propre et si je veux implémenter un /reload se sera problématique puisque je n'ai pas de fonction par exemple, initMessageList mais un init directement.

Il faudrait plutôt mettre ton messageList en var et transformer ton bloc init { ... } par une fonction.

J'ai supprimé les Enum, puisque ça m'apporte plus de flexibilite, d'autant que je pourrais faire un repo maven pour utiliser ce MessageBundle sur plusieurs plugins directement (qui auront donc des message.yml tres differents), mais je ne sais pas si c'est une bonne practice du coup.

Honnêtement c'est plus courant de passer la clef sous forme d'une chaîne de caractères plutôt qu'un enum.

class MessageBuilder {
    companion object {
        private var messageList: Map<String, List<String>> = hashMapOf();
        
        fun init() {
            // FIXME: mettre à jour `messageList`
        }
        
        fun of(path: String): MessageBuilder {
            // FIXME: `messageList` doit toujours avoir une valeur !
            return MessageBuilder(messageList.getOrDefault(messagePath, emptyList()));
        }
    }
    
    // FIXME: aucune idée de comment faire un constructeur en Kotlin
    private val messages: List<String>;

    fun sendMessage(player: Player) {
        if (messages.isEmpty())
            return
        for (line in messages)
            // FIXME: ChatColor#translateAlternateColorCodes
            player.sendMessage(line.replace('&', '§'))
    }

    fun withPlaceHolder(placeHolder: String, value: String): MessageBuilder {
        messages.indices.forEach { i ->
            messages[i].replace(placeHolder, value)
        }
        return this
    }
    
    // FIXME: surcharger `toString()` ?
}

// Tu peux compresser un peu `withPlaceHolder` si t'as pas vraiment d'autre `withXXX`
MessageBuilder.of("hello").arg("%hi", "ho").toString();