Нужны некоторые предложения по моей архитектуре программного обеспечения. [Обзор кода] - PullRequest
7 голосов
/ 12 июня 2010

Я делаю библиотеку C # с открытым исходным кодом для использования другими разработчиками.Моя ключевая проблема - простота использования .Это означает использование интуитивно понятных имен, интуитивное использование методов и тому подобное.

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

У меня есть три класса: Downloader, Parser и Movie

Я подумал, что было бы лучше, если бы только выставить класс Movie моей библиотеки и иметь Downloaderи Parser остаются скрытыми от вызова.

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

с использованием FreeIMDB;

public void Test()
{
    var MyMovie = Movie.FindMovie("The Matrix");
    //Now MyMovie would have all it's fields set and ready for the big show.
}

Можете ли вы просмотреть, как я планируюэто, и укажите на любые неправильные суждения, которые я сделал и где я мог бы улучшить.

Помните, моя главная проблема - простота использования.

Movie.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Drawing;


namespace FreeIMDB
{
    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; }

        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.
        }
    }
}

Parser.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using HtmlAgilityPack;

namespace FreeIMDB
{
    /// <summary>
    /// Provides a simple, and intuitive way for searching for movies and actors on IMDB.
    /// </summary>
    class Parser
    {
        private Downloader downloader = new Downloader();                
        private HtmlDocument Page;

        #region "Page Loader Events"
        private Parser()
        {

        }

        public static Parser FromMovieTitle(string MovieTitle)
        {
            var newParser = new Parser();
            newParser.Page = newParser.downloader.FindMovie(MovieTitle);
            return newParser;
        }

        public static Parser FromActorName(string ActorName)
        {
            var newParser = new Parser();
            newParser.Page = newParser.downloader.FindActor(ActorName);
            return newParser;
        }

        public static Parser FromMovieID(string MovieID)
        {
            var newParser = new Parser();
            newParser.Page = newParser.downloader.FindKnownMovie(MovieID);
            return newParser;
        }

        public static Parser FromActorID(string ActorID)
        {
            var newParser = new Parser();
            newParser.Page = newParser.downloader.FindKnownActor(ActorID);
            return newParser;
        }
        #endregion

        #region "Page Parsing Methods"
        public string Poster()
        {
            //Logic to scrape the Poster URL from the Page element of this.
            return null;
        }

        public string Title()
        {
            return null;
        }

        public DateTime ReleaseDate()
        {
            return null;
        }        
        #endregion        
    }
}

-----------------------------------------------

Как вы думаете, ребята, я иду по правильному пути, илиПозже я настраиваюсь на мир зла?

Моей первоначальной мыслью было разделить загрузку, анализ и фактическое заполнение, чтобы легко иметь расширяемую библиотеку.Представьте себе, если бы однажды веб-сайт изменил свой HTML, мне бы пришлось только изменить класс синтаксического анализа, не касаясь класса Downloader.cs или Movie.cs.

Спасибо за чтение и за помощь!

Есть еще идеи?

Ответы [ 4 ]

5 голосов
/ 12 июня 2010

Ваш 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...
}
1 голос
/ 12 июня 2010

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

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

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

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

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

0 голосов
/ 13 июня 2010

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

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

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

// 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.

0 голосов
/ 12 июня 2010

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

Также в вашем классе Кино я бы делал информацию только Getable, но не Setable. функциональность «сохранить» в классе отсутствует, поэтому нет смысла редактировать информацию после ее получения.

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

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

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

Movie myMovie = new Movie («Имя»); или же Movie myMovie = новый фильм (1245);

Добро пожаловать на сайт PullRequest, где вы можете задавать вопросы и получать ответы от других членов сообщества.
...