Является ли этот шаблон проектирования хранилища действительным и эффективным?

Я использую Dapper ORM в моем Уровне доступа к данным, но я думаю, что анализ кода ниже не зависит от какой-либо конкретной ORM. Каким должно быть одно общее место на всем уровне доступа к данным для инициализации строки соединения?

Базовый абстрактный класс

abstract class Repository
    {
        protected static IDbConnection _db = new SqlConnection(ConfigurationManager.ConnectionStrings["GuideDB"].ConnectionString);

      //should I use Constructor here
    }

Общий интерфейс репозитория

public interface IRepository<T>
    {
        IEnumerable<T> GetAll();
        T GetItem(int id);
        T Create(T item);
        bool Update(T item);
        bool Delete(int id);
        void Reset();
    }

Класс бетона

public class StateRepository :Repository, IRepository<State> 
    {

        private static ICollection<State> _states = PopulateStates();// doing this valid ??
        private static object _lock = new object();

        #region Public Methods

        public IEnumerable<State> GetAll()
        {
            lock (_lock)
            {
                return _states.ToArray();
            }
        }

        public IEnumerable<State> GetAllStatesByCountryId(short id)
        {
            lock (_lock)
            {
                return _states.Where(s => s.CountryId == id).ToArray();
            }
        }

        public State GetItem(int id)
        {
            lock (_lock)
            {
                return _states.FirstOrDefault(s => s.Id == id);
            }
        }

        public State Create(State state)
        {
            lock (_lock)
            {
                if (state == null)
                {
                    throw new ArgumentNullException("state");
                }

                state.Id = (short)(_states.Any() ? _states.Max(s => s.Id) + 1 : 1);
                _states.Add(state);
                return state;
            }
        }

        public bool Update(State item)
        {
            lock (_lock)
            {
                var state = _states.FirstOrDefault(s => s.Id == item.Id);

                if (state != null)
                {
                    state.Id = item.Id;
                    state.Name = item.Name;
                    state.ShortInfo = item.ShortInfo;
                    state.CountryId = item.CountryId;

                    return true;
                }

                return false;
            }
        }

        public bool Delete(int id)
        {
            lock (_lock)
            {
                var state = _states.FirstOrDefault(s => s.Id == id);
                return _states.Remove(state);
            }
        }

        public void Reset()
        {
            lock (_lock)
            {
                _states = PopulateStates(); // doing this valid ??
            }
        }
        #endregion

        private static ICollection<State> PopulateStates()
        {
            return  _db.Query<State>("SELECT * FROM States").ToList(); //Dapper ORM Code
        }

       //How to Add a efficient Dispose method to this repository , is it really needed
    }
11 голосов | спросил Abhi 26 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowFri, 26 Sep 2014 13:29:59 +0400 2014, 13:29:59

5 ответов


9

Я думаю, ваш абстрактный класс должен «реализовать» интерфейс IRepository, потому что в противном случае вам всегда нужно будет поместить как интерфейс, так и базовый класс в вашем списке наследования. Кроме того, я думаю, вы должны назвать свой репозиторий -> SqlRepository, поскольку весь код в базовом классе связан с Sql. Как отметил @Heslacher, вы должны ввести ваше соединение, потому что сохранение статического соединения может вызвать проблемы. Что произойдет, если одна из моих Repository будет удалять соединение? Все ваши репозитории будут недоступны.

abstract class SqlRepository<T> : IRepository<T>
{
    protected IDbConnection Connection { get; set; }

    protected SqlRepository<T>(string connectionString)
    {
        Connection = new SqlConnection(connectionString);
    }

    abstract IEnumerable<T> GetAll();
    abstract T GetItem(int id);
    abstract T Create(T item);
    abstract bool Update(T item);
    abstract bool Delete(int id);
    abstract void Reset();
}

Если ваше приложение работает в одном экземпляре, ваш репозиторий будет в порядке. Но поскольку он помещает все сущности базы данных в кеш (вашу коллекцию) и никогда не возвращается в базу данных, если вы не укажете ее, у вас возникнет проблема. Пример: клиент A (на машине X) загружает базу данных в кеш, клиент B (на машине Y) делает то же самое, затем добавляет State в репозиторий. Клиент A вызывает GetAll, каков должен быть ожидаемый результат? У клиента A не будет нового State. И если клиент B вызывает Reset, новое состояние никогда не сохранялось, поэтому оно потеряно.

Я не думаю, что вы должны использовать ICollection<State>, каждый ваш звонок должен идти в базу данных, поэтому вы уверены, что иметь последнюю версию своих данных. Если вы удалите его, я не думаю, что вам нужно больше обращаться с безопасностью потоков.

ответил IEatBagels 26 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowFri, 26 Sep 2014 16:39:17 +0400 2014, 16:39:17
6

Чтобы ваши репозитории не зависели от СУБД, вы должны ввести параметр IDbConnection в свой конструктор.

IRepository<T>

Вы должны либо переименовать GetItem() в Get() или переименовать другие методы в XxItem(), чтобы поддерживать имена методов согласованными.

StateRepository

Я бы переименовал входной параметр метода Create() из state to (как в другом методе) item. Таким образом, более очевидно, что это метод интерфейса.

В противном случае ваш код выглядит хорошо для меня.

