Это хорошая практика, чтобы избежать констант с помощью геттеров?

25

Является ли хорошей практикой замена констант, используемых вне классов, геттерами?

Как пример, лучше использовать if User.getRole().getCode() == Role.CODE_ADMINили if User.getRole().isCodeAdmin()?

Это привело бы к этому классу:

class Role {
    constant CODE_ADMIN = "admin"
    constant CODE_USER = "user"

    private code

    getRoleCode() {
       return Role.code
    }

    isCodeAdmin () {
       return Role.code == Role.CODE_ADMIN
    }

    isCodeUser () {
       return Role.code == Role.CODE_USER
    }
}
перейти к
источник
15
Я бы лучше использовал что-то вроде User.HasRole(Role.Admin).
CodesInChaos
4
Проверьте принцип « Не спрашивай» .
Энди
Я подвергаю сомнению предпосылку: User.getRole().getCode()это уже неприятное чтение, сравнение Кодекса с Ролью делает его еще более неловким.
Msw

Ответы:

47

Прежде всего, обратите внимание, что выполнение чего-то подобного entity.underlyingEntity.underlyingEntity.method()считается запахом кода согласно Закону Деметры . Таким образом, вы раскрываете много деталей реализации потребителю. И каждая необходимость расширения или модификации такой системы будет очень больно.

Поэтому, учитывая это, я бы порекомендовал вам использовать метод HasRoleили IsAdminв Userсоответствии с комментарием CodesInChaos. Таким образом, способ реализации ролей для пользователя остается деталью реализации для потребителя. Кроме того, более естественно спрашивать пользователя о его роли, а не спрашивать его о деталях его роли, а затем принимать решение на основе этого.


Пожалуйста, также избегайте использования strings, если это необходимо. nameхороший пример stringпеременной, потому что ее содержимое неизвестно заранее. С другой стороны, что-то вроде того, roleгде у вас есть два разных значения, которые хорошо известны во время компиляции, вам лучше использовать строгую типизацию. Вот где в игру вступает тип перечисления ...

сравнить

public bool HasRole(string role)

с

public enum Role { Admin, User }

public bool HasRole(Role role)

Второй случай дает мне гораздо больше представления о том, что я должен обойти. Это также препятствует тому, чтобы я ошибочно передал инвалид stringв случае, если я понятия не имел о ваших константах роли.


Далее следует решение о том, как будет выглядеть роль. Вы можете использовать enum, который хранится непосредственно у пользователя:

public enum Role
{
    Admin,
    User
}

public class User
{
    private Role _role;

    public bool HasRole(Role role)
    {
        return _role == role;
    }

    // or
    public bool IsAdmin()
    {
        return _role == Role.Admin;
    }
}

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

public enum RoleType
{
    User,
    Admin
}

public class Role
{
    private RoleType _roleType;

    public bool IsAdmin()
    {
        return _roleType == RoleType.Admin;
    }

    public bool IsUser()
    {
        return _roleType == RoleType.User;
    }

    // more role-specific logic...
}

public class User
{
    private Role _role;

    public bool IsAdmin()
    {
        return _role.IsAdmin();
    }

    public bool IsUser()
    {
        return _role.IsUser();
    }
}

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

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

Зденек Елинек
источник
10
Я бы хотел, чтобы люди так делали это везде. Наличие метода get для атрибута privatr, просто для проверки, является ли он равным чему-то другому, является такой ужасной практикой.
Энди
2
Re "instance.property.property.method () ..." Разве это не жидкая штука ?
Питер Мортенсен
2
@PeterMortensen Это отличается от свободного интерфейса. Свободный интерфейс по типу X, безусловно, позволит вам создавать строки вызовов функций, например X.foo().bar().fee().... В этом свободном интерфейсе foo, bar и fee все будут функциями внутри класса, Xвозвращающими объект типа X. Но для instance.property.property.method (), упомянутого в этом примере, эти два propertyвызова фактически будут в отдельных классах. Проблема в том, что вы проходите через несколько уровней абстракции, чтобы овладеть деталями низкого уровня.
Shaz
10
Хороший ответ, но Закон Деметры - не упражнение по подсчету точек. instance.property.property.method()не обязательно является нарушением или даже запахом кода; это зависит от того, работаете ли вы с объектами или структурами данных. Node.Parent.RightChild.Remove()Вероятно, не является нарушением LoD (хотя вонючий по другим причинам). var role = User.Role; var roleCode = role.Code; var isAdmin = roleCode == ADMIN;это почти наверняка нарушение, несмотря на то, что я всегда «использовал только одну точку».
Карл Лет
1
Я понимаю, что instance.property.property.method()это нарушение LoD, но у OP instance.method().method()должно быть все в порядке. В вашем последнем примере есть так много стандартного кода, Userкоторый служит только фасадом для Role.
Берги
9

Кажется глупым иметь функцию, чтобы проверить, является ли сохраненный код кодом администратора. То, что вы действительно хотите знать, является ли этот человек администратором. Поэтому, если вы не хотите раскрывать константы, вам также не следует раскрывать наличие кода и вызывать методы isAdmin () и isUser ().

Тем не менее, «если User.getRole (). GetCode () == Role.CODE_ADMIN» действительно полезен только для проверки того, что пользователь является администратором. Сколько вещей нужно помнить разработчику, чтобы написать эту строку? Он или она должны помнить, что у пользователя есть роль, у роли есть код, а у класса Role есть константы для кодов. Это много информации, которая просто о реализации.

gnasher729
источник
3
Хуже того: у пользователя всегда есть ровно одна роль, ни больше, ни меньше.
Дедупликатор
5

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

И это делает ужасным улучшение. Может быть, вы хотели бы иметь суперпользовательский тип в один момент, который, очевидно, также имеет права администратора. С инкапсуляцией, это в основном однострочное добавление.

Он не только короткий и чистый, но и простой в использовании и понимании. И - может быть, больше всего - трудно ошибиться.

Эйко
источник
2

Хотя я в значительной степени согласен с предложениями избегать констант и иметь метод isFoo()и т. Д., Один возможный контрпример.

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

Обратите внимание, что использование перечислений или hasRole()избавление от необходимости писать сотни методов, так что это лучший из всех возможных в мире.

user949300
источник
2

Я не думаю, что любой из представленных вами вариантов в корне неверен.

Я вижу, вы не предложили одну вещь, которую я бы назвал категорически неправильной: жесткое программирование кодов ролей в функциях вне класса Role. То есть:

if (user.getRole().equals("Administrator")) ...

Я бы сказал, это определенно неправильно. Я видел программы, которые делают это, а затем получают загадочные ошибки, потому что кто-то неправильно записал строку. Я вспоминаю один раз, когда программист писал «сток», когда функция проверяла «сток».

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

public bool isActuary() { return code==Role.ACTUARY; }
public bool isAccountant() { return code==Role.ACTUARY; }
... etc ...

Лично я бы также избегал цепочек звонков. Я бы лучше написал

if (user.getRole().equals(Role.FOOBATER))

тогда

if (user.getRole().getRoleCode()==Role.FOOBATER_CODE)

и в этот момент, почему примечание напишите:

if (user.hasRole(Role.FOOBATER))

Затем сообщите классу User, как проверить роль.

сойка
источник