У меня проблема:

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

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

  • Задача: содержит свойства, которые должны быть сгенерированы.
  • Конфигурация: это параметры диапазона или условия для возникновения проблемы.
  • Фабрика: он отвечает за создание новой проблемы.

 Конфигурация -> ProblemFactory -> Проблема

Здесь у меня есть интерфейс и интерфейсные интерфейсы:

public abstract class Problem { }
public abstract class Configuration { }

public interface IProblemFactory
{
    Configuration Configuration { get; set; }
    Problem CreateProblem();
}

Это базовый класс для фабрик, потому что мне нужен класс Random. Все мои классы, которые реализуют этот, должны иметь одно и то же семя, поэтому у меня есть статический экземпляр.

public abstract class ProblemFactoryBase<P, C> : IProblemFactory
    where P : Problem
    where C : Configuration
{
    private const int DEFAULT_SEED = 100;
    protected C _config;
    private static Random _random;

    public ProblemFactoryBase()
    {
        if (_random == null) _random = new Random(DEFAULT_SEED);
    }

    public ProblemFactoryBase(C config)
    {
        _config = config;

        if (_random == null) _random = new Random(DEFAULT_SEED);
    }

    protected Random Random { get { return _random; } }

    public C Configuration
    {
        get { return _config; }
        set { _config = value; }
    }

    #region IProblemFactory Implementation

    Configuration IProblemFactory.Configuration
    {
        get { return _config; }
        set
        {
            C config = value as C;
            if (config == null) throw new InvalidCastException("config");

            _config = config;
        }
    }

    protected abstract P CreateProblem(C config);

    #endregion

    public Problem CreateProblem()
    {
        if (_config == null) throw new InvalidOperationException("config");
        return CreateProblem(_config);
    }

    public static void SetSeed()
    {
        _random = new Random(DEFAULT_SEED);
    }
}

Обратите внимание, что класс ProblemFactoryBase<P, C> является общим типом и реализует абстрактный метод при построении типа.

Когда у меня есть реализация всего этого. Например, для модуля BinaryProblems, такого как 2+3, будет:

public class BinaryConfiguration : Configuration
{
    public Range<int> Range1 { get; set; }
    public Range<int> Range2 { get; set; }
    public List<Operators> Operators { get; set; }

    public BinaryConfiguration(Range<int> range1, Range<int> range2, List<Operators> operators)
    {
        this.Range1 = range1;
        this.Range2 = range2;
        this.Operators = operators;
    }

public class BinaryProblem : Problem
{
    public BinaryProblem(decimal x, decimal y, Operators op, decimal response)
    {
        this.X = x;
        this.Y = y;
        this.Response = response;
    }

    public decimal X { get; private set; }
    public decimal Y { get; private set; }
    public decimal Response { get; private set; }
}

public enum Operators
{
    Addition, Substract, Multiplication, Division
}

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

 public class BinaryFactory : ProblemFactoryBase<BinaryProblem, BinaryConfiguration>
{
    protected override BinaryProblem CreateProblem(BinaryConfiguration config)
    {
        var x = GenerateValueInRange(config.Range1);
        var y = GenerateValueInRange(config.Range2);

        var index = Random.Next(config.Operators.Count);
        var op = config.Operators[index];

        return new BinaryProblem(x, y, op, x + y);
    }

    private decimal GenerateValueInRange(Range<int> range)
    {
        return Random.Next(range.Min, range.Max);
    }
}

И реализовать это:

