This project is read-only.

¿Refactorizamos Register<T>()?

Jul 13, 2010 at 7:58 AM

Esta discusión comenzó en el grupo: http://groups.google.es/group/altnet-hispano/browse_thread/thread/c8263582791b0b47/0cbdbadcbab11942#0cbdbadcbab11942

Traida aqui siguiendo sugerencia de Leonardo Micheloni.

 

La propuesta consiste en si sería conveniente alterar la signatura:

 

void Register<T>(Func<T, string> assert); 

 

para desacoplar la validación propiamente dicha de la generación del mensaje de error del siguiente modo:

 

Esta variante para cuando la validación es especifica a una propiedad: 

   void Register<T>(string property, string PropertyFriendlyName, Func<T, bool> assert, string errorMessage); 

 

Y esta otra para cuando la validación no depende de una sola propiedad, sino del estado agregado del objeto (más de una propiedad). 

   void Register(Func<T, bool> assert, string errorMessage); 

 

 

En esta misma línea y si se acepta este modo de registro, también pudiera ser conveniente introducir otra signatura para el tratamiento de errores: 

 

public interface IValidator 
{ 
    IEnumerable<IValidationError> GetValidationErrors(object entity); 
... 


Donde tendriamos: 

 

public interface IValidationError 
{ 
    public string PropertyName {get;} 
    public PropertyInfo Property {get;}  //? 
    public string PropertyFriendlyName {get;} 
    public string ErrorMessage {get;} 
} 

De este modo podriamos tener más información sobre que fue mal en la validación. Y tal vez de paso simplifica la adición de algunas de las propuestas de extensión que hay sobre la mesa. 

¿Que opinan? ¿Pros y contras? 

Jul 13, 2010 at 2:11 PM
Poner un Func<T, bool> implica que hay que ir a buscar el mensaje de otro lado y allí empieza la rosca de generar clases de validacción que tengan un metodo de Match y un mensaje... y vamos con la rosca. Por ahora el engine se banca cualquier funcionamiento se le quiera dar externamente. Vendría a ser "Yo ejecuto ese Func, si tenes algo que no te gusta mandame el mensaje, si no no me mandes nada." Un solo roundtrip y cero "if" Con un Predidicate sería: "Che te doy este T decime si te gustó: ah, no te gustó ? bueno ahora decime por que en un mensaje." Dos roundtrip y un "if".
Jul 13, 2010 at 2:19 PM

Me parece muy razonable lo que comenta Pedro, dicho de otra manera, lo primero que se esperaría de un "validador" es "que valide" (que diga si es válido o no el objeto bajo análisis), los mensajes de "por qué no es válido" son más bien un valor agregado, útil o necesario pero secundario.

Me parece que está interpretación ...
if (!invalidMessage.Any())
{
	// personRepository.Save(person);
}

No debería dejarse bajo responsabilidad del usuario del FWK, sino que corresponde al mismo FWK.
En cuanto a la siguiente expresión ...
> Con un Predidicate sería:
> "Che te doy este T decime si te gustó: ah, no te gustó ? bueno ahora decime por que en un mensaje."
> Dos roundtrip y un "if".
Entiendo que en "decime si te gustó" de todas formas se evaluarían todas las reglas de validación (se podrían configurar otros comportamientos) y con sus resultados se iría llenando una colección que luego del if estaría disponible para que el usuario del FWK la lea si la necesita. No le veo mucho problema a esto, quedaría más OOP y se mantiene a disposición la lista de mensajes.
Así, el método Save de PersonCrudAppService.cs quedaría algo como:
public IEnumerable<string> Save(Person person)
{
	if (validator.IsValid(person))
	{
		// personRepository.Save(person);
	}
	return validator.InvalidMessages;
}

En este caso no se uso un if adicional, el que aparece ahí ya lo tenía la implementación original; incluso se restó una línea de código.
Por otro lado, alguien podría darle un mal uso a la API actual así:
public IEnumerable<string> Save(Person person)
{
	if (validator.IsValid(person))
	{
		// personRepository.Save(person);
	}
	return validator.GetInvalidMessages(person);
}

En este caso, es claro que la evaluación de las reglas/acciones de validación se haría dos veces. Claro, esto se podría solucionar sin cambiar el estilo actual.

 

Jul 13, 2010 at 2:20 PM

Viéndolo mejor, dado que AltNetHispano.Vale.Validator es un servicio y no una entidad, más bien, podría ser algo como:

 

public IEnumerable<string> Save(Person person)
{
	var validationResult = validator.Validate(person)
	if (validationResult.IsValid)
	{
		// personRepository.Save(person);
	}
	return validator.InvalidMessages;
}

Jul 13, 2010 at 2:31 PM

Jorge:

 

public IEnumerable<string> Save(Person person)
{
	if (validator.IsValid(person))
	{
		// personRepository.Save(person);
	}
	return validator.InvalidMessages;
}


a mi esto no me gusta, los servicios no deberían tener estado. Pensa en aplicaciones multithread etc...es un caos.

public IEnumerable Save(Person person)
{
        var validationResult = validator.GetInvalidMessages(person);
	if (validationResult.Any())
	{
		// personRepository.Save(person);
	}
	return validationResult;
}

Esto es mejor, si bien la primera validación se ejecutaría 2 veces en el peor de los casos. Ya que es IEnumerable la interfaz... si queres que no se ejecute dos veces podes hacer un pequeño truco, pero no vale la pena.


 

Jul 13, 2010 at 2:32 PM

ah perdón.. ya tenemos el metodo IsValid que hace lo mismo que el Any() que puse ahí ;)

