Comment implémenter une propriété sur la classe A qui fait référence à une propriété d'un objet enfant de la classe A

9

Nous avons ce code qui, une fois simplifié, ressemble à ceci:

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; }
}

Nous avons maintenant trois points de vue.

1) Il s'agit d'un bon code car la Clientpropriété doit toujours être définie (c'est-à-dire non nulle), de sorte que la valeur Client == nullne se produira jamais et que la valeur Id 0indique un faux Id de toute façon (c'est l'opinion de l'auteur du code ;-))

2) Vous ne pouvez pas compter sur l'appelant pour savoir qu'il 0s'agit d'une valeur fausse Idet lorsque la Clientpropriété doit toujours être définie, vous devez jeter un exceptiondans la getlorsque la Clientpropriété se trouve être nulle.

3) Lorsque la Clientpropriété doit toujours être définie, il vous suffit de revenir Client.Idet de laisser le code lever une NullRefexception lorsque la Clientpropriété est nulle.

Laquelle de ces réponses est la plus correcte? Ou existe-t-il une quatrième possibilité?

Michel
la source
5
Pas une réponse mais juste une note: ne jetez jamais d'exceptions aux getters de propriété.
Brandon
3
Pour développer le point de Brandon: stackoverflow.com/a/1488488/569777
MetaFight
1
Bien sûr: L'idée générale ( msdn.microsoft.com/en-us/library/… , stackoverflow.com/questions/1488472/… , entre autres) est que les propriétés devraient faire le moins possible, être légères et représenter le propre , l'état public de votre objet, les indexeurs étant une légère exception. C'est également un comportement inattendu de voir des exceptions dans une fenêtre de surveillance pour une propriété pendant le débogage.
Brandon
1
Vous ne devriez pas (avoir besoin) d'écrire du code qui lance un getter de propriété. Cela ne signifie pas que vous devez masquer et avaler toutes les exceptions qui se produisent en raison d'un objet entrant dans un état non pris en charge.
Ben
4
Pourquoi ne pouvez-vous pas simplement faire Room.Client.Id? Pourquoi voulez-vous envelopper cela dans Room.ClientId? Si c'est pour rechercher un client, alors pourquoi pas quelque chose comme Room.HasClient {get {return client == null; }} qui est plus simple et permet toujours aux gens de faire le Room.Client.Id normal quand ils ont réellement besoin de l'id?
James

Réponses:

25

Cela sent que vous devriez limiter le nombre d'états dans lesquels votre Roomclasse peut être.

Le fait même que vous vous demandiez quoi faire quand Clientest nul est un indice que Rooml'espace d'état est trop grand.

Pour garder les choses simples, je n'autoriserais jamais la Clientpropriété d'une Roominstance à être nulle. Cela signifie que le code qu'il contient Roompeut sans risque supposer que le Clientn'est jamais nul.

Si pour une raison quelconque dans l'avenir Clientdevient nullrésister à l'envie de soutenir cet état. Cela augmentera la complexité de votre maintenance.

Au lieu de cela, laissez le code échouer et échouer rapidement. Après tout, ce n'est pas un état pris en charge. Si l'application se met dans cet état, vous avez déjà franchi une ligne de non-retour. La seule chose raisonnable à faire est alors de fermer l'application.

Cela peut se produire (tout naturellement) à la suite d'une exception de référence nulle non gérée.

MetaFight
la source
2
Agréable. J'aime la pensée "Pour simplifier les choses, je ne laisserais jamais la propriété Client d'une instance de Room être nulle." C'est en effet mieux que de se demander quoi faire quand elle est nulle
Michel
@Michel si vous décidez par la suite de modifier cette exigence, vous pouvez remplacer la nullvaleur par un NullObject(ou plutôt NullClient) qui fonctionnerait alors avec votre code existant et vous permettrait de définir le comportement d'un client inexistant si nécessaire. La question est de savoir si le cas «sans client» fait partie de la logique métier ou non.
null
3
Tout à fait correct. N'écrivez pas un seul caractère de code pour prendre en charge un état non pris en charge. Laissez-le simplement lancer un NullRef.
Ben
@Ben Disagree; Les directives MS indiquent explicitement que les accesseurs de propriété ne doivent pas lever d'exceptions. Et permettre à des références nulles d'être levées lorsqu'une exception plus significative pourrait être levée à la place est juste paresseux.
Andy
1
@Andy comprendre la raison de la directive vous permettra de savoir quand elle ne s'applique pas.
Ben
21

Quelques considérations:

a) Pourquoi existe-t-il un getter spécifiquement pour ClientId alors qu'il existe déjà un getter public pour l'instance Client elle-même? Je ne vois pas pourquoi les informations sur le ClientId longdoivent être gravées dans la pierre dans la signature de Room.

b) Concernant le deuxième avis, vous pourriez introduire une constante Invalid_Client_Id.

c) Concernant les opinions un et trois (et étant mon point principal): Une chambre doit toujours avoir un client? C'est peut-être juste de la sémantique mais cela ne sonne pas bien. Il serait peut-être plus approprié d'avoir des classes complètement séparées pour la chambre et le client et une autre classe qui les relie. Peut-être rendez-vous, réservation, occupation? (Cela dépend de ce que vous faites réellement.) Et sur cette classe, vous pouvez appliquer les contraintes "doit avoir une salle" et "doit avoir un client".

VolkerK
la source
5
+1 pour "Je ne vois pas par exemple pourquoi les informations que le ClientId est long doivent être gravées dans la pierre dans la signature de Room"
Michel
Ton anglais est bon. ;) Rien qu'en lisant cette réponse, je n'aurais pas su que votre langue maternelle n'est pas l'anglais sans que vous le disiez.
jpmc26
Les points B et C sont bons; RE: a, c'est une méthode pratique, et le fait que la pièce ait une référence au client pourrait juste être un détail d'implémentation (on pourrait faire valoir que le client ne devrait pas être publiquement visible)
Andy
17