        BinaryConfiguration configuration = new BinaryConfiguration() {.. }
        IProblemFactory factory = new BinaryFactory(configuration);
        var a = factory.CreateProblem();

Он делает то, что мне нравится, потому что я хочу поместить несколько заводов в массив, но в то же время реализация ProblemFactoryBase хороша для меня, потому что нет необходимости в создании типов литья.

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

29 голосов | спросил Darf Zon 31 MarpmSat, 31 Mar 2012 18:42:14 +04002012-03-31T18:42:14+04:0006 2012, 18:42:14

5 ответов


13

Этот дублированный код

public ProblemFactoryBase()
{
    if (_random == null) _random = new Random(DEFAULT_SEED);
}

public ProblemFactoryBase(C config)
{
    _config = config;

    if (_random == null) _random = new Random(DEFAULT_SEED);
}  

можно удалить, используя цепочку конструкторов и быть преувеличенным, используя фигурные скобки {}, например,

public ProblemFactoryBase()
{
    if (_random == null) { _random = new Random(DEFAULT_SEED); }
}

public ProblemFactoryBase(C config)
    : this()
{
    _config = config;
}  

Поскольку константа уровня DEFAULT_SEED используется только с static Random _random, она должна быть static readonly. Кроме того, на основе принципов именования его следует называть с помощью оболочки PascalCase, см. Также: https: //stackoverflow.com/a/242549/2655508 он должен выглядеть так

private static readonly int DefaultSeed = 100;  

Потому что Random _random и свойство Random Random (которое не должно быть названо так, потому что вы не должны называть свойство, подобное его типу) не будет быть изменено, кроме как в конструкторе, почему бы вам не сделать Random _random a protected readonly вместо этого?

Хорошо, теперь я заметил это

public static void SetSeed()
{
    _random = new Random(DEFAULT_SEED);
}  

, который является ошибочным IMO, по крайней мере, методname подразумевает нечто иное, чем это сделано. Либо измените имя этого метода, либо пусть метод имеет аргумент метода, который затем используется как семя.


О #region IProblemFactory Implementation прочитайте Обладают ли запасы антипаттера или кода?

Если вы хотите сохранить этот регион, вы должны по крайней мере включить метод Problem CreateProblem(). Кстати, почему у вас неявные и явные реализации интерфейса ????


Это

public C Configuration
{
    get { return _config; }
    set { _config = value; }
}

Configuration IProblemFactory.Configuration
{
    get { return _config; }
    set
    {
        C config = value as C;
        if (config == null) throw new InvalidCastException("config");

        _config = config;
    }
}

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

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

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


Либо реализация конкретного BinaryFactory, либо его использование в нем ошибочна и не будет компилироваться, потому что в примере использования используется конструктор, который BinaryFactory не выполняет предоставлять.

ответил Heslacher 14 MonEurope/Moscow2015-12-14T10:27:10+03:00Europe/Moscow12bEurope/MoscowMon, 14 Dec 2015 10:27:10 +0300 2015, 10:27:10
11

При передаче параметров в public метод всегда нужно утверждать, что параметр parameter не null, иначе ваш код будет бросать NullReferenceException, и им никто не нравится.

Типичные типы C и P не должны называться таким образом. «Неписанное» соглашение состоит в том, что буквой по умолчанию является T. Это не значит, что мы должны использовать любые буквы! :) Если мы проверим именование структуры .Net, мы должны переименовать имя параметра your в TProblem, TConfig.

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

В классе ProblemFactoryBase вы дублируете код. Существует метод SetSeed, который содержит тот же код, который вызывается в вашем конструкторе. Вы должны вызвать SetSeed в конструкторе вместо вызова того же кода.

Кстати, действительно ли этот метод SetSeed полезен? Переменная _random является private, поэтому никто другой, кроме вас, не может решить установить ее в другое значение. Но вы не меняете семя _random, поэтому зачем выставлять для него метод public public static? Я не думаю, что вам нужен этот метод. И вместо того, чтобы подвергать protected Random, почему бы вам не выставить метод GetRandomNumber(), который вернет _random.Next(). Думаю, это было бы яснее. Кроме того, уверены ли вы, что Random не является деталью реализации? Почему Random должен быть в вашем базовом классе? Что делать, если ребенок не хочет использовать Random? Я думаю, что Random должен быть исключен из базового класса, ребенок будет создавать свои собственные, если им это нужно.

Вам действительно нужно это свойство:

public C Configuration
{
    get { return _config; }
    set { _config = value; }
}

Уже существует реализация interface. Это может привести к некоторой путанице. Я почти уверен, что выставить только реализацию interface будет достаточно! Если вы беспокоитесь о необходимости придать свой Configuration и не зная тип, мне нужно спросить вас: когда вам нужно будет отбросить Configuration извне ProblemFactoryBase (который знает тип Configuration, благодаря TConfig)?

Сообщение InvalidOperationException может быть более четким. В конце концов, объясняя, что "the configuration must not be null" не так много длинного сообщения, и это намного яснее, чем "config".

Я не уверен, что вы используете лучший шаблон дизайна для решения своей проблемы. В конце концов, Problem и Configuration ничего не разделяют. Problem зависит от Configuration. Поскольку Configuration не создается на заводе-изготовителе, мы начнем с удаления его здесь. В конце концов, зачем ProblemFactoryBase нужен параметр Configuration в качестве параметра?