ответил Heslacher 26 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowFri, 26 Sep 2014 13:55:15 +0400 2014, 13:55:15
5
private static ICollection<State> PopulateStates()
{
    return  _db.Query<State>("SELECT * FROM States").ToList(); //Dapper ORM Code
}

Отметив проблему, TopinFrassi рассказал о том, следует ли вообще кэшировать всю таблицу, каждый из ваших методов public возвращает либо a State, bool или объект IEnumberable<State>. Нет причин использовать ToList здесь.

Просто сохраните свой кеш в виде IEnumerable<State> и перестаньте звонить ToArray в ваших методах GetAll*.

state.Id = item.Id;
state.Name = item.Name;
state.ShortInfo = item.ShortInfo;
state.CountryId = item.CountryId;

Предполагая, что CountryId является внешним ключом для чего-то вроде Countries, я бы рекомендовал включить public Country Country { get; set; } в ваш State. Затем используйте многокадровый запрос Dapper, чтобы получить оба одновременно:

string sql = @"SELECT s.Id, s.Name, s.ShortInfo, s.CountryId,
                      c.Id, c.Name -- etc. for Country table
               FROM States s
               JOIN Countries c ON s.CountryId = c.Id";
return _db.Query<State, Country, State>(sql, (state, country) =>
    {
        state.Country = country;
        return state;
    });

Обратите внимание, что если первый столбец, который вы выбираете для каждого объекта после первого, не называется «Id», вам нужно указать splitOn в метод Query. splitOn - это список имен столбцов с разделителями-запятыми. Если первый столбец для каждого объекта после первого (первый объект не имеет значения, поскольку он очевиден там, где он начинается!) Называется «Id», splitOn не требуется.

Включая свойство Country в вашем State упростит ссылку на любую страну, в которой находится каждое государство. Вместо того, чтобы снова ударять базу данных, вы просто получаете страну в одно и то же время. Даже если вы решили остановить кеширование своих состояний, вы можете использовать эту стратегию для своего метода GetItem.

string sql = @"SELECT s.Id, s.Name, s.ShortInfo, s.CountryId,
                      c.Id, c.Name -- etc. for Country table
               FROM States s
               JOIN Countries c ON s.CountryId = c.Id
               WHERE s.Id = @Id";
return _db.Query<State, Country, State>(sql, (state, country) =>
    {
        state.Country = country;
        return state;
    }, new { Id = id }).SingleOrDefault();

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

В моем втором примере вы также увидите, что я использую SingleOrDefault вместо FirstOrDefault. Я делаю здесь предположение, что Id является первичным ключом (или потенциально уникальным индексом), поэтому набор результатов SQL должен never создает несколько записей. (Если это предположение неверно, конечно, вы должны придерживаться FirstOrDefault.)

ответил Brian S 26 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowFri, 26 Sep 2014 19:29:03 +0400 2014, 19:29:03
5

Этот код ужасен. Никогда больше не пишите этот код.

Этот «шаблон» - это, безусловно, самый повторный отказ от дизайна, который я видел. Я также буду первым признаться , в прошлом я использовал ТОЧНЫЙ тот же код , когда я был младшим разработчиком.

Что принципиально неверно, это abstract class Repository. Этот код принудительно наследует без причины. Вы не собираетесь действовать полиморфно через репозитории, такие как

List<Repository> Repositories = new List<Repository> { ...... };
Repositories.ForEach(x=> x.Create(foo));

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

Почему abstract class Repository SO BAD?

Возьмите свой пример StateRepository. Государства очень тесно переплетаются с городами, зипкодами и округами. Предположим, вы хотите иметь GeoRepository, который предоставляет вам город, графство и штат Zipcode? Как вы собираетесь достичь этого с помощью этого кода? C # не поддерживает множественное наследование, поэтому вы не можете иметь GeoRepository : ZipCodeRepository, StateRepository, CityRepository

Итак, следующая попытка будет выглядеть примерно так:

GeoRepository : ZipCodeRepository : StateRepository : CityRepository

Даже упоминание ZipCodeRepository : StateRepository : CityRepository показывает абсурдность этого «шаблона». ZipCodes не наследуют от штата или города, но это тип отношений, который будет принудительно abstract class Repository. Кроме того, это просто не сработает, потому что вы будете переписывать одни и те же методы. Предположим, что код использовал Repository<T>, тогда, по крайней мере, компилятор разрешит эту атакующую цепочку наследования. Это все равно будет так же плохо.

Вы упоминаете Dapper.

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

Чтобы отвлечься от Dapper, вы предоставляете себе самый низкий общий знаменатель вместо полной функциональности Dapper.

ответил Chris Marisic 26 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowFri, 26 Sep 2014 23:17:43 +0400 2014, 23:17:43
1

Если я вызываю Update на State, который не существует, я ожидал бы, что будет выбрано исключение, а не просто вернуть false. Вот как я его напишу:

    public void Update(State item)
    {
        lock (_lock)
        {
            var state = _states.Single(s => s.Id == item.Id);

            state.Id = item.Id;
            state.Name = item.Name;
            state.ShortInfo = item.ShortInfo;
            state.CountryId = item.CountryId;
        }
    }
ответил Gabe 26 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowFri, 26 Sep 2014 17:29:43 +0400 2014, 17:29:43

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

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

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