-2

Please can you tell me if my Data Access Model makes any sense?

Thank you very much.

enter image description here

10
  • 7
    Define "makes sense." Feb 27, 2018 at 22:54
  • 2
    Makes no sense to me as you've shown me a bunch of random boxes, rather than real code...
    – David Arno
    Feb 27, 2018 at 22:59
  • 2
    @DavidArno: Well, you can infer several things from the class diagram without seeing any code, but without some basis for comparison like a requirements specification there's no way we can meaningfully accomplish a "makes sense" evaluation. Feb 27, 2018 at 23:08
  • So, my idea is to create a Data Access Layer model that allows me to change from one database technology to another, without compromising the overall application (BLL, GUI, etc). Thank you very much Feb 27, 2018 at 23:20
  • 2
    @DavidArno: You're kidding, right? TDD does not substitute for an effective design process. I know there are people out there who think it does, but it doesn't. System design doesn't naturally emerge from red-green-refactor; human thought and planning is required. TDD can only inform good design; it cannot create it from whole cloth. Feb 28, 2018 at 1:30

5 Answers 5

2

Unfortunately, because you've posted a picture, rather than code, it's harder to reference your design when commenting on it.

For me, the point where your design goes all wrong is:

Public DAL(int DbType)
    switch(DbType ...

Do not make DAL reponsible for creating a concrete implementation of AbstractDAL as well as being an implementation of IFootballProject. That undermines all the careful work the rest of your picture puts into abstracting things. Remember, tell, don't ask. And remember the single responsibility principle. Inject the concrete DAL (created by your app setup code, eg the IoC container) into an implementation of IFootballProject

Just noticed that this is a C# question. In which case, do not make AbstractDAL an abstract base class. Make it an interface. Please.

0

@David Arno's answer is IMHO fine. However, when fixing the DAL class, there are some more details to consider:

  • the name of the class should not be DAL, it should probably be FootballProject. This still smells like becoming a god class, but it is ok to start with it and refactor when it becomes bigger.

  • the switch in the constructor code belongs somewhere else. It could be refactored to some factory class, and then the application's setup code can use it to create a specific implementation of an abstract DAL.

  • the switch should not use an int, it should be utilizing an enum, where the values have proper names.

0

When I last wrote a DAL it looked something like this:

// Season data, stored in a Season table
class Season
{
  public static List<Season> GetAllSeasons(DataReader dataReader);

  // plus public season properties and/or methods
}

My DataReader is like your AbstractDAL i.e. it implements DB-access methods. It contains (i.e. it wraps, encapsulates) a DbConnection which might be a SqlConnection or something else.

The GetAllSeasons method (for example) uses it to read instances of Seasons from its corresponding table.

I add new data types by adding new classes (e.g. Country) without editing existing classes.


Application code might use it like this:

using (DataReader dr = new DataReader())
{
  List<Season> seasons = Season.GetAllSeasons(dr);
}

The application controls the lifetime of the DataReader (and DataWriter) instances.

Note that DataReader and DataWriter are short-lived, easy to use (you just create it with no constructor argument, and pass it as a parameter to static methods of data classes like Season which know how to use them), and implement IDisposable.


In C# you can define static extension methods. So something like:

static class SeasonExtensions
{
    public static List<Season> GetAllSeasons(this DataReader dataReader)
    {
        return Season.GetAllSeasons(dataReader);
    }
}

... "extends" the DataReader class so that the application can use it like this:

using (DataReader dr = new DataReader())
{
  List<Season> seasons = dr.GetAllSeasons();
}

I wrote, "my DataReader is like your AbstractDAL" ... but, my DataReader isn't abstract: it's a leaf subclass which the application can instantiate easily.

The extent to which it is abstract is a class hierarchy like this:

// base class, implements methods for reading
abstract class DataBase { ... }

// subclass, inherits methods for reading
sealed class DataReader : DatabaseBase { ... }

// subclass, adds methods for writing
sealed class DataWriter : DataBase { ... }

The application decides whether to instantiate a DataReader or a DataWriter: depending on whether it needs to do any writing during this transaction, or only reading.

So the class for a specific type of data (e.g. Season) defines methods like:

class Season
{
  // can be called with DataReader or DataWriter
  public static List<Season> GetAllSeasons(DataBase dataBase);

  // can only be called with a DataWriter
  public void InsertNew(DataWriter dataWriter)

  // plus public season properties and/or methods
}
-2

No. The good bit is your IFootballProject.

Just implement it twice, once per database type. Don't try and abstract the database access you dont need to and something like mongoDB wont fit with sql abstractions

-2

Thank you all for your comments.

I'm just learning this stuff, so sometimes your vocabulary is hard for me to understand but for what I could infer from comments.

Someone said my base class AbstractDAL should be an interface, but I have some implementations which will be transversal for all sub classes that may derive from it.

public abstract class AbstractDAL
{
    protected DbCommand     Command     { get; set; }
    protected DbConnection  Connection  { get; set; }


    protected void Open()
    {
        Connection.Open();
    }
    protected void Close()
    {
        Connection.Close();
    }


    protected int ExecuteNonQuery()
    {
        return Command.ExecuteNonQuery();
    }

    protected object ExecuteScalar()
    {
        return Command.ExecuteScalar();
    }

    protected DbDataReader ExecuteReader()
    {
        return Command.ExecuteReader();
    }
}

And then I'm implementing it twice because I know my project will be running at least on two servers: MSSql Server and MySQL.

public class MsSqlServerDAL : AbstractDAL
{
    public MsSqlServerDAL(string ConnectionString)
    {
        Command = new SqlCommand();
        Connection = new SqlConnection(ConnectionString);

        Command.Connection = Connection;
    }
}

// Here will be MySqlServerDAL

Now I will be able to create the public class FootballProjectDAL that has a private member of AbstractDAL type, and I can inject into FootballProjectDAL through the constructor the implemented variation of AbstractDAL.

I hope this makes sense.

 public class FootballProjectDAL : IFootballProject
{
    private AbstractDAL dal;

    public FootballProjectDAL(AbstractDAL DAL)
    {
        dal = DAL;
    }


    //
    // Interface IFootballProject Implementation.
    //
    public List<Competition> GetCompetitionByName(string Name)
    public List<Competition> GetCompetitions()
    public List<Competition> GetCompetitionsByCountry(Country Country)

    public List<Country> GetCountries()  
    public List<Country> GetCountriesByParent(string Parent)
    public Country GetCountryByName(string Name)

    public List<Group> GetGroups()

    public List<Phase> GetPhases()

    public Season GetSeasonByName(string Name)
    public List<Season> GetSeasons()

    public List<Team> GetTeamByName(string Name)
    public List<Team> GetTeams()
    public List<Team> GetTeamsByCountry(Country Country)

    // Here will be all the other methods from IFootballProject interface.
}

To finalize I have updated the initial pic.

enter image description here

Again thank you so much for your time and tips.

4
  • AbstractDAL doesn't need to be abstract. It can be sealed. It contains abstract members e.g. DbConnection. You need a way to decide what subclass of DbConnection to create, but you don't have to do that by making DAL abstract and that decision can be made outside DAL. Also when someone said "make it an interface" they could (should) have said "give it an interface"), e.g. let the Dal class implement some IDal interface, and pass IDal (not Dal) to any code which needs to use the Dal.
    – ChrisW
    Feb 28, 2018 at 11:20
  • 2
    Please do not reply to answers by posting more answers. And why have you aksed the question with one account and said "thanks" with another?
    – David Arno
    Feb 28, 2018 at 12:19
  • this forum is complicated
    – one
    Feb 28, 2018 at 12:54
  • 2
    It's not a forum, it's meant to be a Question and Answer site -- with one question at the top, and only answers (not more questions) underneath. It's not really meant for discussion (if you need to discuss your design instead of just question it, this site isn't meant to support that).
    – ChrisW
    Feb 28, 2018 at 13:04

Not the answer you're looking for? Browse other questions tagged or ask your own question.