Jul 13, 2010 at 2:45 PM

Estamos de acuerdo sobre lo primero José, por eso lo corregí en mi segundo mensaje, es que vengo del mundo CSLA y allá la validación va encapsulada en la entidad (entity.IsValid), pero detallo un poco más el último código que escribí:

public IEnumerable<string> Save(Person person)
{
	ValidationResult result = validator.Validate(person)
	if (result.IsValid)
	{
		// personRepository.Save(person);
	}
	return result.InvalidMessages;
}

Pedro propuso el cambio a Predicate<T>, yo comento más bien sobre el uso de la API. Díganme si no les parece, pero a mi me parece que este estilo es más explícito y natural (validator.Validate(person), result.IsValid , result.InvalidMessages).

 

Jul 13, 2010 at 3:47 PM
Edited Jul 13, 2010 at 3:49 PM

Esta ultima forma expresada por Jorge Gamba es la que me parece más natural. Resuelve bien el tema de ser un servicio sin estado al quedar todo el objeto resultado encapsulado en ValidationResult.

- Un caso de uso que necesite solo saber si es valido, le alcanza con invocar:  validator.Validate(res).IsValid --> Bool

- Un caso más elaborado que necesite reportar el tipo de error: puede extraer los mensajes de error desde el objeto ValidationResult incluyendo más información como la propiedad causante de la "ofensa".

 

Fabio comentaba: 

>Poner un Func<T, bool> implica que hay que ir a buscar el mensaje de otro lado y allí empieza la rosca de generar clases de validacción que tengan un metodo de Match y un mensaje... 

>y vamos con la rosca. Por ahora el engine se banca cualquier funcionamiento se le quiera dar externamente. Vendría a ser "Yo ejecuto ese Func, si tenes algo que no te gusta mandame el mensaje,

>si no no me mandes nada." Un solo roundtrip y cero "if" Con un Predidicate sería: "Che te doy este T decime si te gustó: ah, no te gustó ? bueno ahora decime por que en un mensaje." Dos roundtrip y un "if".

 

Propuse esta alternativa porque, en general, me gusta dar más peso a que la interfaz pubicada (API) quede lo más limpia y sencilla posible. Y eso aun a costa de incrementar la complejidad de la "caja negra" que es el framework. Uno siempre espera con un framework que el número de usuarios del framework sea mayor que el número de desarrolladores del framework (es decir, que tenga algo exito): y en este escenario, la facilidad de uso para el cliente tiene prioridad frente a la complejidad interna. De todos modos, en el peor de los casos, un par de ifs, dentro del framework no nos incrementará mucho más la complejidad ciclomática del código.

 

Si me permiten la broma: desde el punto de vista de la complijidad y legibilidad me asustan más algunas de las expresiones lambda que Vds. escriben. }:-)

No me entiendan mal: las expresiones lambda y LINQ son muy potentes y expresivas, pero en mi fuero interno siento que su abuso puede resentir la legibilidad del código en ocasiones.

Jul 16, 2010 at 4:54 PM

Hola señores:

Hice una variante para explorar como quedaria este refactoring del API. Lo tienen aqui por si quieren echar un vistazo. https://hg01.codeplex.com/forks/pjmolina/refactoringapiregisterfork

El API lleva un cambio fuerte e impacta en muchos de los test, ya que la forma de uso cambiaría considerablemente.

Incluso un test que comprobaba la evaluación parcial a proposito, falla ahora con la implementación de IValidationResult que propuse que precalcula todas las reglas que fallaron para un objeto.

Como siempre: comparemos y busquemos pros y contras a cada aproximación, la filosofia de uso tras los dos APIs es muy diferente.

Saludos, Pedro J.