Аргумент №1: Если класс Problem или CreateProblem не нужен класс Configuration, это означает Configuration - это деталь реализации. Интерфейсы /абстрактные классы не должны показывать детали реализации. Чтобы показать свою точку зрения, вот пример. Представьте, что в один прекрасный день у вас есть два дочерних класса ProblemFactoryBase. Один, который использует Configuration и тот, который использует IFooBarAlgorithmService. Вы добавите IFooBarAlgorithmService в свой ProblemFactoryBase?Не могли бы вы добавить такие параметры для всех дочерних классов? Класс ProblemFactoryBase оказался бы бесполезным.

Аргумент № 2: Если Configuration является свойством, потому что вы не хотите, чтобы он был параметром метода CreateProblem, что произойдет, если вы создали проблему который не нуждался в Configuration? Метод CreateProblem выбрал бы InvalidOperationException. Но действительно ли это недействительно? Нет. Класс child не хотел использовать Configuration и не должен использовать его, поскольку он не является параметром метода CreateProblem. . р>

Итак, теперь, когда вы видите, почему свойство Configuration не является лучшим шагом, давайте посмотрим на решения:

Либо вы полностью удаляете свойство Configuration из класса ProblemFactoryBase, так как это деталь реализации; или вы добавляете Configuration в качестве параметра общедоступного метода CreateProblem.

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

Прямо сейчас, я думаю Configuration должен быть параметром вашего метода Configuration, давайте проверим все это. Сначала мы создаем интерфейс CreateProblem:

(Примечание: я думаю, что interface будет лучшим именем, чем просто ProblemConfiguration), это менее запутанно! И для следующих примеров я применим обзор, который я сделал выше )

Configuration

Этот public interface IProblemFactory<TProblem, TConfig> where TProblem : IProblem where TConfig : IProblemConfiguration { TProblem CreateProblem(TConfig configuration); } довольно прост. В нем говорится: «Все, что меня реализует, должно быть создать проблему с помощью конфигурации ". Это именно то, чего мы хотим. Всегда старайтесь поддерживать свои интерфейсы как можно ясными и краткими! (Ваш предыдущий интерфейс был прекрасен, это просто подсказка!)

Далее, как выглядит реализация?

interface

Это чистое и похоже на ваш предыдущий класс, но обратите внимание, что между public class BinaryProblemFactory : IProblemFactory<BinaryProblem, BinaryConfiguration> { private static readonly Random Random = new Random(); public BinaryProblem CreateProblem(BinaryConfiguration configuration) { if (configuration == null) throw new ArgumentNullException(nameof(configuration)); var x = GenerateValueInRange(configuration.Range1); var y = GenerateValueInRange(configuration.Range2); var index = Random.Next(configuration.Operators.Count); var op = configuration.Operators[index]; return new BinaryProblem(x, y, op, x + y); } private static decimal GenerateValueInRange(Range<int> range) { return Random.Next(range.Min, range.Max); } } и этим interface нет базового класса, и он не создавал гораздо больше code (Мы используем переменную class, которая могла быть использована в другом подклассе, или нет!)

Теперь у вас будет несколько подкласс класса Random. Было бы здорово иметь одну входную дверь, где мы могли бы дать конфигурацию и получить соответствующую проблему? О да, это было бы аккуратно. Именно здесь приходит Абстрактная фабрика .

Считайте его классом-оболочкой вокруг all ваших «под» заводов.

Что круто в том, что он может реализовать тот же существующий интерфейс!

IProblemFactory

