Refactoring, um doppelten Code zu vermeiden

Question

Ich versuche, einige repetitive Code herauszufiltern, aber es fängt jetzt an, funky zu riechen. sagen, dass ich mit diesem nicht ganz richtig beginnen, aber Sie meine Drift zu fangen:

public virtual OrganisationEntity Get(int id) 
    { 
     SqlCommand command = new SqlCommand(); 
     command.CommandText = @"SELECT t.Id, t.Description FROM Organisation t Where t.Id = @Id"; 
     command.Parameters.Add("@id", SqlDbType.Int).Value = id; 

     List<OrganisationEntity> entities = new List<OrganisationEntity>(); 

     SqlDataReader reader = Database.ExecuteQuery(command, ConnectionName.Dev); 

     while (reader.Read()) 
     { 
      OrganisationEntityMapper mapper = Mapper; 
      entities = mapper.MapAll(reader); 
     } 

     return entities.First<OrganisationEntity>(); 
    } 

Es ist ziemlich offensichtlich jede andere Get (int id) Methode hat, abgesehen von der Abfrage die gleiche Form, so dass mein nächster Schritt würde sein, um eine Basisklasse zu machen, sucht RepositoryBase wie:

public abstract class RepositoryBase<T> where T : new() 
{ 
    /// <summary> 
    /// 
    /// </summary> 
    public abstract EntityMapperBase<T> Mapper { get; } 

    public virtual T Get(int id) 
    { 

     List<T> entities = new List<T>(); 

     SqlDataReader reader = Database.ExecuteQuery(Command, ConnectionName); 

     while (reader.Read()) 
     { 
      EntityMapperBase<T> mapper = Mapper; 
      entities = mapper.MapAll(reader); 
     } 

     return entities.First<T>(); 
    } 
} 

einige generischen Funkyness hinzuzufügen, aber das ist auch, wo es hässlich wird. Zuerst erwartet Database.ExecuteQuery einen SqlCommand und eine Enum, also dachte ich, ok, dann füge ich zwei Eigenschaften hinzu, die ich einfach mit ein paar Sachen starten werde. THe merke ich brauche den int id Parameter hier nicht mehr, da ich die Abfrage in einer Unterklasse konstruiere, also könnte ich genauso gut den Befehl und connectionName als Parameter übergeben, ich möchte den connectionName trotzdem vom OrganisationRepository abhängig machen (andere brauchen eine andere Zeichenfolge):

public class OrganisationRepository : RepositoryBase<OrganisationEntity> 
{ 
    protected override EntityMapperBase<OrganisationEntity> Mapper 
    { 
     get 
     { 
      return new OrganisationMapper(); 
     }    
    } 

    public override OrganisationEntity Get(int id) 
    { 
     SqlCommand command = new SqlCommand(); 
     command.CommandText = @"SELECT t.Id, t.Description FROM Organisation t Where t.Id = @Id"; 
     command.Parameters.Add("@id", SqlDbType.Int).Value = id; 
     return base.Get(command, ConnectionName.Dev); 
    } 
} 

Aber oops, natürlich, jetzt die Methodensignaturen sind nicht mehr synchron ... oops! Also, im Grunde frage ich mich. Es fühlt sich einfach unangenehm an, weiß aber nicht genau warum. Auf der einen Seite möchte ich Repetitive-Code so viel wie möglich herausfiltern, aber jetzt lässt es mich damit!

Wie reformiere ich dies zu (mehr) richtigen OO? Sollte ich einfach vergessen, die Abfragezeichenfolgen herauszufiltern und viele Duplikate zu schreiben?

Answer 1

Ihr "nächster Schritt" wäre nicht derselbe wie meiner.

Mein nächster Schritt wäre, ein weiteres Beispiel für diesen "gemeinsamen Code" zu finden, den Sie zu refaktorisieren versuchen. Vielleicht eine Methode "CustomerEntity.Get (int id)".

Nun stellen wir uns vor, der einzige Unterschied zwischen den Versionen CustomerEntity und OrganisationEntity sind die Abfragezeichenfolge und die Ersetzung des Begriffs "Organisation" durch "Kunde". Mein nächster Schritt wäre, zu versuchen, die beiden Methoden mehr und mehr identisch zu machen. Angenommen, diese Methode ist Teil einer OrganizationEntityRepository-Klasse, würde ich diese zu einer EntityRepository1-Klasse und das CustomerEntityRepository zu EntityRepository2 umgestalten.

