Cómo implementar una propiedad en la clase A que se refiere a una propiedad de un objeto hijo de la clase A

9

Tenemos este código que, cuando se simplifica, se ve así:

public class Room
{
    public Client Client { get; set; }

    public long ClientId
    {
        get
        {
            return Client == null ? 0 : Client.Id;
        }
    }
}

public class Client 
{
    public long Id { get; set; }
}

Ahora tenemos tres puntos de vista.

1) Este es un buen código porque la Clientpropiedad siempre debe establecerse (es decir, no es nula) para Client == nullque nunca ocurra y el valor de Id 0denota un Id falso de todos modos (esta es la opinión del escritor del código ;-))

2) No puede confiar en que la persona que llama sabe que 0es un valor falso para Idy cuando la Clientpropiedad siempre debe establecerse, debe arrojar un exceptionen getcuando la Clientpropiedad sea nula

3) Cuando la Clientpropiedad siempre debe establecerse, simplemente regresa Client.Idy deja que el código arroje una NullRefexcepción cuando la Clientpropiedad resulta ser nula.

¿Cuál de estos es el más correcto? ¿O hay una cuarta posibilidad?

Michel
fuente
55
No es una respuesta, sino solo una nota: nunca arroje excepciones de captadores de propiedades.
Brandon
3
Para explicar el punto de Brandon: stackoverflow.com/a/1488488/569777
MetaFight el
1
Claro: la idea general ( msdn.microsoft.com/en-us/library/… , stackoverflow.com/questions/1488472/… , entre otros) es que las propiedades deben hacer lo menos posible, ser livianas y representar la limpieza , estado público de su objeto, con indizadores que son una ligera excepción. También es un comportamiento inesperado ver excepciones en una ventana de observación para una propiedad durante la depuración.
Brandon
1
No debería (necesita) escribir código que arroje un captador de propiedades. Eso no significa que deba ocultar y tragar cualquier excepción que ocurra debido a que un objeto entra en un estado no admitido.
Ben
44
¿Por qué no puedes simplemente hacer Room.Client.Id? ¿Por qué quieres envolver eso en Room.ClientId? Si es para buscar un cliente, ¿por qué no algo como Room.HasClient {get {return client == null; }} que es más sencillo y todavía permite que las personas hagan el Room.Client.Id normal cuando realmente necesitan la identificación?
James

Respuestas:

25

Parece que deberías limitar la cantidad de estados en los que Roompuede estar tu clase.

El hecho mismo de que esté preguntando qué hacer cuando Clientes nulo es una pista de que Roomel espacio de estado es demasiado grande.

Para simplificar las cosas, no permitiría que la Clientpropiedad de cualquier Roominstancia sea nula. Eso significa que el código dentro Roompuede asumir con seguridad que Clientnunca es nulo.

Si por alguna razón en el futuro se Clientvuelve nullresistir el impulso de apoyar ese estado. Hacerlo aumentará su complejidad de mantenimiento.

En su lugar, permita que el código falle y falle rápidamente. Después de todo, este no es un estado compatible. Si la aplicación entra en este estado, ya ha cruzado una línea de no retorno. Lo único razonable que se puede hacer es cerrar la aplicación.

Esto puede suceder (naturalmente) como resultado de una excepción de referencia nula no controlada.

MetaFight
fuente
2
Agradable. Me gusta la idea "Para simplificar las cosas, no permitiría que la propiedad del Cliente de ninguna instancia de Room sea nula". Eso es realmente mejor que cuestionar qué hacer cuando es nulo
Michel
@Michel si luego decide cambiar ese requisito, puede reemplazar el nullvalor con un NullObject(o más bien NullClient) que aún funcionaría con su código existente y le permitiría definir el comportamiento de un cliente inexistente si es necesario. La pregunta es si el caso "sin cliente" es parte de la lógica empresarial o no.
nulo
3
Totalmente correcto No escriba un solo carácter de código para admitir un estado no compatible. Solo deja que arroje un NullRef.
Ben
@Ben no está de acuerdo; Las pautas de MS establecen explícitamente que los captadores de propiedades no deben lanzar excepciones. Y permitir que se arrojen referencias nulas cuando se pueda lanzar una excepción más significativa es simplemente perezoso.
Andy
1
@Andy comprender el motivo de la directriz le permitirá saber cuándo no se aplica.
Ben
21