Бум, у вас теперь есть общее место, чтобы получить все ваши заводы. Вы должны будете добавить много public class ProblemFactory : IProblemFactory<IProblem, IProblemConfiguration> { public IProblem CreateProblem(IProblemConfiguration configuration) { if (configuration == null) throw new ArgumentNullException(nameof(configuration)); if (configuration.GetType() == typeof (BinaryConfiguration)) { return new BinaryProblemFactory().CreateProblem((BinaryConfiguration)configuration); } //else if(configuration.GetType() == typeof(SomeOtherConfig) // return new FooProblemFactory().CreateProblem((FooConfiguration)configuration); throw new InvalidOperationException("The configuration type isn't mapped to a problem"); } } , хотя, по прошествии времени. Мы могли бы исправить это, изменив наш интерфейс, что изменило бы некоторые вещи в структуре нашей программы, но это решение также имеет недостатки. Я покажу вам это, так что вы можете решить, что лучше подходит вам.

Итак, если наш if/else был:

(Примечание. При наборе текста я понял, что interface должен быть назван CreateProblem. В конце концов, вы уже знаете, что собираетесь создать проблему, так как тип возврата Create)

IProblem

Недостаток происходит в ваших дочерних классах:

public interface IProblemFactory
{
    IProblem Create(IProblemConfiguration configuration);
}

Но мы получаем одно преимущество в вашем public class BinaryProblemFactory : IProblemFactory { private static readonly Random Random = new Random(); private static decimal GenerateValueInRange(Range<int> range) { return Random.Next(range.Min, range.Max); } public IProblem Create(IProblemConfiguration configuration) { if (configuration == null) throw new ArgumentNullException(nameof(configuration)); //Here, we need to cast and verify for null, that's the drawback. //To "counter" it, the parameter xml comments should state we await a BinaryConfiguration var binaryConfig = configuration as BinaryConfiguration; if (binaryConfig == null) throw new ArgumentException($"{nameof(binaryConfig)} should be of type {nameof(BinaryConfiguration)}"); var x = GenerateValueInRange(binaryConfig.Range1); var y = GenerateValueInRange(binaryConfig.Range2); var index = Random.Next(binaryConfig.Operators.Count); var op = binaryConfig.Operators[index]; return new BinaryProblem(x, y, op, x + y); } } :

ProblemFactory

Теперь мы можем использовать public class ProblemFactory : IProblemFactory { private readonly Dictionary<Type, IProblemFactory> _factoryMap; public ProblemFactory() { _factoryMap = new Dictionary<Type, IProblemFactory>(); _factoryMap.Add(typeof (BinaryConfiguration), new BinaryProblemFactory()); } public IProblem Create(IProblemConfiguration configuration) { if (configuration == null) throw new ArgumentNullException(nameof(configuration)); IProblemFactory factory; bool exists = _factoryMap.TryGetValue(configuration.GetType(), out factory); if (!exists) { throw new InvalidOperationException("The configuration type isn't mapped to a problem"); } return factory.Create(configuration); } } вместо цепочки Dictionary.

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

if

Вам решать, какое решение лучше всего между первым и вторым, поскольку оба они имеют преимущества и недостатки, но оба они должны быть довольно прочными! :)

ответил IEatBagels 14 MonEurope/Moscow2015-12-14T17:56:32+03:00Europe/Moscow12bEurope/MoscowMon, 14 Dec 2015 17:56:32 +0300 2015, 17:56:32
6

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

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

Проверьте это:

http://www.dofactory.com/Patterns/PatternInterpreter.aspx#_self2

ответил Alex 19 PMpThu, 19 Apr 2012 23:02:57 +040002Thursday 2012, 23:02:57
5

Незначительное дополнение к уже сделанным комментариям:

private decimal GenerateValueInRange(Range<int> range)
{
    return Random.Next(range.Min, range.Max);
}
  1. Почему это смешение decimal и int? Мне кажется, что если генератор создает проблемы с использованием ints, он не должен использовать десятичные знаки где угодно; и если он порождает проблемы с десятичными знаками, было бы более понятным использовать Range<decimal> для границ.

  2. Почему это частный метод в генераторе? Я могу сделать аргумент для включения его в Random (или обертку вокруг Random); Я могу сделать аргумент для включения его в Range<T>, если вы можете найти способ сделать это, который не сталкивается с проблемами с системой типов; и я могу сделать аргумент для включения его в качестве метода расширения для Range<int>. Но если это частный метод в генераторе, вы в конечном итоге скопируете его повсюду.

ответил Peter Taylor 15 TueEurope/Moscow2015-12-15T14:25:49+03:00Europe/Moscow12bEurope/MoscowTue, 15 Dec 2015 14:25:49 +0300 2015, 14:25:49
5

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

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

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

ответил RobH 18 FriEurope/Moscow2015-12-18T14:07:35+03:00Europe/Moscow12bEurope/MoscowFri, 18 Dec 2015 14:07:35 +0300 2015, 14:07:35

Похожие вопросы

Популярные теги

security × 330linux × 316macos × 2827 × 268performance × 244command-line × 241sql-server × 235joomla-3.x × 222java × 189c++ × 186windows × 180cisco × 168bash × 158c# × 142gmail × 139arduino-uno × 139javascript × 134ssh × 133seo × 132mysql × 132