Как реализовать свойство класса A, которое ссылается на свойство дочернего объекта класса A

9

У нас есть этот код, который при упрощении выглядит следующим образом:

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

Теперь у нас есть три точки зрения.

1) Это хороший код, потому что Clientсвойство всегда должно быть установлено (т. Е. Не равно нулю), чтобы Client == nullникогда не происходило, и значение 0Id в любом случае обозначает ложный Id (это мнение автора кода ;-))

2) Вы не можете полагаться на то, что вызывающая сторона знает, что 0это ложное значение, Idи когда Clientсвойство всегда должно быть установлено, вы должны бросить exceptionв, getкогда Clientсвойство оказывается пустым

3) Когда Clientсвойство всегда должно быть установлено, вы просто возвращаетесь Client.Idи позволяете коду генерировать NullRefисключение, когда Clientсвойство оказывается пустым.

Что из этого является наиболее правильным? Или есть четвертая возможность?

Мишель
источник
5
Не ответ, а просто примечание: никогда не бросайте исключения от получателей собственности.
Брэндон
3
Чтобы уточнить точку зрения Брэндона: stackoverflow.com/a/1488488/569777
MetaFight
1
Конечно: общая идея ( среди прочего, msdn.microsoft.com/en-us/library/… , stackoverflow.com/questions/1488472/… ) состоит в том, что свойства должны делать как можно меньше, быть легковесными и представлять чистые публичное состояние вашего объекта, за исключением индексаторов. Это также неожиданное поведение - видеть исключения в окне наблюдения для свойства во время отладки.
Брэндон
1
Вы не должны (должны) писать код, который добавляет метод получения свойства. Это не означает, что вы обязаны скрывать и глотать любые исключения, возникающие из-за попадания объекта в неподдерживаемое состояние.
Бен
4
Почему ты не можешь просто сделать Room.Client.Id? Почему вы хотите обернуть это в Room.ClientId? Если это проверить на наличие клиента, то почему бы не что-то вроде Room.HasClient {get {return client == null; }} это более прямо и все же позволяет людям делать обычный Room.Client.Id, когда им действительно нужен идентификатор?
Джеймс

Ответы:

25

Пахнет, как будто вы должны ограничить количество состояний, в которых Roomможет быть ваш класс.

Тот факт, что вы спрашиваете о том, что делать, когда значение Clientравно нулю, указывает на то, что Roomпространство состояний слишком велико.

Для простоты я бы не допустил, чтобы Clientсвойство любого Roomэкземпляра когда-либо было нулевым. Это означает, что код внутри Roomможет смело предполагать, что Clientникогда не является нулевым.

Если по каким-то причинам в будущем Clientстанет nullсопротивляться желание поддержать это государство. Это увеличит сложность обслуживания.

Вместо этого позвольте коду терпеть неудачу и терпеть неудачу быстро. В конце концов, это не поддерживаемое состояние. Если приложение переходит в это состояние, вы уже переступили черту невозврата. Единственное разумное, что можно сделать, - закрыть приложение.

Это может произойти (вполне естественно) в результате необработанного исключения нулевой ссылки.

MetaFight
источник
2
Ницца. Мне нравится мысль: «Чтобы все было просто, я бы не позволил свойству Client любого экземпляра Room быть нулевым». Это действительно лучше, чем задавать вопрос, что делать, когда он нулевой
Мишель
@Michel, если позже вы решите изменить это требование, вы можете заменить nullзначение на NullObject(или скорее NullClient), которое затем будет работать с вашим существующим кодом и позволит вам при необходимости определить поведение для несуществующего клиента. Вопрос в том, является ли случай «без клиента» частью бизнес-логики или нет.
ноль
3
Совершенно верно. Не пишите ни одного символа кода для поддержки неподдерживаемого состояния. Просто дайте ему выбросить NullRef.
Бен
@ Ben Disagree; В руководствах MS прямо говорится, что получатели свойств не должны генерировать исключения. И позволять выбрасывать нулевые ссылки, когда вместо этого можно создать более значимое исключение, просто лениво.
Энди
1
@ Если вы поймете причину, по которой вы будете руководствоваться, вы узнаете, когда она не применяется.
Бен
21

Всего несколько соображений:

а) Почему существует геттер специально для ClientId, когда уже есть публичный геттер для самого экземпляра клиента? Я не понимаю, почему информация о ClientId longдолжна быть высечена в камне в подписи Room.

б) Что касается второго мнения, вы можете ввести константу Invalid_Client_Id.

в) Относительно первого и третьего мнения (и, будучи моим главным пунктом): в комнате всегда должен быть клиент? Может быть, это просто семантика, но это звучит неправильно. Возможно, было бы более уместно иметь совершенно отдельные классы для Room и Client и другой класс, который связывает их вместе. Может быть, назначение, бронирование, размещение? (Это зависит от того, что вы на самом деле делаете.) И в этом классе вы можете применить ограничения «должна быть комната» и «должен быть клиент».

VolkerK
источник
5
+1 за «Я не понимаю, например, почему информация о том, что ClientId является длинным, должна быть вырезана в камне в подписи Room»
Мишель
Ваш английский в порядке. ;) Просто прочитав этот ответ, я бы не узнал, что твой родной язык не английский, если ты не скажешь.
jpmc26
Баллы B & C хорошие; RE: a, это удобный метод, и тот факт, что в комнате есть ссылка на Клиента, может быть просто деталью реализации (можно утверждать, что Клиент не должен быть публично видимым)
Энди
17