Solo algunas consideraciones:

a) ¿Por qué hay un getter específicamente para el ClientId cuando ya hay un getter público para la propia instancia del Cliente? No veo por qué la información de que el ClientId es longtiene que ser tallada en piedra con la firma de Room.

b) Con respecto a la segunda opinión, podría introducir una constante Invalid_Client_Id.

c) Con respecto a la opinión uno y tres (y siendo mi punto principal): ¿Una habitación siempre debe tener un cliente? Tal vez sea solo semántica, pero esto no suena bien. Quizás sería más apropiado tener clases completamente separadas para Room y Client y otra clase que las vincule. Tal vez cita, reserva, ocupación? (Esto depende de lo que realmente esté haciendo). Y en esa clase puede imponer las restricciones "debe tener una habitación" y "debe tener un cliente".

VolkerK
fuente
55
+1 para "No veo, por ejemplo, por qué la información de que el ClientId es largo tiene que ser tallada en piedra en la firma de Room"
Michel
Tu ingles está bien. ;) Solo por leer esta respuesta, no habría sabido que tu lengua materna no es el inglés sin que tú lo digas.
jpmc26
Los puntos B y C son buenos; RE: a, es un método de conveniencia, y el hecho de que la sala tenga una referencia al Cliente podría ser solo un detalle de implementación (se podría argumentar que el Cliente no debería ser públicamente visible)
Andy
17

No estoy de acuerdo con las tres opiniones. Si Clientnunca puede ser null, ¡ entonces ni siquiera lo hagas posiblenull !

  1. Establecer el valor de Clienten el constructor
  2. Lanza un ArgumentNullExceptionen el constructor.

Entonces su código sería algo como:

public class Room
{
    private Client theClient;

    public Room(Client client) {
        if(client == null) throw new ArgumentNullException();
        this.theClient = client;
    }

    public Client Client { 
        get { return theClient; }
        set 
        {
            if (value == null) 
                throw new ArgumentNullException();
            theClient = value;
        }
    }

    public long ClientId
    {
        get
        {
            return theClient.Id;
        }
    }
}
public class Client 
{
    public long Id { get; set; }
}

Esto no está relacionado con su código, pero probablemente sería mejor usar una versión inmutable de Roommaking theClient readonly, y luego, si el cliente cambia, hacer una nueva habitación. Esto mejorará la seguridad del hilo de su código además de la seguridad nula de los otros aspectos de mi respuesta. Ver esta discusión sobre mutable vs inmutable