Schritt 1 wäre, einen generischen Parameter für den Typ der Entität einzuführen. Sie müssen das Gleiche für die Klassen OrganizationEntityMapper und CustomerEntityMapper tun.

Als nächstes gehen Sie zurück und schauen Sie, was noch anders ist. Ich sehe, dass sie verschiedene Mapper-Klassen verwenden, also lassen Sie den Mappertyp generisch werden. Um dies zu tun und immer noch auf die MapAll-Methode zu verweisen, führe ich eine IMapper-Schnittstelle mit der MapAll-Methode ein und lasse dies von meinen zwei konkreten Mapper-Klassen implementieren.

Jetzt ist der nächste große Unterschied die Abfrage. Ich werde das in eine virtuelle "CommandText" -Eigenschaft einfügen.

Jetzt denke ich bin ich bereit für eine Basisklasse, vielleicht EntityRepositoryBase<TEntity,TMapper>.Mit geeigneten Annahmen, winde ich mit der Follow-up:

public abstract class EntityRepositoryBase<TEntity, TMapper> 
    where TMapper : IMapper<TEntity> 
{ 
    public virtual TEntity Get(int id) 
    { 
     List<TEntity> entities; 
     using (var command = new SqlCommand {CommandText = CommandText}) 
     { 
      command.Parameters.Add("@id", SqlDbType.Int).Value = id; 

      entities = new List<TEntity>(); 

      using (var reader = Database.ExecuteQuery(command, ConnectionName.Dev)) 
      { 
       while (reader.Read()) 
       { 
        var mapper = Mapper; 
        entities = mapper.MapAll(reader); 
       } 
      } 
     } 

     return entities.First(); 
    } 

    protected abstract string CommandText { get; } 
    protected abstract TMapper Mapper { get; } 
} 

public class OrganisationEntityRepository : 
    EntityRepositoryBase<OrganisationEntity, OrganisationEntityMapper<OrganisationEntity>> 
{ 
    protected override string CommandText 
    { 
     get { return @"SELECT t.Id, t.Description FROM Organisation t Where t.Id = @Id"; } 
    } 

    protected override OrganisationEntityMapper<OrganisationEntity> Mapper 
    { 
     get { throw new NotImplementedException(); } 
    } 
} 

public class CustomerEntityRepository : EntityRepositoryBase<CustomerEntity, CustomerEntityMapper<CustomerEntity>> 
{ 
    protected override string CommandText 
    { 
     get { return @"SELECT t.Id, t.Description FROM Customer t Where t.Id = @Id"; } 
    } 

    protected override CustomerEntityMapper<CustomerEntity> Mapper 
    { 
     get { throw new NotImplementedException(); } 
    } 
} 

---

Und unnötig zu sagen, obwohl ich es trotzdem sagen würde: Requisiten zu JetBrains ReSharper 5.1 zu tun alle bewegen Dingen, so Ich musste nicht.

Answer 2

Ich glaube, ORMs wie Entity Framework oder nHibernate wäre geeignetere Lösung für diese Situation. Sie können sich um alle Rohrleitungen kümmern, die Sie selbst bauen möchten.

Answer 3

Wenn Sie ein ORM-Framework in Ihrem Projekt verwenden können, denke ich, dass dies vorzuziehen wäre, anstatt alles manuell zu schreiben.

jedoch eine Möglichkeit, Sie dieses Refactoring tun können, ist eine abstrakte Methode zu haben, in RepositoryBase diese Signatur mit:

public abstract T Get(int id); 

und auch eine geschützte Methode mit dieser Signatur:

protected T Get(SqlCommand command, SqlConnection connection) 

, das hat der gleiche Code, den Sie zuvor in RepositoryBase gezeigt haben.

Auf diese Weise müssen Sie in abgeleiteten Klassen nur die implementieren, in der Sie den Befehl erstellen, und den Befehl aus der Basisklasse aufrufen, die den eigentlichen Datenbankaufruf ausführt.

Answer 4

Ich würde mit dem folgenden beginnen. Definieren Sie die Datenbank- und Mapping-Funktionalität in separaten Schnittstellen und fügen Sie sie in das Repository ein. Auf diese Weise wird das Repository einfacher zu testen sein. Mit dieser Methode sollten Sie in der Lage sein, das Repository um andere CRUD-Operationen zu erweitern.

