Utilité singleton

Le problème exposé dans ce sujet a été résolu.

Bonjour à tous,

Je suis en train de m'intéresser au code d'un bot Discord (un chat) et je suis tombé sur la classe suivante:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
public class MessageHandler {

    private static final LinkedBlockingQueue<MessageAction> queue = new LinkedBlockingQueue<>();
    private static MessageHandler ourInstance = new MessageHandler();

    public static MessageHandler getInstance() {
        return ourInstance;
    }
    private MessageHandler() {
    }

    public static LinkedBlockingQueue<MessageAction> getQueue() {
        return queue;
    }
    public void addCreateToQueue(String _id, MessageCreateAction.Type _type, Message _message, StreamEntity... _stream){
        [...]
            synchronized (queue){
                queue.add(messageItem);
                queue.notify();
            }
    }
}

Ensuite, dans d'autres classes, les méthodes de MessageHandler sont appelées de façon différentes. Lorsque getQueue() est appelée, le code ressemble à MessageHandler.getQueue(). Par contre, si on appelle la méthode addCreateToQueue(…), le code ressemble à MessageHandler.getInstance().addCreateToQueue(…).

Voici ma question : à quoi sert l'appel à getInstance() ? Selon moi, étant donné qu'on a un synchronize sur l'objet queue, on pourrait rendre la méthode addCreateToQueue(…) statique et l'appeler de la même façon que getQueue() et ainsi se débarrasser de l'instance interne.

Est-ce correct ? Ou passé-je à côté de quelque de crucial ?

Merci d'avance pour vos réponses !

Miguel

+0 -0

Je pense que queue est static car elle est partagée entre tous les threads.

Le code dans son entièreté peut être trouvé ici.

Et voici un c/c de la classe entière :

  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103
package ws.discord.messages;

import entity.StreamEntity;
import entity.local.MessageAction;
import entity.local.MessageCreateAction;
import entity.local.MessageDeleteAction;
import entity.local.MessageEditAction;
import net.dv8tion.jda.MessageBuilder;
import net.dv8tion.jda.entities.Message;

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.LinkedBlockingQueue;

public class MessageHandler {

    private static final LinkedBlockingQueue<MessageAction> queue = new LinkedBlockingQueue<>();
    private static final LinkedBlockingQueue<StreamEntity> deletequeue = new LinkedBlockingQueue<>();
    private static MessageHandler ourInstance = new MessageHandler();

    public static MessageHandler getInstance() {
        return ourInstance;
    }

    private MessageHandler() {
    }

    public static LinkedBlockingQueue<MessageAction> getQueue() {
        return queue;
    }

    public static LinkedBlockingQueue<StreamEntity> getDeleteQueue() {
        return deletequeue;
    }

    public void addCreateToQueue(Long _id, MessageCreateAction.Type _type, Message _message, StreamEntity... _stream){
        this.addCreateToQueue(Long.toString(_id), _type, _message, _stream);
    }

    public void addCreateToQueue(String _id, MessageCreateAction.Type _type, Message _message, StreamEntity... _stream){
        String message = _message.getRawContent();
        List<String> messages = new ArrayList<>();
        while(message.length()>2000)
        {
            int index = message.lastIndexOf("\n",1999);
            if(index==-1)
                index = message.lastIndexOf(" ",1999);
            if(index==-1)
                index=1999;
            messages.add(message.substring(0,index));
            message = message.substring(index);
        }
        messages.add(message);
        for(String cutMessage : messages){
            MessageBuilder builder = new MessageBuilder();
            MessageCreateAction messageItem;
            if(_stream.length > 0){
                messageItem = new MessageCreateAction(_id, _type, builder.appendString(cutMessage).build(), _stream[0]);
            }
            else {
                messageItem = new MessageCreateAction(_id, _type, builder.appendString(cutMessage).build(), null);
            }
            synchronized (queue){
                queue.add(messageItem);
                queue.notify();
            }
        }
    }

    public void addEditToQueue(Long id, Message message, String newMessage){
        this.addEditToQueue(Long.toString(id), message, newMessage);
    }

    public void addEditToQueue(String id, Message message, String newMessage){
        if(newMessage.length() < 2000){
            MessageEditAction messageEditAction = new MessageEditAction(id, message, newMessage);
            synchronized (queue){
                queue.add(messageEditAction);
                queue.notify();
            }
        }
    }

    public void addDeleteToQueue(Long id, Message message){
        this.addDeleteToQueue(Long.toString(id), message);
    }

    public void addDeleteToQueue(String id, Message message){
            MessageDeleteAction messageDeleteAction = new MessageDeleteAction(id, message);
            synchronized (queue){
                queue.add(messageDeleteAction);
                queue.notify();
            }
    }

    public void addStreamToDeleteQueue(StreamEntity streamEntity){
        synchronized (deletequeue){
            deletequeue.add(streamEntity);
            deletequeue.notify();
        }
    }

}
+0 -0
  • vu que tous les attributs sont statiques, difficile de dire a quoi peut servir une instance de la classe.

J'ai l'impression que c'est un mélange des genres entre singleton et classe statique. C'est peut-être simplement une erreur de design/code.

  • synchronized est utilisé n'importe comment sur des objets par essence déjà synchronisés…

Qu'est-ce qui te fait dire qu'ils sont déjà synchronisés ? static ne laisse pas présager de synchronisation. De plus, tu es obligé d’acquérir le mutex avant d'appeler .notify(), sous peine de IllegalMonitorStateException.

+0 -0

Qu'est-ce qui te fait dire qu'ils sont déjà synchronisés ? static ne laisse pas présager de synchronisation.

Clems

Non, mais la classe LinkedBlockingQueue est conçue pour être thread-safe, donc rajouter des synchronisations autour, c'est pas sensé être utile à moins qu'on soit vraiment en train de faire des conneries monumentales ailleurs.

Note : le singleton est souvent un anti-pattern, et là ça y ressemble bien.

+1 -0

Qu'est-ce qui te fait dire qu'ils sont déjà synchronisés ? static ne laisse pas présager de synchronisation.

Cf. le post de mon VDD.

De plus, tu es obligé d’acquérir le mutex avant d'appeler .notify(), sous peine de IllegalMonitorStateException.

Clems

Je veux bien, mais a quoi donc sert le notify ici ?

Bref, le code ne me semble pas très pro, même si je ne suis pas expert java.

+0 -0

Ah oui d'accord, je n'avais pas compris qu'on parlait de l'implémentation de la queue en elle même.

Les synchronized servent aussi à rendre le couple add()/notify() atomique; je ne suis pas convaincu que le notify() soit utile dans ce cas puisque c'est déjà censé être le rôle de la queue d'être bloquante. De plus, on préfère souvent utiliser notifyAll() au lieu de notify().

Je pense que tu devrais être méfiant avec ce code, et ne pas forcément chercher trop loin à comprendre ^^

+0 -0
Connectez-vous pour pouvoir poster un message.
Connexion

Pas encore membre ?

Créez un compte en une minute pour profiter pleinement de toutes les fonctionnalités de Zeste de Savoir. Ici, tout est gratuit et sans publicité.
Créer un compte