Правильный подход, когда нарушение SRP, похоже, сбрасывает сложность по цепочке

Question

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

Этот код предназначен для очистки данных. В настоящее время здесь есть два отдельных процесса: мы очищаем адресные данные с помощью внешнего приложения, которое вызывается через код. Мы очищаем другие поля данных с помощью внутренних алгоритмов на C#.

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

public interface IAddressCleaner 
{ 
    string CleanAddress(StringBuilder inputAddress); 
    void CleanFile(FileContents fc); 
} 

public interface IFieldCleaner 
{ 
    string CleanPhoneNumber(string phoneToClean); 
    void CleanAllPhoneFields(FileRow row, FileContents fc); 
    void MatchObscentities(FileRow row, FileContents fc); 
    void CleanEmailFields(FileRow row, FileContents fc); 
} 

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

С другой стороны, похоже, что IFieldCleaner уже нарушает SRP, потому что он выполняет три функции: чистка телефонных номеров, электронных писем и поиск грубых слов, все из которых являются логически отличными процессами. Таким образом, казалось бы, есть причина разделить его на IPhoneCleaner, IObscenityMatcher и IEmailCleaner.

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

public class ReadFileService : IExecutableObject 
{ 
    private ILogger _log; 
    private IRepository _rep; 
    private IFileHelper _fileHelp; 
    private IFieldCleaner _fieldCleaner; 
    private IFileParser _fileParser; 
    private IFileWriter _fileWriter; 
    private IEmailService _mailService; 
    private IAddressCleaner _addressCleaner; 

    public ReadFileService(ILogger log, IRepository rep, IFileHelper fileHelp, IFileParser fileParse, IFileWriter fileWrite, IEmailService email, IAddressCleaner addressCleaner) 
    { 
     // assign to privates 
    } 

    // functions 
} 

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

Какой здесь подход? Должен ли я иметь один интерфейс ICleaner или разделить его на пять?

Answer 1

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

Удачи!

---

У вас есть доступ к Pluralsight? Покупка быстрого месяца полностью стоит того, чтобы пройти через Encapsulation and SOLID. Один из моментов «а-ха», которые я испытывал при просмотре, заключался в том, чтобы взглянуть на ваши сигнатуры методов, чтобы помочь определить интерфейсы, которые вы могли бы извлечь, чтобы упростить код. Игнорируйте имена, просто посмотрите на параметры.

Я попытаюсь выполнить упражнение с кодом, который вы предоставили, но мне нужно будет сделать предположения на пути, который может быть неправильным.

На IFieldCleaner у вас есть 3 метода с той же подписью:

void CleanAllPhoneFields(FileRow row, FileContents fc); 
void MatchObscentities(FileRow row, FileContents fc); 
void CleanEmailFields(FileRow row, FileContents fc); 

Обратите внимание, как все эти методы являются точно такой же. Это наводит на мысль можно извлечь один интерфейс с 3-х реализаций:

interface IFieldCleaner { 
    void Clean(FileRow row, FileContents fc); 
} 
class PhoneFieldCleaner : IFieldCleaner { } 
class ObscentitiesFieldCleaner : IFieldCleaner { } 
class EmailFieldCleaner : IFieldCleaner { } 

Теперь это приятно разделить обязанности по очистке этих полей в укус размера классов.

Теперь у вас есть несколько других методов очистки:

string CleanPhoneNumber(string phoneNumber); 
string CleanAddress(StringBuilder inputAddress); 

Они очень похожи, за исключением того, один берет StringBuilder предположительно потому, что реализация заботится об отдельных строках? Давайте просто включить его в string и предположим, что реализация будет заботиться о линии расщепления/разборе, то мы получим тот же результат, как и раньше - два метода с той же подписью:

string CleanPhoneNumber(string phoneNumber); 
string CleanAddress(string inputAddress); 

Итак, после нашей предыдущей логики давайте также создать интерфейс, связанный с дезинфицирующими строками:

interface IStringCleaner { 
    string Clean(string s); 
} 

class PhoneNumberStringCleaner : IStringCleaner { } 
class AddressStringCleaner : IStringCleaner { } 

Теперь мы разделены этими обязанности в свои реализации.

На данный момент у нас есть только один метод осталось решить:

void CleanFile(FileContents fc); 

Я не уверен, что делает этот метод. Почему это часть IAddressCleaner? Итак, на данный момент я оставлю это из обсуждения - возможно, это способ прочитать файл, найти адрес, затем очистить его, и в этом случае вы можете позвонить, называя наш новый AddressStringCleaner.

Итак, давайте посмотрим, где мы находимся.

interface IFieldCleaner { 
    void Clean(FileRow row, FileContents fc); 
} 

class PhoneFieldCleaner : IFieldCleaner { } 
class ObscentitiesFieldCleaner : IFieldCleaner { } 
class EmailFieldCleaner : IFieldCleaner { } 

