Рефакторинг кучу и отчетности


Пытался рефакторить этот код и не могу показаться, чтобы придумать способ сделать его чище. Тип - это свойство в мое лицо класса.

foreach (var item in list)
{
    if (person.Type != PersonType.Employee && person.Type != PersonType.Manager && person.Type != PersonType.Contractor && person.Type != PersonType.Executive)
    {
        DoSomething();
    }
}

public enum PersonType : int {
   Employee = 0,
   Manager = 1,
   Contractor = 2,
   President = 3,
   Executive = 4
}

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



1113
8
задан 23 февраля 2011 в 03:02 Источник Поделиться
Комментарии
7 ответов

Если “allowedness” из PersonType значение определенного определенного алгоритма вы пишете, я обычно пишу это как массив:

var disallowedTypes = new[] {
PersonType.Employee,
PersonType.Manager,
PersonType.Contractor,
PersonType.Executive
};

foreach (var item in list.Where(p => !disallowedTypes.Contains(p.Type)))
DoSomething(item);

Если этот набор запрещенных типов занимает центральное место в ваше приложение (или конкретный класс), я бы объявить его статическим полем в статическом классе (или в соответствующем классе). Этак всю вашу программу (или этого класса) имеет удобный доступ к нему, и вам нужно только обновить ее в одном месте при изменении бизнес-логики:

public static class Data
{
public static readonly PersonType[] DisallowedTypes = {
PersonType.Employee,
PersonType.Manager,
PersonType.Contractor,
PersonType.Executive
};
}

// [ ... ]

foreach (var item in list.Where(p => !Data.DisallowedTypes.Contains(p.Type)))
DoSomething(item);

Если набор запрещенных видах присуща семантика PersonType перечисление само собой, я хотел бы сделать это очень ясно с помощью пользовательских атрибутов перечислимого типа сам. Конечно, вы должны придумать более описательное имя, чем isallowed задано и исправить комментария XML атрибута типа объяснить, что это действительно означает:

/// <summary>Specifies that a <see cref="PersonType"/> value is allowed.</summary>
public sealed class IsAllowedAttribute : Attribute { }

public enum PersonType
{
Employee,
Manager,
Contractor,
Executive,

[IsAllowed]
President
}

public static class Data
{
/// <summary>This array is automatically prefilled with the
/// PersonType values that do not have an [IsAllowed] custom attribute.
/// To change this array, change the custom attributes in the
/// <see cref="PersonType"/> enum instead.</summary>
public static readonly PersonType[] DisallowedTypes;

static Data()
{
DisallowedTypes = typeof(PersonType)
.GetFields(BindingFlags.Public | BindingFlags.Static)
.Where(f => !f.IsDefined(typeof(IsAllowedAttribute), false))
.Select(f => f.GetValue(null))
.ToArray();
}
}

Таким образом, информация, является ли какое-либо определенное значение enum-это разрешено или не хранится само по себе перечисление и ни в каком другом месте. Что делает код очень ясно, легко следовать, и легко изменить в естественно-правильном пути.

13
ответ дан 23 февраля 2011 в 05:02 Источник Поделиться

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

interface Person { void DoSomething(); }

class PrivilegedPerson : Person {
public void DoSomething() {
// do something
}
}

class UnprivilegedPerson : Person {
public void DoSomething() {} // literally do nothing
}

class Employee : UnprivilegedPerson {}
class Manager : UnprivilegedPerson {}
class Contractor : UnprivilegedPerson {}
class President : UnprivilegedPerson {}
class Executive : UnprivilegedPerson {}

14
ответ дан 23 февраля 2011 в 06:02 Источник Поделиться

[Править] исправлены дали редактировать на вопрос

Попробовать чем-то вроде этого

[Flags]
public enum PersonType
{
None = 0,
Employee = 1 << 0,
Manager = 1 << 1,
Contractor = 1 << 2,
President = 1 << 3,
Executive = 1 << 4
}

foreach (var item in list)
{
if ((person.Type & PersonType.President) == 0)
{
DoSomething();
}
}

Теперь вы можете также добавить сочетаются такие ценности, как

[Flags]
public enum PersonType
{
None = 0,
Employee = 1 << 0,
Manager = 1 << 1,
Contractor = 1 << 2,
President = 1 << 3,
Executive = 1 << 4,
NotPresident = Employee | Manager | Contractor | Executive,
Boss = Manager | President | Executive
}

Учитывая это перечисление, этот тест Нанит проходит

[Test]
public void TestEnum()
{
var personType = PersonType.Manager | PersonType.Contractor;
Assert.That(personType == PersonType.None, Is.Not.True);
Assert.That((personType & PersonType.Manager) != 0, Is.True);
Assert.That((personType & PersonType.Boss) != 0, Is.True);
Assert.That((personType & PersonType.Employee) != 0, Is.Not.True);
}

5
ответ дан 23 февраля 2011 в 03:02 Источник Поделиться

Если вы не хотите превратить PersonType в реальный полиморфный класс, простая очистка:

foreach (var item in list)
{
switch (person.Type) {
case PersonType.Employee:
case PersonType.Manager:
case PersonType.Contractor:
case PersonType.Executive:
// Do nothing.
break;
default:
DoSomething();
}
}

Вы должны быть осторожны с перечисления, хотя. Если новый PersonType добавляется, это сделать() правильное поведение для этого типа? Безопасного осуществления не полагаться на значения по умолчанию.

3
ответ дан 24 февраля 2011 в 04:02 Источник Поделиться

Я бы, наверное, пойти с кодом, который выглядел как настоящий.

list.ForEach(c => 
{
if (DoAction(c.Type))
{
DoSomething();
}
});

private static bool DoAction(PersonType personType)
{
switch (personType)
{
case PersonType.Employee:
case PersonType.Manager:
case PersonType.Contractor:
case PersonType.Executive:
return false;
default:
return true;
}
}

2
ответ дан 23 февраля 2011 в 03:02 Источник Поделиться

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

foreach (var item in list)
{
if (!Enum.IsDefined(typeof(PersonType), person.Type))
{
DoSomething();
}
}

2
ответ дан 23 февраля 2011 в 03:02 Источник Поделиться

Если это всего лишь один тестовый кадр, то я хотел бы использовать что-то вроде этого. Если это было использовано в нескольких местах, я бы, вероятно, использовать флаги, как НДР.

PersonType[] ignoredPersonTypes = 
{
PersonType.Employee,
PersonType.Manager,
PersonType.Contractor,
PersonType.Executive,
}

foreach (var item in list)
{
if (ignoredPersonTypes.Contains(person.Type) == false)
{
DoSomething();
}
}

0
ответ дан 23 февраля 2011 в 05:02 Источник Поделиться