Est-ce exagéré d'envelopper une collection dans une classe simple uniquement pour une meilleure lisibilité?

15

J'ai la carte suivante:

Map<Double, List<SoundEvent>> soundEventCells = new HashMap<Double, List<SoundEvent>>();

Cela HashMapmappe les doublevaleurs (qui sont des points dans le temps) à la SoundEvent«cellule» correspondante : chaque «cellule» peut contenir un certain nombre de SoundEvents. C'est pourquoi il est implémenté en tant que List<SoundEvent>, car c'est exactement ce que c'est.

Pour une meilleure lisibilité du code, j'ai pensé à implémenter une classe interne statique très simple comme ceci:

private static class SoundEventCell {
    private List<SoundEvent> soundEvents = new ArrayList<SoundEvent>();
    public void addEvent(SoundEvent event){
        soundEvents.add(event);
    }
    public int getSize(){
        return soundEvents.size();
    }
    public SoundEvent getEvent(int index){
        return soundEvents.get(index);
    }
    // .. remove() method unneeded
}

Et que la déclaration de carte (et beaucoup d'autres codes) serait meilleure, par exemple:

Map<Double, SoundEventCell> soundEventCells = new HashMap<Double, SoundEventCell>();

Est-ce exagéré? Le feriez-vous dans vos projets?

Aviv Cohn
la source
on peut faire valoir que, conceptuellement, cela a été traité dans Comment sauriez-vous si vous avez écrit du code lisible et facilement maintenable? Si vos pairs continuent de se plaindre de votre façon de faire les choses, que ce soit d'une manière ou d'une autre, vous feriez mieux de changer pour qu'ils se sentent mieux
moucher
1
Qu'est-ce qui fait de la liste des événements sonores une "cellule" plutôt qu'une liste? Ce choix de mots signifie-t-il qu'une cellule a ou aura éventuellement un comportement différent d'une liste?
x-code
@DocBrown Pourquoi? La classe est private staticparce qu'elle ne sera utilisée que par la classe externe, mais elle n'est liée à aucune instance spécifique de la classe externe. N'est-ce pas exactement la bonne utilisation de private static?
Aviv Cohn
2
@Doc Brown, Aviv Cohn: Il n'y a pas de balise spécifiant une langue, donc tout peut être bien ou mal en même temps!
Emilio Garavaglia
@EmilioGaravaglia: Java (je pense que c'est assez clair car à en juger par la syntaxe, cela pourrait être Java ou C #, et les conventions utilisées le limitent à Java;)).
Aviv Cohn

Réponses:

12

Ce n'est pas du tout exagéré. Commencez par les opérations dont vous avez besoin, plutôt que de commencer par «Je peux utiliser un HashMap». Parfois, un HashMap est exactement ce dont vous avez besoin.
Dans votre cas, je pense que non. Ce que vous voulez probablement faire, c'est quelque chose comme ceci:

public class EventsByTime {
    public EventsByTime addEvent(double time, SoundEvent e);
    public List<SoundEvent> getEvents(double time);
    // ... more methods specific to your use ...
}

Vous ne voulez certainement pas avoir un tas de code disant ceci:

List<SoundEvent> events = eventMap.get(time);
if (events == null) {
   events = new ArrayList<SoundEvent>();
   eventMap.put(time, events);
}

Ou peut-être pourriez-vous simplement utiliser l'une des implémentations de Guava Multimap .

Kevin Cline
la source
Vous préconisez donc d'utiliser une classe, qui est essentiellement un mécanisme de dissimulation d'informations, comme ... mécanisme de dissimulation d'informations? L'horreur.
Robert Harvey
1
En fait, oui, j'ai une TimeLineclasse exactement pour ce genre de chose :) C'est une mince enveloppe autour d'un HashMap<Double, SoundEventCell>(finalement j'ai choisi l' idée SoundEventCellplutôt que l' List<SoundEvent>idée). Je peux donc simplement faire timeline.addEvent(4.5, new SoundEvent(..))et encapsuler les trucs de niveau inférieur :)
Aviv Cohn
14

Bien qu'il puisse aider à la lisibilité dans certaines régions, il peut également compliquer les choses. Personnellement, je m'éloigne de l'emballage ou de l'extension des collections par souci de fluidité, car le nouveau wrapper, à la lecture initiale, implique pour moi qu'il peut y avoir un comportement dont je dois être conscient. Considérez cela comme une nuance du principe de la moindre surprise.

S'en tenir à la mise en œuvre de l'interface signifie que je n'ai qu'à me soucier de l'interface. L'implémentation concrète peut, bien sûr, abriter un comportement supplémentaire, mais je ne devrais pas avoir à m'en soucier. Donc, lorsque j'essaie de trouver mon chemin dans le code de quelqu'un, je préfère les interfaces simples pour la lisibilité.

Si, d'autre part, vous trouvez un cas d'utilisation qui fait bénéficier le comportement ajouté, alors vous avez un argument pour améliorer le code en créant une classe à part entière.

Boucle de rétroaction
la source
11
Un wrapper peut également être utilisé pour supprimer (ou masquer) les comportements inutiles.
Roman Reiner
4
@RomanReiner - Je mettrais en garde contre de telles choses. Le comportement inutile aujourd'hui est souvent le programmeur maudissant votre nom demain. Tout le monde sait ce que l' Liston peut faire, et il fait toutes ces choses pour une bonne raison.
Telastyn
J'apprécie le désir de maintenir la fonctionnalité, bien que je pense que la solution est un équilibre prudent entre fonctionnalité et abstraction. SoundEventCellpourrait implémenter Iterablepour SoundEvents, ce qui offrirait l'itérateur de soundEventsmembre, de sorte que vous seriez en mesure de lire (mais pas d'écrire) comme n'importe quelle liste. J'hésite à masquer la complexité presque autant que j'hésite à en utiliser Listquand j'ai besoin de quelque chose de plus dynamique à l'avenir.
Neil
2

L'encapsuler limite votre fonctionnalité aux seules méthodes que vous décidez d'écrire, augmentant fondamentalement votre code sans aucun avantage. À tout le moins, j'essaierais ce qui suit:

private static class SoundEventCell : List<SoundEvent>
{
}

Vous pouvez toujours écrire le code de votre exemple.

Map<Double, SoundEventCell> soundEventCells = new HashMap<Double, SoundEventCell>();

Cela dit, je ne l'ai fait que lorsqu'il y a des fonctionnalités dont la liste elle-même a besoin. Mais je pense que votre méthode serait exagérée. Sauf si vous aviez une raison de vouloir limiter l'accès à la plupart des méthodes de List.

Lawtonfogle
la source
-1

Une autre solution pourrait être de définir votre classe wrapper avec une seule méthode qui expose la liste:

private static class SoundEventCell
{
    private List<SoundEvent> events;

    public SoundEventCell(List<SoundEvent> events)
    {
        this.events = events;
    }

    public List<SoundEvent> getEvents()
    {
        return events;
    }
}

Cela vous donne votre classe bien nommée avec un code minimal, mais vous donne toujours l'encapsulation, vous permettant par exemple de rendre la classe immuable (en faisant une copie défensive dans le constructeur et en utilisant Collections.unmodifiableListdans l'accesseur).

(Cependant, si ces listes ne sont en effet utilisées que dans cette classe, je pense que vous feriez mieux de remplacer votre Map<Double, List<SoundEvent>>par un Multimap<Double, SoundEvent>( docs ), car cela économise souvent beaucoup de logique et d'erreurs de vérification nulle.)

MikeFHay
la source