durron597
fuente
1
¿No deberían ser estos ArgumentNullExceptions?
Mathieu Guindon
44
No. El código del programa nunca debe arrojar un NullReferenceException, lo arroja la VM .Net "cuando hay un intento de desreferenciar una referencia de objeto nulo" . Debería lanzar un ArgumentNullException, que se arroja "cuando se pasa una referencia nula (Nothing en Visual Basic) a un método que no lo acepta como argumento válido".
Pharap
2
@Pharap Soy un programador de Java que intenta escribir código C #, pensé que cometería errores como ese.
durron597
@ durron597 Debería haberlo adivinado por el uso del término 'final' (en este caso, la palabra clave C # correspondiente es readonly). Habría editado, pero sentí que un comentario serviría para explicar mejor por qué (para futuros lectores). Si aún no hubiera dado esta respuesta, estaría dando exactamente la misma solución. La inmutabilidad también es una buena idea, pero no siempre es tan fácil como uno espera.
Pharap
2
Las propiedades generalmente no deberían arrojar excepciones; en lugar de un setter que arroje ArgumentNullException, proporcione un método SetClient en su lugar. Alguien publicó el enlace a una respuesta sobre esto en la pregunta.
Andy
5

Deben evitarse las opciones segunda y tercera: el captador no debe golpear a la persona que llama con la excepción de que no tienen control.

Debe decidir si el Cliente puede ser nulo. Si es así, debe proporcionar una forma para que la persona que llama compruebe si es nulo antes de acceder (p. Ej., Propiedad bool ClientIsNull).

Si decide que el Cliente nunca puede ser nulo, conviértalo en un parámetro obligatorio para el constructor y arroje la excepción allí si se pasa un nulo.

Finalmente, la primera opción también contiene un código de olor. Debe dejar que el Cliente se ocupe de su propia propiedad de identificación. Parece excesivo codificar un captador en una clase de contenedor que simplemente llama a un captador en la clase contenida. Simplemente exponga al Cliente como una propiedad (de lo contrario, terminará duplicando todo lo que un Cliente ya ofrece ).

long clientId = room.Client.Id;

Si el Cliente puede ser nulo, al menos le dará responsabilidad a la persona que llama:

if (room.Client != null){
    long clientId = room.Client.Id;
    /* other code follows... */
}
Kent A.
fuente
1
Creo que "terminarás duplicando todo lo que un cliente ya ofrece" debe estar en negrita o al menos en cursiva.
Pharap
2

Si una propiedad de cliente nulo es un estado admitido, considere usar un objeto nulo .

Pero lo más probable es que este sea un estado excepcional, por lo que debe hacer que sea imposible (o simplemente no muy conveniente) terminar con un valor nulo Client:

public Client Client { get; private set; }

public Room(Client client)
{
    Client = client;
}

public void SetClient(Client client)
{
    if (client == null) throw new ArgumentNullException();
    Client = client;
}

Sin embargo, si no es compatible, no pierda el tiempo con soluciones medio horneadas. En este caso:

return Client == null ? 0 : Client.Id;

¡Has 'resuelto' la NullReferenceException aquí pero a un gran costo! Esto obligaría a todas las personas que llaman Room.ClientIda verificar 0:

var aRoom = new Room(aClient);
var clientId = aRoom.ClientId;
if (clientId == 0) RestartRoombookingWizard();
else ContinueRoombooking(clientid);

Pueden surgir errores extraños con otros desarrolladores (¡o con usted mismo!) Olvidando verificar el valor de retorno de "ErrorCode" en algún momento.
Juega seguro y falla rápido. Permita que se arroje la NullReferenceException y "con gracia" la atrape en algún lugar más arriba de la pila de llamadas en lugar de permitir que la persona que llama arruine las cosas aún más ...

En una nota diferente, si está tan interesado en exponer el Clienty también ClientIdpensar en TellDontAsk . Si le pides demasiado a los objetos en lugar de decirles lo que quieres lograr, puedes terminar de acoplar todo, haciendo que el cambio sea más difícil más adelante.

Laoujin
fuente
Eso NotSupportedExceptionestá siendo mal utilizado, deberías estar usando un ArgumentNullExceptionlugar.
Pharap
1

A partir de c # 6.0, que ahora se publica, solo debe hacer esto,

public class Room
{
    public Room(Client client)
    {
        this.Client = client;
    }

    public Client Client { get; }
}

public class Client
{
    public Client(long id)
    {
        this.Id = id;
    }

    public long Id { get; }
}

Elevar una propiedad de Clienta Roomes una violación evidente de la encapsulación y el principio DRY.

Si necesita acceder al Idde unClient de las Roomque puede hacer,

var clientId = room.Client?.Id;

Tenga en cuenta el uso del operador condicional nulo , clientIdserá una long?, si Clientes null, clientIdserá null, de lo contrario clientIdtendrá el valor de Client.Id.

Jodrell
fuente
pero el tipo de clientides un largo anulable entonces?
Michel
@Michel, bueno, si una habitación debe tener un Cliente, pon un cheque nulo en el ctor. Entonces var clientId = room.Client.Idserá a la vez seguro y long.
Jodrell