¿Es excesivo envolver una colección en una clase simple solo en aras de una mejor legibilidad?

15

Tengo el siguiente mapa:

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

Esto HashMapasigna doublevalores (que son puntos en el tiempo) a la SoundEvent'celda' correspondiente : cada 'celda' puede contener un número de SoundEvents. Por eso se implementa como unList<SoundEvent> , porque eso es exactamente lo que es.

En aras de una mejor legibilidad del código, pensé en implementar una clase interna estática muy simple como esta:

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
}

Y que la declaración del mapa (y muchos otros códigos) se vería mejor, por ejemplo:

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

¿Es esto excesivo? ¿Harías esto en tus proyectos?

Aviv Cohn
fuente
Uno puede argumentar que conceptualmente, esto se ha abordado en ¿Cómo sabría si ha escrito código legible y fácil de mantener? Si sus compañeros siguen quejándose de su manera de hacer las cosas, ya sea de una forma u otra, es mejor que cambie para que se sientan mejor
mosquito
1
¿Qué es lo que hace que la lista de eventos de sonido sea una "celda" en lugar de una lista? ¿Esta elección de palabras significa que una celda tiene o eventualmente tendrá un comportamiento diferente al de una lista?
código x
@DocBrown ¿Por qué? La clase se private staticdebe a que solo será utilizada por la clase externa, pero no está relacionada con ninguna instancia específica de la clase externa. ¿No es exactamente el uso apropiado de private static?
Aviv Cohn
2
@ Doc Brown, Aviv Cohn: ¡No hay una etiqueta que especifique ningún idioma, por lo que cualquier cosa puede estar bien o mal al mismo tiempo!
Emilio Garavaglia
@EmilioGaravaglia: Java (creo que está bastante claro ya que a juzgar por la sintaxis podría ser Java o C #, y las convenciones utilizadas lo reducen a Java;)).
Aviv Cohn

Respuestas:

12

No es exagerado en absoluto. Comience con las operaciones que necesita, en lugar de comenzar con "Puedo usar un HashMap". A veces, un HashMap es justo lo que necesitas.
En tu caso sospecho que no lo es. Lo que probablemente quieras hacer es algo como esto:

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

Definitivamente no quieres tener un montón de código que diga esto:

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

O tal vez podría usar una de las implementaciones de Guava Multimap .

Kevin Cline
fuente
¿Así que aboga por usar una clase, que es esencialmente un mecanismo de ocultación de información, como un ... mecanismo de ocultación de información? El horror.
Robert Harvey
1
En realidad sí, tengo una TimeLineclase exactamente para ese tipo de cosas :) Es una envoltura delgada alrededor de un HashMap<Double, SoundEventCell>(eventualmente fui con la idea en SoundEventCelllugar de la List<SoundEvent>idea). Así que puedo hacer timeline.addEvent(4.5, new SoundEvent(..))y encapsular las cosas de nivel inferior :)
Aviv Cohn
14

Si bien puede ayudar a la legibilidad en algunas áreas, también puede complicar las cosas. Personalmente, me inclino por envolver o extender colecciones por fluidez, ya que el nuevo contenedor, en la lectura inicial, me implica que puede haber un comportamiento que debo tener en cuenta. Considérelo una sombra del Principio de la menor sorpresa.

Seguir con la implementación de la interfaz significa que solo necesito preocuparme por la interfaz. La implementación concreta puede, por supuesto, albergar un comportamiento adicional, pero no debería tener que preocuparme por eso. Entonces, cuando intento encontrar el camino a través del código de alguien, prefiero las interfaces simples para facilitar la lectura.

Si, por otro lado, está encontrando un caso de uso que se beneficia del comportamiento agregado, entonces tiene un argumento para mejorar el código al crear una clase completa.

Bucle de retroalimentación
fuente
11
Un contenedor también puede usarse para eliminar (u ocultar) comportamientos innecesarios.
Roman Reiner
44
@RomanReiner: advertiría contra tales cosas. Comportamiento innecesario hoy es a menudo el programador maldiciendo su nombre mañana. Todos saben lo que Listpuede hacer, y hace todas esas cosas por una buena razón.
Telastyn
Aprecio el deseo de mantener la funcionalidad, aunque creo que la solución es un cuidadoso equilibrio entre funcionalidad y abstracción. SoundEventCellpodría implementarse Iterablepara SoundEvents, que ofrecería el iterador de soundEventsmiembro, por lo que podría leer (pero no escribir) como cualquier lista. Dudo en enmascarar la complejidad casi tanto como dudo en usar un Listcuando necesito algo más dinámico en el futuro.
Neil
2

Ajustarlo limita su funcionalidad a solo aquellos métodos que decida decidir escribir, básicamente aumentando su código sin ningún beneficio. Por lo menos, intentaría lo siguiente:

private static class SoundEventCell : List<SoundEvent>
{
}

Todavía puede escribir el código de su ejemplo.

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

Dicho esto, solo he hecho esto cuando hay alguna funcionalidad que la lista misma necesita. Pero creo que tu método sería excesivo para esto. A menos que tenga una razón para querer limitar el acceso a la mayoría de los métodos de List.

Lawtonfogle
fuente
-1

Otra solución podría ser definir su clase de contenedor con un único método que exponga la lista:

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

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

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

Esto le da a su clase bien nombrada con un código mínimo, pero aún así le proporciona encapsulación, lo que le permite, por ejemplo, hacer que la clase sea inmutable (haciendo una copia defensiva en el constructor y usando Collections.unmodifiableListen el descriptor de acceso).

(Sin embargo, si estas listas solo se usan en esta clase, creo que sería mejor reemplazarlas Map<Double, List<SoundEvent>>por un Multimap<Double, SoundEvent>( docs ), ya que eso a menudo ahorra mucha lógica y errores de verificación nula).

MikeFHay
fuente