Нужны некоторые предложения на моей архитектуре программного обеспечения. [Обзор кода]

Вот пара предложений, ничего важного, просто некоторые вещи, которые нужно учесть.

1. Я понимаю, что вы хотите, чтобы API был минимальным, тем самым сделав парсер и загрузчик частными / внутренними, но вы все равно можете рассмотреть возможность сделать их общедоступными. Самая большая причина в том, что поскольку это будет проект с открытым исходным кодом, у вас, скорее всего, появятся люди, которые, ну, хакеры. Если случайно они захотят сделать что-то, что напрямую не поддерживается предоставляемым вами API, они будут признательны, если вы предоставите им доступ к битам, чтобы сделать это самостоятельно. Сделайте «стандартные» варианты использования максимально простыми, но также сделайте так, чтобы людям было легко делать с ними все, что они хотят.

2. Похоже, есть некоторое дублирование данных между вашим классом Movie и вашим парсером. В частности, парсер получает поля, определенные вашим фильмом. Похоже, было бы разумнее сделать Movie объектом данных (только свойствами) и позволить классу Parser работать с ним напрямую. Таким образом, ваш синтаксический анализатор FromMovieTitle может возвращать фильм вместо синтаксического анализатора.Теперь возникает вопрос, что делать с методами класса Movie FindMovie и FindKnownMovie . Я бы сказал, что вы могли бы создать класс MovieFinder , в котором были бы эти методы, и они использовали бы синтаксический анализатор для возврата Movie.

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

В целом, если вы помните Принцип единой ответственности и Принцип открытости / закрытости вместе с вашей целью упрощения стандартного использования, вы должны в конечном итоге получить то, что люди найдет легким в использовании то, что вы думали поддерживать, и легким расширением для того, чего у вас еще не было.

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

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

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

// Data Contract (or Data Transfer Object)
public class Movie
{
        public Image Poster { get; set; }
        public string Title { get; set; }
        public DateTime ReleaseDate { get; set; }
        public string Rating { get; set; }
        public string Director { get; set; }
        public List<string> Writers { get; set; }
        public List<string> Genres { get; set; }
        public string Tagline { get; set; }
        public string Plot { get; set; }
        public List<string> Cast { get; set; }
        public string Runtime { get; set; }
        public string Country { get; set; }
        public string Language { get; set; }
}

// Movie database searching service contract
public interface IMovieSearchService    
{
        Movie FindMovie(string Title);
        Movie FindKnownMovie(string ID);
}

// Movie database searching service
public partial class MovieSearchService: IMovieSearchService
{
        public Movie FindMovie(string Title)
        {
            Movie film = new Movie();
            Parser parser = Parser.FromMovieTitle(Title);

            film.Poster = parser.Poster();
            film.Title = parser.Title();
            film.ReleaseDate = parser.ReleaseDate();
            //And so an so forth.
        }

        public Movie FindKnownMovie(string ID)
        {
            Movie film = new Movie();
            Parser parser = Parser.FromMovieID(ID);

            film.Poster = parser.Poster();
            film.Title = parser.Title();
            film.ReleaseDate = parser.ReleaseDate();
            //And so an so forth.
        }
}

Это может показаться тривиальным, однако отделение поведения от ваших данных может стать критическим по мере роста системы. Создавая интерфейс для службы поиска фильмов, вы обеспечиваете развязку и гибкость.Если вам по какой-либо причине необходимо добавить другой тип службы поиска фильмов, обеспечивающий такую ​​же функциональность, вы можете сделать это, не нарушая требований потребителей. Тип данных Movie можно использовать повторно, ваши клиенты привязываются к интерфейсу IMovieSearchService, а не к конкретному классу, что позволяет взаимозаменять реализации (или использовать несколько реализаций одновременно). Лучше всего поместить интерфейс IMovieSearchService и тип данных Movie в отдельные чем класс MovieSearchService.

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

public abstract class Parser
{
    public abstract IEnumerable<Movie> Parse(string criteria);
}

public class ByTitleParser: Parser
{
    public override IEnumerable<Movie> Parse(string title)
    {
        // TODO: Logic to parse movie information by title
        // Likely to return one movie most of the time, but some movies from different eras may have the same title
    }
}