Je ne suis pas d'accord avec les trois opinions. Si Clientcela ne peut jamais arriver null, alors ne le permettez même pasnull !

  1. Définissez la valeur de Clientdans le constructeur
  2. Jetez un ArgumentNullExceptiondans le constructeur.

Donc, votre code serait quelque chose comme:

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; }
}

Ceci n'est pas lié à votre code, mais vous feriez probablement mieux d'utiliser une version immuable de Roomen créant theClient readonly, puis si le client change, en créant une nouvelle salle. Cela améliorera la sécurité des threads de votre code en plus de la sécurité nulle des autres aspects de ma réponse. Voir cette discussion sur mutable vs immuable

durron597
la source
1
Cela ne devrait-il pas être ArgumentNullExceptions?
Mathieu Guindon
4
Non. Le code du programme ne doit jamais lancer un NullReferenceException, il est lancé par la VM .Net "quand il y a une tentative de déréférencer une référence d'objet nul" . Vous devez lancer un ArgumentNullException, qui est levé "lorsqu'une référence null (Nothing en Visual Basic) est passée à une méthode qui ne l'accepte pas comme argument valide".
Pharap
2
@Pharap Je suis un programmeur Java essayant d'écrire du code C #, je me suis dit que je ferais des erreurs comme ça.
durron597
@ durron597 J'aurais dû deviner que d'après l'utilisation du terme «final» (dans ce cas, le mot-clé C # correspondant est readonly). J'aurais juste édité mais je sentais qu'un commentaire servirait à mieux expliquer pourquoi (pour les futurs lecteurs). Si vous n'aviez pas déjà donné cette réponse, je donnerais exactement la même solution. L'immuabilité est également une bonne idée, mais ce n'est pas toujours aussi facile qu'on l'espère.
Pharap
2
Les propriétés ne doivent généralement pas lever d'exceptions; au lieu d'un setter qui lève ArgumentNullException, fournissez une méthode SetClient à la place. Quelqu'un a affiché le lien vers une réponse à ce sujet sur la question.
Andy
5

Les deuxième et troisième options doivent être évitées - le getter ne doit pas frapper l'appelant à une exception qu'il n'a aucun contrôle.

Vous devez décider si le client peut être nul. Si tel est le cas, vous devez fournir à l'appelant un moyen de vérifier s'il est nul avant d'y accéder (par exemple, la propriété bool ClientIsNull).

Si vous décidez que le client ne peut jamais être nul, faites-en un paramètre obligatoire pour le constructeur et lancez-y l'exception si un null est passé.

Enfin, la première option contient également une odeur de code. Vous devez laisser le client gérer sa propre propriété d'identification. Il semble exagéré de coder un getter dans une classe conteneur qui appelle simplement un getter sur la classe contenue. Exposez simplement le client en tant que propriété (sinon, vous finirez par dupliquer tout ce qu'un client offre déjà ).

long clientId = room.Client.Id;

Si le client peut être nul, vous allez au moins donner la responsabilité à l'appelant:

if (room.Client != null){
    long clientId = room.Client.Id;
    /* other code follows... */
}
Kent A.
la source
1
Je pense que "vous finirez par dupliquer tout ce qu'un client offre déjà" doit être en gras ou au moins en italique.
Pharap
2

Si une propriété Client nulle est un état pris en charge, envisagez d'utiliser un objet NullObject .

Mais très probablement, c'est un état exceptionnel, vous devriez donc rendre impossible (ou tout simplement pas très pratique) de se retrouver avec un null 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;
}

S'il n'est pas pris en charge cependant, ne perdez pas de temps sur des solutions à moitié cuites. Dans ce cas:

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

Vous avez "résolu" l'exception NullReferenceException ici, mais à un coût élevé! Cela forcerait tous les appelants Room.ClientIdà vérifier 0:

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

Des bogues étranges peuvent survenir avec d'autres développeurs (ou vous-même!) En oubliant de vérifier la valeur de retour "ErrorCode" à un moment donné.
Jouez en toute sécurité et échouez rapidement. Autoriser la levée de l'exception NullReferenceException et l'attraper "gracieusement" quelque part plus haut dans la pile des appels au lieu de permettre à l'appelant de gâcher encore plus les choses ...

Sur une note différente, si vous êtes si désireux d'exposer le Clientet aussi de ClientIdpenser à TellDontAsk . Si vous en demandez trop aux objets au lieu de leur dire ce que vous voulez réaliser, vous pouvez mettre fin à tout coupler ensemble, ce qui rendra le changement plus difficile plus tard.

Laoujin
la source
Cela NotSupportedExceptionest mal utilisé, vous devriez plutôt utiliser un ArgumentNullException.
Pharap
1

À partir de c # 6.0, qui est maintenant disponible, vous devez simplement le faire,

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; }
}

Élever une propriété de Clientto Roomest une violation évidente de l'encapsulation et du principe DRY.

Si vous devez accéder à l' Idun Clientdes éléments que Roomvous pouvez faire,

var clientId = room.Client?.Id;

Notez que l'utilisation de l' opérateur conditionnel Null , clientIdsera un long?, si Clientest null, clientIdsera null, sinon clientIdaura la valeur de Client.Id.

Jodrell
la source
mais le type de clientidest un nullable longtemps alors?
Michel
@Michel bien, si une chambre doit avoir un client, mettez un chèque nul dans le ctor. Alors var clientId = room.Client.Idsera à la fois sûr et long.
Jodrell