J'ai la carte suivante:
Map<Double, List<SoundEvent>> soundEventCells = new HashMap<Double, List<SoundEvent>>();
Cela HashMap
mappe les double
valeurs (qui sont des points dans le temps) à la SoundEvent
«cellule» correspondante : chaque «cellule» peut contenir un certain nombre de SoundEvent
s. 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?
java
design
object-oriented
Aviv Cohn
la source
la source
private static
parce 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 deprivate static
?Réponses:
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:
Vous ne voulez certainement pas avoir un tas de code disant ceci:
Ou peut-être pourriez-vous simplement utiliser l'une des implémentations de Guava Multimap .
la source
TimeLine
classe exactement pour ce genre de chose :) C'est une mince enveloppe autour d'unHashMap<Double, SoundEventCell>
(finalement j'ai choisi l' idéeSoundEventCell
plutôt que l'List<SoundEvent>
idée). Je peux donc simplement fairetimeline.addEvent(4.5, new SoundEvent(..))
et encapsuler les trucs de niveau inférieur :)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.
la source
List
on peut faire, et il fait toutes ces choses pour une bonne raison.SoundEventCell
pourrait implémenterIterable
pourSoundEvent
s, ce qui offrirait l'itérateur desoundEvents
membre, 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 utiliserList
quand j'ai besoin de quelque chose de plus dynamique à l'avenir.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:
Vous pouvez toujours écrire le code de votre exemple.
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.
la source
Une autre solution pourrait être de définir votre classe wrapper avec une seule méthode qui expose la liste:
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.unmodifiableList
dans 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 unMultimap<Double, SoundEvent>
( docs ), car cela économise souvent beaucoup de logique et d'erreurs de vérification nulle.)la source