public class ByActorParser: Parser
{
    public override IEnumerable<Movie> Parse(string actor)
    {
        // TODO: Logic to parse movie information by actor
        // This one can return more than one movie, as an actor may act in more than one movie
    }
}

public class ByIdParser: Parser
{
    public override IEnumerable<Movie> Parse(string id)
    {
        // TODO: Logic to parse movie information by id
        // This one should only ever return a set of one movie, since it is by a unique key
    }
}

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

public class ParserFactory
{
    public virtual Parser GetParser(string criteriaType)
    {
        if (criteriaType == "bytitle") return new ByTitleParser();
        else if (criteriaType == "byid") return new ByIdParser();
        else throw new ArgumentException("Unknown criteria type.", "criteriaType");
    }
}

// Improved movie database search service
public class MovieSearchService: IMovieSearchService
{
        public MovieSearchService(ParserFactory parserFactory)
        {
            m_parserFactory = parserFactory;
        }

        private readonly ParserFactory m_parserFactory;

        public Movie FindMovie(string Title)
        {
            var parser = m_parserFactory.GetParser("bytitle");
            var movies = parser.Parse(Title); // Parse method creates an enumerable set of Movies that matched "Title"

            var firstMatchingMovie = movies.FirstOrDefault();

            return firstMatchingMovie;
        }

        public Movie FindKnownMovie(string ID)
        {
            var parser = m_parserFactory.GetParser("byid");
            var movies = parser.Parse(Title); // Parse method creates an enumerable set of Movies that matched "ID"

            var firstMatchingMovie = movies.FirstOrDefault();

            return firstMatchingMovie;
        }
}

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

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

Также в вашем классе Movie я бы сделал информацию доступной только для получения, но не для настройки. в классе нет функции "сохранить", поэтому нет причин редактировать информацию, как только вы ее получите.

Кроме того, если это для других людей, я бы прокомментировал, для чего предназначены каждый класс, член и каждая переменная публичного / частного класса. Для класса Movie я бы, вероятно, включил пример того, как его использовать в комментарии к классу.

И последнее: если есть ошибка в двух частных классах, пользователь класса Movie должен быть каким-то образом проинформирован. Возможно, это общедоступная переменная типа bool с именем success?

В качестве личного предпочтения для класса Your Movie я бы сделал две ваши функции конструкторами, чтобы я мог просто построить класс следующим образом.

Фильм myMovie = новый Фильм ("Название"); или Фильм myMovie = новый фильм (1245);

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

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

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

public interface IMovieRepository
{
    Movie FindMovieById(string id);

    Movie FindMovieByTitle(string title);
}

Затем определите что-то, что отвечает за загрузку HTML-документов:

public interface IHtmlDownloader
{
    HtmlDocument DownloadHtml(Uri uri);
}

Затем определите реализацию репозитория, которая использует загрузчик:

public class MovieRepository : IMovieRepository
{
    private readonly IHtmlDownloader _downloader;

    public MovieRepository(IHtmlDownloader downloader)
    {
        _downloader = downloader;
    }

    public Movie FindMovieById(string id)
    {
        var idUri = ...build URI...;

        var html = _downloader.DownloadHtml(idUri);

        return ...parse ID HTML...;
    }

    public Movie FindMovieByTitle(string title)
    {
        var titleUri = ...build URI...;

        var html = _downloader.DownloadHtml(titleUri);

        return ...parse title HTML...;
    }
}

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

public class NeedsMovies
{
    private readonly IMovieRepository _movies;

    public NeedsMovies(IMovieRepository movies)
    {
        _movies = movies;
    }

    public void DoStuffWithMovie(string title)
    {
        var movie = _movies.FindMovieByTitle(title);

        ...
    }
}

Кроме того, теперь вы можете легко протестировать логику синтаксического анализа без необходимости выполнять веб-вызовы. Просто сохраните HTML и создайте загрузчик, который передаст его в репозиторий:

public class TitleHtmlDownloader : IHtmlDownloader
{
    public HtmlDocument DownloadHtml(Uri uri)
    {
        return ...create document from saved HTML...
    }
}

[Test]
public void ParseTitle()
{
    var movies = new MovieRepository(new TitleHtmlDownloader());

    var movie = movies.GetByTitle("The Matrix");

    Assert.AreEqual("The Matrix", movie.Title);

    ...assert other values from the HTML...
}