miércoles
oct172012
Refactorización de un POJO
chocesPuestos a plantear cuestiones que sugieran un debate, a la par que ayuden a mejorar el dominio del lenguaje, y la calidad del código, propongo un asunto bastante habitual.
Supongamos que tenemos una clase como la que sigue:
public class Lista<T> {
private List<T> lista;
public List<T> getLista() {
return lista;
}
public void setLista(List<T> lista) {
this.lista = lista;
}
}
Con una refactorización del campo que es habitual en los IDE.
Se pretende refactorizar la clase, de manera que sea thread-safe, y que sea segura en todos los aspectos.
Reader Comments (12)
Quizá, ¿haciéndola inmutable?
public class Lista<T> {
private List<T> lista;
public List<T> getLista() {
return new List<T>(lista);
}
public void setLista(List<T> lista) {
this.lista = new List<T>(lista);
}
}
¡Sin quizás! ;)
Esta solución me parece mejor, porque elimina el setter, y permite declarar el campo como final:
public class Lista<T> {
private final List<T> lista;
public Lista(final List<T> lista) {
this.lista = lista == null ? new ArrayList<T>(0) : new ArrayList<T>(lista);
}
public List<T> getLista() {
return new ArrayList<T>(lista);
}
}
Ahora la cuestión es cómo operar con la lista, añadiendo, eliminando, verificando, siendo inmutable como es ;)
Nota: no se pueden crear instancias de List; supongo que querías poner ArrayList ;)
Me quedo loquer chavales.... Esnucau, lo tendre que pensar
Si se te quema alguna neurona pensando... ¡Reclama a James Gosling! ;D
Buenas, llevo tiempo siguiendo este portal, pero es la primera vez que aporto algo.
En realidad, poniendo la lista como final y recibiéndola en el constructor, lo que hace inmutable es la clase contenedora (Lista) no la lista en sí (List), a menos que el constructor reciba como argumento una lista inmutable. Si se quere que la lista no se pueda modificar, se debe asignar al atributo final, una copia inmutable:
public class Lista<T> {private final List<T> lista;
public Lista(final List<T> lista) {
this.lista = lista == null ? Collections.emptyList() : Collections.unmodifiableList(lista);
}
public List<T> getLista() {
return lista;
}
}
Además, así nos evitaríamos tener que crear una copia cada vez que se invoca el getter.
Si la lista debe poderse modificar y no puede ser inmutable, podemos guardar una versión sincronizada (manteniendo inmutable la clase contenedora):
public class Lista<T> {private final List<T> lista;
public Lista(final List<T> lista) {
this.lista = lista == null ? Collections.synchronizedList(new ArrayList<T>()) : Collections.synchronizedList(lista);
}
public List<T> getLista() {
return lista;
}
}
El objeto List que nos devuelve Collections.synchronizedList() es thread-safe (o se supone)
@adeteran
¡Esperemos que haya más aportaciones tuyas, porque para ser la primera es excelente!
Has puesto sobre la mesa dos conceptos importantes, que sin duda usaremos: vista inmutable y concurrencia.
Tal y como lo implementas, la clase no tendría la flexibilidad que se pretende: o es inmutable o concurrente.
Lo que se plantea es que el campo lista se pueda modificar, al tiempo que permanece encapsulada, y que sea thread-safe.
public class Lista<T> {
private final List<T> lista;
public Lista(final List<T> lista) {
this.lista = lista == null ? new ArrayList<T>(0) : new ArrayList<T>(lista);
}
public List<T> getLista() {
return new ArrayList<T>(lista);
}
public List<T> getListaInmutable() {
return Collections.unmodifiableList(lista);
}
}
Con esta nueva versión, gracias a tu aportación, podemos devolver una vista inmutable, además de otra mutable. En ambos casos, lista permanece encapsulada.
Para resolver la cuestión de la multitarea, antes debemos implementar la mutabilidad encapsulada del campo lista :)
"El objeto List que nos devuelve Collections.synchronizedList() es thread-safe".
Sí, pero... al igual que con todas las clases de este estilo, como CopyOnWriteArrayList o ConcurrentHashMap, los iteradores no son thread-safe
Cuando se hace una iteración, debe incluirse en un bloque sincronizado.
interesantes respuesta y muy buen nivel técnico de debate.
gracias por las aportaciones, abro una nueva pregunta...
Me quedo con final es bueno para inmutable... no había caído... copón
Voy a proponer una solución:
public class Lista<T> {
private final List<T> lista;
private final ReentrantLock lock;
public Lista(final List<T> lista) {
this.lista = lista == null ? new ArrayList<T>(0) : new ArrayList<T>(lista);
this.lock = new ReentrantLock();
}
public List<T> getLista() {
final ReentrantLock lock = this.lock;
lock.lock();
try {
return new ArrayList<T>(lista);
} finally {
lock.unlock();
}
}
public List<T> getListaInmutable() {
final ReentrantLock lock = this.lock;
lock.lock();
try {
return Collections.unmodifiableList(lista);
} finally {
lock.unlock();
}
}
public boolean add(final T elemento) {
final ReentrantLock lock = this.lock;
lock.lock();
try {
return lista.add(elemento);
} finally {
lock.unlock();
}
}
public boolean remove(final T elemento) {
final ReentrantLock lock = this.lock;
lock.lock();
try {
return lista.remove(elemento);
} finally {
lock.unlock();
}
}
public boolean contains(final T elemento) {
final ReentrantLock lock = this.lock;
lock.lock();
try {
return lista.contains(elemento);
} finally {
lock.unlock();
}
}
}
No había caído en lo del iterador. Volviendo con las utilidades que nos proporciona la clase Collections, y simplificando la solución de choces, voy a proponer lo siguiente:
public class Lista<T> {private final List<T> lista;
public Lista(final List<T> lista) {
this.lista = lista == null ? Collections.synchronizedList(new ArrayList<T>(0)) : Collections.synchronizedList(lista);
}
public List<T> getListaInmutable() {
return Collections.unmodifiableList(lista);
}
public boolean add(final T elemento) {
return lista.add(elemento);
}
public boolean remove(final T elemento) {
return lista.remove(elemento);
}
public boolean contains(final T elemento) {
return lista.contains(elemento);
}
}
Con la lista que nos da Collections.synchronizedList() no necesitamos sincronizar "a mano" los métodos add, remove y contains. Para iterar, usamos la lista inmutable.
Sólo veo un problema, y es que si un thread itera y otro modifica la lista, el que itera no ve los cambios. Pero eso ocurre con cualquier solución en el que se devuelva una copia, y no la lista interna de la clase.
Así el código queda más limpio, a menos que haya alguna ventaja con respecto a usar ReentrantLock en vez de la lista sincronizada que nos da Collections (que internamente usa el syncronized() de siempre).
Ya sospechaba que vendrías con una solución de este tipo ;D
Por supuesto que es perfectamente válida; y en este caso tan sencillo, con una única lista, vale tanto como usar synchronized en los métodos, o usar una CopyOnWriteArrayList en vez de una ArrayList.
La razón por la que he propuesto una solución con ReentrantLock, aparte de para que se conozca (o se refresque la memoria) cómo se usa, es: si hay más de un campo del estilo de lista, por ejemplo más List, o Map, y no se desean usar (por el motivo que sea) las envolturas de Collections, o las clases específicamente sincronizadas, si se usan sincronizadores "globales" (sobre los métodos o sobre la misma clase como monitor), cuando una tarea accede a uno de los métodos, bloquea todos los demás, incluidos los de las colecciones que no le interesan.
Usando monitores específicos, como ReentrantLock, se puede atomizar mejor el acceso en esos casos: cada colección de la clase usa su propio lock.
El problema con los iteradores no creo que tenga una solución fácil, por lo que bien dices.
O bien se hace una sincronización externa (lo que puede ser una complicación), o bien se traslada el código de la iteración a esta misma clase, de manera que sea fácilmente sincronizable (aquí sí que habría que usar una sincronización explícita con monitores internos), y devolver el resultado.