Я не согласен со всеми тремя мнениями. Если Clientникогда не может быть null, тогда даже не позволяйте этому бытьnull !

  1. Установите значение Clientв конструкторе
  2. Киньте ArgumentNullExceptionв конструктор.

Таким образом, ваш код будет выглядеть примерно так:

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

Это не связано с вашим кодом, но вам, вероятно, было бы лучше использовать неизменную версию Room, сделав theClient readonly, а затем, если клиент изменился, создать новую комнату. Это улучшит безопасность потока вашего кода в дополнение к нулевой безопасности других аспектов моего ответа. Смотрите это обсуждение мутабельных против неизменных

durron597
источник
1
Разве это не должно быть ArgumentNullExceptionс?
Матье Гиндон
4
Нет. Программный код никогда не должен генерировать a NullReferenceException, он генерируется .Net VM «при попытке разыменования пустой ссылки на объект» . Вы должны бросить ArgumentNullException, который выбрасывается, «когда нулевая ссылка (Nothing в Visual Basic) передается методу, который не принимает ее в качестве допустимого аргумента».
Pharap
2
@Pharap Я программист на Java, пытающийся написать код на C #, я решил, что буду совершать подобные ошибки.
durron597
@ durron597 Я должен был догадаться об этом по использованию термина 'final' (в данном случае соответствующее ключевое слово C # - readonly). Я бы только что отредактировал, но я чувствовал, что комментарий поможет лучше объяснить почему (для будущих читателей). Если бы вы еще не дали этот ответ, я бы дал точно такое же решение. Неизменность - также хорошая идея, но это не всегда так просто, как можно надеяться.
Pharap
2
Свойства обычно не должны вызывать исключения; вместо установщика, который выбрасывает ArgumentNullException, вместо этого предоставьте метод SetClient. Кто-то разместил ссылку на ответ по этому вопросу на вопрос.
Энди
5

Следует избегать второго и третьего вариантов - получатель не должен шлепать вызывающего, за исключением того, что он не контролирует его.

Вы должны решить, может ли Клиент быть нулевым. Если это так, вы должны предоставить вызывающей стороне способ проверить, является ли он пустым, прежде чем получить к нему доступ (например, свойство bool ClientIsNull).

Если вы решите, что Client никогда не может иметь значение null, то сделайте его обязательным параметром для конструктора и добавьте туда исключение, если в него передан null.

Наконец, первый вариант также содержит запах кода. Вы должны позволить Клиенту иметь дело со своим собственным идентификатором. Это кажется излишним - кодировать метод получения в классе контейнера, который просто вызывает метод получения в содержащемся классе. Просто представьте Клиента как свойство (в противном случае вы в конечном итоге продублируете все, что Клиент уже предлагает ).

long clientId = room.Client.Id;

Если Client может быть нулевым, то вы по крайней мере возьмете ответственность перед вызывающей стороной:

if (room.Client != null){
    long clientId = room.Client.Id;
    /* other code follows... */
}
Кент А.
источник
1
Я думаю, что «в конечном итоге вы продублируете все, что клиент уже предлагает» должно быть выделено жирным шрифтом или хотя бы курсивом.
Pharap
2

Если нулевое свойство Client является поддерживаемым состоянием, рассмотрите возможность использования NullObject .

Но, скорее всего, это исключительное состояние, поэтому вы должны сделать невозможным (или просто не очень удобным) получить нулевое значение 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;
}

Однако, если это не поддерживается, не тратьте время на полуобжаренные растворы. В этом случае:

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

Вы «решили» исключение NullReferenceException здесь, но по высокой цене! Это заставит всех абонентов Room.ClientIdпроверять наличие 0:

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

Странные ошибки могут возникать из-за того, что другие разработчики (или вы сами!) Забывают проверить возвращаемое значение ErrorCode в какой-то момент времени.
Играйте безопасно и быстро. Позвольте выбрасывать исключение NullReferenceException и "изящно" перехватывать его где-то выше в стеке вызовов, вместо того, чтобы вызывающий мог все испортить еще больше ...

С другой стороны, если вы так заинтересованы, чтобы показать, Clientа также ClientIdподумать о TellDontAsk . Если вы попросите у объектов слишком много, вместо того, чтобы сказать им, чего вы хотите достичь, вы можете прекратить объединять все вместе, что усложнит изменения в дальнейшем.

Laoujin
источник
Это NotSupportedExceptionнеправильно, вы должны использовать ArgumentNullExceptionвместо.
Pharap
1

Начиная с c # 6.0, который сейчас выпущен, вы должны просто сделать это,

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

Повышение свойства Clientto Roomявляется очевидным нарушением инкапсуляции и принципа DRY.

Если вам необходимо получить доступ к Idиз Clientиз Roomвы можете сделать,

var clientId = room.Client?.Id;

Обратите внимание на использование Null условного оператора , clientIdбудет long?, если Clientесть null, clientIdбудет null, в противном случае clientIdбудет иметь значение Client.Id.

Jodrell
источник
но тип clientidэто обнуляемый долго тогда?
Мишель
@ Хорошо, если в комнате должен быть Клиент, поставьте нулевую отметку в ctor. Тогда var clientId = room.Client.Idбудет и безопасно, и long.
Джодрелл