Ein Problem mit diesem Ansatz ist die Trennung zwischen dem Mapper und der Erstellung des SqlCommand, es mag nicht sehr offensichtlich sein, welche Spalten über die select-Anweisung zurückgegeben werden.

// The concrete implementation of this interface will handle connections to the 
// database 
public interface IDatabase 
{ 
    SqlDataReader ExecuteQuery(SqlCommand command); 
} 

public interface IEntityMapper<T> 
{ 
    T MapAll(SqlDataReader reader); 
} 

public abstract class EntityRepository<T> 
{ 
    private readonly IDatabase _database; 
    private readonly IEntityMapper<T> _mapper; 

    protected EntityRepository(IEntityMapper<T> mapper, IDatabase database) 
    { 
     _mapper = mapper; 
     _database = database; 
    } 

    public T Get(int id) 
    { 
     return this.Get(_mapper, _database, id); 
    } 

    protected virtual T Get(IEntityMapper<T> mapper, IDatabase database, int id) 
    { 
     // Create a command can be used to fetch the entity, remember to dispose when complete 
     using (var cmd = this.CreateGetCommand(id)) 
     { 
      using (var reader = database.ExecuteQuery(cmd)) 
      { 
       // No need to read all the rows, just the first... 
       return reader.Read() ? mapper.MapAll(reader) : default(T); 
      } 
     } 
    } 

    protected abstract SqlCommand CreateGetCommand(int id); 
} 

und Umsetzung der folgenden

public class OrganisationEntityRepository : EntityRepository<OrganisationEntity> 
{ 
    public OrganisationEntityRepository(IEntityMapper<OrganisationEntity> mapper, IDatabase database) : base(mapper, database) 
    { 
    } 

    protected override SqlCommand CreateGetCommand(int id) 
    { 
     var command = new SqlCommand(@"SELECT t.Id, t.Description FROM Organisation t Where t.Id = @Id"); 
     command.Parameters.Add("@id", SqlDbType.Int).Value = id; 

     return command; 
    } 
} 

public class OrganisationEntityMapper : IEntityMapper<OrganisationEntity> 
{ 
    public OrganisationEntity MapAll(SqlDataReader reader) 
    { 
     return new OrganisationEntity(); // Populate using the reader... 
    } 
} 

Answer 5

Hier ist, wie ich es Refactoring würde:

public class Repository<T> : IRepository<T> where T : class, new() 
{ 
    private IEntityMapper<T> _mapper; 

    public Repository(IEntityMapper<T> mapper) 
    { 
     _mapper = mapper; 
    } 

    public virtual T Find(string value) 
    { 
     SqlCommand command = new SqlCommand(); 
     command.CommandText = @"SELECT t.Id, t.Description FROM Organisation t Where t.Description LIKE @value"; 
     command.Parameters.Add("@value").Value = value + "%"; 

     SqlDataReader reader = Database.ExecuteQuery(command, ConnectionName.Dev); 
     return FillCollection(reader); 
    } 

    public void T Get(int id) 
    { 
     SqlCommand command = new SqlCommand(); 
     command.CommandText = @"SELECT t.Id, t.Description FROM Organisation t Where t.id = @value"; 
     command.Parameters.Add("@value").Value = id; 

     SqlDataReader reader = Database.ExecuteQuery(command, ConnectionName.Dev); 
     if (!reader.Read()) 
      return null; 

     T entity = new T(); 
     _mapper.Map(entity, reader); 
     return entity; 
    } 

    protected IList<T> FillCollection(IDataReader reader) 
    { 
     List<T> items = new List<T>(); 
     while (reader.Read()) 
     { 
      T entity = new T(); 
      _mapper.Map(entity, reader); 
      _items.Add(entity); 
     } 
     return items; 
    } 
} 



public interface IEntityMapper<T> 
{ 
    //row is the most generic part of a DataReader 
    void Map(T entity, IDataRow row); 
} 

Eigenpunkte:

1. eine Basisklasse mit FillCollection als geschützte Erstellt Methode.
2. Erzwungene Entität als Klasse mit einem Standardkonstruktor zur Beschleunigung der Objekterstellung.
3. Verwenden Sie eine Schnittstelle für den Mapper und nehmen Sie es im Konstruktor.
4. Ich würde nicht eine ganze Sammlung füllen, um nur den ersten Artikel zu bekommen. Verschwendung von CPU.
5. Versuchen als generische Schnittstellen wie möglich zu verwenden, wie IDataRecord statt DbDataReader