Comments 15
Имплементировать два IEquitable<T>
это неправильное использование интерфейса. С чего это класс T должен реализовывать интерфейс для какого-то другого IFoo? Пусть даже для него есть удобный синтаксис в языке.
Нехорошо модифицировать параметры, которые приходят в конструктор. Если я создаю new Person("blabla", null, null)
я рассчитываю, что LastName будет null, а не какой-нибудь "unknown". А потом словить баг, где я проверяю наличие фамилии (ну вдруг речь про африканца, у которого есть только имя), а у меня никогда это условие не прокает. Придется лезть в конструктор и видеть, что он подменяет то, что я хочу видеть. Так что у конструктора тут два варианта: либо сеттить все, что угодно, либо явно кидать исключение "Должна быть фамилия".
Хэшкод будет давать одинаковый хэш для "A", "B", null и "B", "A", null. В таком случае обычно делается умножение на константу.
Непонятно, зачем статический метод? Почему человек просто не может вызывать ==? Вот честно, я скорее вызову if (a == b)
чем if (PersonStruct.Equals(a,b))
. Это и короче (взгляд сразу цепляется за ==, а имя метода нужно парсить), и понятнее (я не припомню сходу структуры со статическими методами подобного рода во фреймворке). Короче, бесполезный метод.
О какой упаковке речь? От того, что вы для равенства создали новый алиас ничего собственно не поменялось.
Так же неясно, о какой магии речь. Там простой как валенок код "проверить наличие значения в обеих Nullable<T>
, если их нет, то тут все понятно, если есть, то вызвать их собственный Equals":
public override bool Equals(object other) {
if (!hasValue) return other == null;
if (other == null) return false;
return value.Equals(other);
}
На мой взгляд корректная реализация должна выглядеть как-то так:
public struct PersonStruct : IEquatable<PersonStruct>
{
public string FirstName { get; }
public string LastName { get; }
public DateTime? BirthDate { get; }
public PersonStruct(string firstName, string lastName, DateTime? birthDate)
{
FirstName = firstName;
LastName = lastName;
BirthDate = birthDate;
}
public bool Equals(PersonStruct other) => FirstName == other.FirstName && LastName == other.LastName && BirthDate.Equals(other.BirthDate);
public override bool Equals(object obj)
{
if (ReferenceEquals(null, obj)) return false;
return obj is PersonStruct && Equals((PersonStruct) obj);
}
public override int GetHashCode()
{
unchecked
{
var hashCode = (FirstName?.GetHashCode()).GetValueOrDefault();
hashCode = (hashCode * 397) ^ (LastName?.GetHashCode()).GetValueOrDefault();
hashCode = (hashCode * 397) ^ BirthDate.GetHashCode();
return hashCode;
}
}
}
Меньше кода — меньше ошибок. Всё, что нужно для структуры и сравнения тут есть. До Nullable нам не должно быть никакого дела, это у майкрософта должна голова болеть, чтобы правильно сравнить два упакованных объекта (и поверьте, они делают это хорошо). У вас много ссылок на документацию, но при этом я ни разу не видел в требованиях к структурам реализовывать дополнительные статические метод с nullable или имплементировать IEquitable<Nullable<T>>
IEquitable<Nullable> — вы правы, интерфейс для сравнения с другим типом архитектурно неверное решение, и на практике в любом случае нет смысла добавлять каждый раз этот код.
Однако, "До Nullable нам не должно быть никакого дела, это у майкрософта должна голова болеть, чтобы правильно сравнить два упакованных объекта (и поверьте, они делают это хорошо)." — очень спорно.
Достаточно примеров, когда заботы о предотвращении упаковки в платформе нет — недавно только на хабре была статья, где это было хорошо разобрано, например:
- при вызове GetHashCode у enum без приведения к базовому типу происходит упаковка;
- Enum.HasFlag — упаковка;
- передача структур (как есть, без предварительного ToString) при форматирование строк — упаковка.
Проверьте — при наличии методов Equals(T) и Equals(Object), при вызове Equals с аргументом "T?" будет вызван именно Equals(Object), и нет никаких причин считать, что здесь будет вызвана "магия", предотвращающая упаковку.
В вашем примере кода нет операторов ==/!=. Экземпляры ваших структур нельзя будет сравнить через ==, хотя вы сами пишите "я скорее вызову if (a == b)"
Если есть ==(T, T), то нужен и Equals(T, T) — для использования в языках без поддержки кастомных операторов, и для предотвращения упаковки и более быстрой работы (чтобы не был вызван Equals(Object, Object)).
Разумеется, в каждом конкретном случае нужно смотреть, насколько полно нужно реализовывать Object Equality.
Модификация параметров, передаваемых через конструктор — зависит от степени анемичности используемой модели.
А то ведь разные best practices требования — и такое, что строковые свойства (как и массивы) не должны быть null (нужны пустые строки/массивы), если только это не чистая DTO.
Вопрос в выборе модели.
Лезть в конструктор не надо. Надо описывать контракт класса.
GetHashCode — верно. Это уже обсудили в предыдущих публикациях.
Достаточно примеров, когда заботы о предотвращении упаковки в платформе нет — недавно только на хабре была статья, где это было хорошо разобрано, например:
А поподробнее? при вызове StringSplitOptions.RemoveEmptyEntries.GetHashCode()
будет боксинг?
Проверьте — при наличии методов Equals(T) и Equals(Object), при вызове Equals с аргументом "T?" будет вызван именно Equals(Object), и нет никаких причин считать, что здесь будет вызвана "магия", предотвращающая упаковку.
Магии в дотнете вообще мало. Тем более, что в фреймворке уже есть статический метод Nullable.Equals
, который делает все то же самое, только обобщенно.
Модификация параметров, передаваемых через конструктор — зависит от степени анемичности используемой модели.
Не согласен в корне. Конструктор может или отвергнуть параметры, или принять, модифицировать их он не имеет права. Это корень такого количества багов, что даже страшно подумать.
при вызове StringSplitOptions.RemoveEmptyEntries.GetHashCode() будет боксинг?
Да, вот здесь можно посмотреть IL-код боксинга.
Конструктор может или отвергнуть параметры, или принять, модифицировать их он не имеет права. Это корень такого количества багов, что даже страшно подумать.
Согласен.
Если данные требуют некой нормализации (например, чтобы их можно было сравнивать, как в нашем случае, при этом "зашивать" нормализацию в сам код компаратора тоже неправильно), то конструктор должен отвергнуть ненормализованные данные.
Однако, в таком случае получается "разработка по контракту" (Design by Contract), и если вы работаете в команде, то увидите, что никто не будет проверять данные перед передачей в ваш конструктор или метод.
И кстати, лучше делать конструктор lightweight, без исключений, только с инициализацией полей.
А если требуется проверка параметров, то нужно делать фабричный метод Create, выполняющий проверки, и если ок, то вызывающий конструктор, который объявлен как private.
Ведь сам конструктор не создает объект, а лишь инициализирует поля в уже созданном объекте.
И порождение исключения в конструкторе приводит к объекту, на который вы не получили ссылки, и который должен быть убран сборщиком мусора.
--return options.GetHashCode();
027704AA in al,dx
027704AB push eax
027704AC mov dword ptr [ebp-4],ecx
027704AF mov ecx,7282A268h
027704B4 call 026430F4
027704B9 mov edx,eax
027704BB mov eax,dword ptr [ebp-4]
027704BE mov dword ptr [edx+4],eax
027704C1 mov ecx,edx
027704C3 mov eax,dword ptr [ecx]
027704C5 mov eax,dword ptr [eax+28h]
027704C8 call dword ptr [eax+8]
027704CB mov esp,ebp
027704CD pop ebp
027704CE ret
--return ((int)options).GetHashCode();
02770482 in al,dx
02770483 push eax
02770484 xor eax,eax
02770486 mov dword ptr [ebp-4],eax
02770489 mov dword ptr [ebp-4],ecx
0277048C mov eax,dword ptr [ebp-4]
0277048F mov esp,ebp
02770491 pop ebp
02770492 ret
и победитель
--return ((int) options);
02BA0602 in al,dx
02BA0603 mov eax,ecx
02BA0605 pop ebp
02BA0606 ret
Однако, в таком случае получается «разработка по контракту» (Design by Contract), и если вы работаете в команде, то увидите, что никто не будет проверять данные перед передачей в ваш конструктор или метод.
Видимо у нас разные представления о том, что такое команда. У нас например код, который падает с ArgumentNullException залить в основную ветку не получится у человека.
Что касается паттерна Create, то на самом деле я в нем особого смысла не вижу. Какая разница, как назвать, если от этого ничего не меняется? То того, что вы конструктору дали alias плохая практика проставлять все депенденси не ушла. Тут скорее нужно больше выносить во всякие менеджеры, которые можно мокнуть.
С другой стороны, такой паттерн естественно появляется при необходимости асинхронно инстанцировать что-то.
Видимо у нас разные представления о том, что такое команда. У нас например код, который падает с ArgumentNullException залить в основную ветку не получится у человека.
Как раз у нас то с вами, видимо, все-таки одинаковые представления о команде..
дубль.
private static int GetHashCodeHelper(int[] subCodes) { int result = subCodes[0]; for (int i = 1; i < subCodes.Length; i++) result = unchecked(result * 397) ^ subCodes[i]; return result; } public override int GetHashCode() => GetHashCodeHelper( new int[] { this.FirstName.GetHashCode(), this.LastName.GetHashCode(), this.BirthDate.GetHashCode() } );
Раз уж Вы сами говорите, что структуры проще классов в том числе потому, что структура не может быть изменена наследованием, то почему Вы создаёте столь тяжеловесное решение для GetHashCode?
Что именно вы понимаете под эффективностью?
Код взят из предыдущих публикаций, где хелпер используется при наследовании (для классов), чтобы хеш-код вычислялся всегда единообразно.
Вопрос баланса эффективности и компактности/повторного использования кода:
Можно отказаться от хелпера, создания массива и цикла, и тогда в случае вычисления хеша с использованием множителя ("типового" 397) придется все писать вручную, для каждой сущности однообразный повторяющийся код, что может привести к ошибкам:
result = firstField.GetHashCode();
result = unchecked(result * 397) ^ secondField.GetHashCode();
result = unchecked(result * 397) ^ thirdField.GetHashCode();
Самый простой вариант:
result = firstField.GetHashCode() ^ secondField.GetHashCode() ^ thirdField.GetHashCode();
является самым эффективным, но, как уже обсудили, может приводить к частым коллизиям с последующим вызовом Equals, нивелирующим эффективность GetHashCode().
Предложите свой вариант эффективного GetHashCode() без copy-paste.
Создание промежуточного объекта требует времени, пускай и немного, и создаёт нагрузку на сборщик мусора в долгосрочной перспективе.
Guideline: the implementation of GetHashCode must be extremely fast
The whole point of GetHashCode is to optimize a lookup operation; if the call to GetHashCode is slower than looking through those ten thousand items one at a time, then you haven’t made a performance gain.
I classify this as a “guideline” and not a “rule” because it is so vague. How slow is too slow? That’s up to you to decide.
Пруф: https://blogs.msdn.microsoft.com/ericlippert/2011/02/28/guidelines-and-rules-for-gethashcode/
public override int GetHashCode() { unchecked { var hashCode = (FirstName?.GetHashCode()).GetValueOrDefault(); hashCode = (hashCode * 397) ^ (LastName?.GetHashCode()).GetValueOrDefault(); hashCode = (hashCode * 397) ^ BirthDate.GetHashCode(); return hashCode; } }
Эффективный GetHashCode можно генерировать + опционально кэшировать результат прозрачно для пользователя (с помощью того же ConditionalWeakTable). Правда, я не вижу в этом большого смысла.
Насчет генерации: можно посмотреть на пример генерации IComparer<T>
в моей тестовой библиотечке. В таком случае никаких массивов выделять не придется, просто динамически создаем нужный делегат и вызываем. Оверхед на вызов делегата как уже считали ~3ns, что приемлемо.
Но если честно, я не вижу большой проблемы сгенерировать equality members каким-нибудь решарпером. Печально, конечно, что тулчейн не умеет сам после сборки добавлять подобный бойлерплейт-код, но тут пока ничего не попишешь: фича пилится, и я её очень жду.
Реализация GetHashCode является критичной к производительности, если объект будет использоваться как элемент словаря и таких объектов создаётся много. Если это так, то реализация GetHashCode должна быть настолько эффективной, насколько это возможно.
Я лишь утвержаю, что реализация с выделением промежуточного массива менее эффективна чем та, которую можно было бы сделать. В данном случае кэширование и эффективность ортогональны и друг другу никак не мешают.
Здесь имеет место лишь преждевременная пессимизация кода исходя из предпосылок, которые по архитектуре приложения явно не должны случиться.
О сравнении объектов по значению — 6: Structure Equality Implementation