interface IStringCleaner { 
    string Clean(string s); 
} 

class PhoneNumberStringCleaner : IStringCleaner { } 
class AddressStringCleaner : IStringCleaner { } 

Они все кажутся похожи на меня и что-то пахнет. Основываясь на ваших оригинальных именах методов, таких как Clean Все поля Поля, похоже, вы можете использовать цикл, чтобы просто очистить определенные столбцы от FileRow? Но почему также зависит от FileContents? Опять же, я не вижу вашей реализации, поэтому я не совсем уверен. Возможно, вы намерены передать исходный файл или базу данных?

Я также не могу увидеть, где вы магазин вымытые результаты - большинство из предыдущих методов, возвращаемые void означает, что вызов метода имеет некоторый побочный эффект (т.е. это команда), тогда как пара методов возвращается только чистый string (запрос).

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

interface IStringCleaner { 
    string Clean(string s); 
} 

class PhoneNumberStringCleaner : IStringCleaner { } 
class AddressStringCleaner : IStringCleaner { } 
class ObscenitiesStringCleaner : IStringCleaner { } 
class EmailStringCleaner : IStringCleaner { } 

Заметьте, что мы удалили потребность в IFieldCleaner, потому что эти строковые очистители имеют дело только с входной строкой для очистки.

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

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

IRepository 
IFileHelper 
IFileWriter 
IFileParser 

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

Независимо от того, мы знаем, что нам нужно в конечном итоге получить строки из полей, может быть, IFileParser поможет?

interface IFileParser { 
    FileContents ReadContents(File file); 
    FileRow[] ReadRows(FileContents fc); 
    FileField ReadField(FileRow row, string column); 
} 

Это может быть более сложным, чем это необходимо be-- FileField может позаботиться о хранении значения поля так, вероятно, вы могли бы составить все эти вместе, чтобы сформировать FileContents сохраняться на диск.

Итак, теперь мы разделили наши конечные цели (чистые вещи), откуда поступают данные (файлы, база данных и т. Д.), И как мы это сохраняем (назад к файлам, базе данных и т. Д.).

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

class ExternalAddressStringCleaner : IStringCleaner { 
    // depend on whatever you need here 

    public string Clean(string s) { 
    // call external program 
    return cleanString; 
    } 
} 

Теперь вы переключаетесь на хранимую процедуру? Хорошо, нет проблем либо:

class DatabaseAddressStringCleaner : IStringCleaner { 

    // depend on database 
    DatabaseAddressStringCleaner(IRepository repository) { 
    } 

    string Clean(string s) { 
    // call your database sproc 
    return cleanString; 
    } 
} 

Это трудно рекомендовать идеи для вашего сервиса - но это возможно, вы можете разбить его на отдельные мелкие услуги (FileReaderService, FileCleaningService и FileStoreService) или упрощения зависимостей вы принимаете.

Теперь, когда у вас только один интерфейс IStringCleaner, вы можете просто объявить очистители, которые вам нужны, и поменять их/изменить их каждый раз.

public FileCleanerService { 
    private IStringCleaner _addressCleaner; 
    private IStringCleaner _phoneCleaner; 
    private IStringCleaner _obscenityCleaner; 
    private IStringCleaner _emailCleaner; 

    ctor(IFileParser parser, /* deps */) { 
    _parser = parser; 

    _addressCleaner = new ExternalAddressStringCleaner(/* deps */); 
    _phoneCleaner = new PhoneStringCleaner(); 
    _obscenityCleaner = new ObscenityStringCleaner(); 
    _emailCleaner = new EmailStringCleaner(); 
    } 

    public void Clean(FileContents fc) { 

    foreach(var row in _parser.ReadRows(fc)) { 
     var address = _parser.ReadField(row, "Address"); 
     var phone = _parser.ReadField(row, "Phone"); 
     var post = _parser.ReadField(row, "PostContent"); 
     var email = _parser.ReadField(row, "Email"); 

     // assumes you want to write back to the field? 
     // handle this however you want 
     address.Value = _addressCleaner.Clean(address.Value); 
     phone.Value = _phoneCleaner.Clean(phone.Value); 
     post.Value = _obscenityCleaner.Clean(post.Value); 
     email.Value = _emailCleaner.Clean(email.Value); 
    } 

} 

Я сделал много предположений о вашем процессе и коде, поэтому это, вероятно, намного сложнее, чем я предполагал. Не имея всей информации, трудно дать руководство, но все еще есть основные вещи, о которых вы можете рассуждать, просто взглянув на интерфейсы и имена, и я надеюсь, что продемонстрировал это. Иногда вам просто нужно смотреть сквозь поверхность, чтобы увидеть 1s и 0s позади матрицы, и тогда все это имеет смысл;)

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