Implementing SOLID Principles - LINQ TO SQL

Question

I have been using Layered Architecture in my web application. Can anyone please advise if my data layer classes (both Linq and booking class) can be improved?

I have just started learning about solid design principles and Dependency injections. Does my object or data layer classes achieve any of the SOLID principles?

Object Layer

        [Table()]
        public class Clerks
        {
            [Column(CanBeNull = false, IsPrimaryKey = true, IsDbGenerated = true)]
            public Int32 ClerkID { get; set; }
            [Column()]
            public string Name { get; set; }
            [Column()]
            public string Email { get; set; }
            [Column(IsVersion = true, IsDbGenerated = true)]
            public byte[] Version { get; set; }
        }

LINQ Class in Data Layer

            /// <summary>
        /// LinqDatabase Class extends DataContext
        /// Entry point for the LINQ to SQL framework
        /// </summary>
        public class LinqDatabase : DataContext
        {

            private const Int32 timeOut = 20;

            private static string connStr = ConfigurationManager.ConnectionStrings["SQLConnection"].ConnectionString;



            public LinqDatabase(bool objTrack = false): base(connStr)
            {
                base.CommandTimeout = timeOut;
                //to track the original value and object identity
                base.ObjectTrackingEnabled = objTrack;
            }

            public Table<Clerks> Clerks;
            public Table<Authorities> Authorities;
            public Table<Bookings> Bookings;


            public static DataTable ToDataTable<T>(IEnumerable<T> collection, string tableName)
            {
                DataTable tbl = ToDataTable(collection);
                tbl.TableName = tableName;
                return tbl;
            }


            public static DataTable ToDataTable<T>(IEnumerable<T> collection)
            {
                DataTable dt = new DataTable();
                Type _t = typeof(T);
                PropertyInfo[] pia = _t.GetProperties();

                //Create the columns in the DataTable
                foreach (PropertyInfo pi in pia)
                {
                    dt.Columns.Add(pi.Name, Nullable.GetUnderlyingType(pi.PropertyType) ?? pi.PropertyType);
                }

                //Populate the table
                foreach (T item in collection)
                {
                    DataRow dr = dt.NewRow();
                    dr.BeginEdit();
                    foreach (PropertyInfo pi in pia)
                    {
                        dr[pi.Name] = pi.GetValue(item, null);
                    }
                    dr.EndEdit();
                    dt.Rows.Add(dr);
                }

                return dt;
            }
        }

Booking Data Layer Class

    public class BookingsDB
    {

        private LinqDatabase db = new LinqDatabase();

        public void Dispose()
        {
            db.Dispose();
        }

        public static void Insert(Bookings l)
        {
            LinqDatabase dbs = new LinqDatabase(true);

            try
            {
                dbs.Bookings.InsertOnSubmit(l);
                dbs.SubmitChanges();
            }
            catch (Exception ex)
            {
                //log error

            }
            finally
            {
                dbs.Dispose();
                dbs = null;
            }
        }


        /// <summary>
        /// Get Report in specific date range
        /// </summary>
        /// <param name=" p_startDate, p_EndDate">Passing date range to get a report</param>
        public DataTable GetReport(DateTime p_startDate, DateTime p_EndDate)
        {

            dynamic query = (from b in db.Bookings
                             join a in db.Authorities on b.AuthorityID equals a.AuthorityID
                             join c in db.Clerks on b.ClerkID equals c.ClerkID
                             where b.BookingDate >= p_startDate && b.BookingDate <= p_EndDate
                             orderby a.Name , b.BookingDate 
                             select new { AuthorityName = a.Name, b.BookingDate, ClerkName = c.Name, c.Email });

            return LinqDatabase.ToDataTable(query);

        }
    }

Answer 1

This is unusual:

dynamic query = (from b in db.Bookings
                 join a in db.Authorities on b.AuthorityID equals a.AuthorityID
                 join c in db.Clerks on b.ClerkID equals c.ClerkID
                 where b.BookingDate >= p_startDate && b.BookingDate <= p_EndDate
                 orderby a.Name , b.BookingDate 
                 select new { AuthorityName = a.Name, b.BookingDate, ClerkName = c.Name, c.Email });

The query is an IQueryable<T>, where T is an anonymous type. These have been around since C# 3.5, when LINQ was introduced along with the var keyword.

Using var keeps the type statically determined at compile time: you can hover the keyword and Visual Studio reveals what the resolved type is.

dynamic is a completely different other beast: it takes typing out of compile-time (deferred to runtime) and literally breaks the strongly-typed nature of the query, in this case for no reason at all.

Use var, not dynamic. Keep dynamic for interop scenarios and other places where the actual type cannot be determined at compile-time. Anonymous types don't fall in this category.

var query = (from b in db.Bookings
             join a in db.Authorities on b.AuthorityID equals a.AuthorityID
             join c in db.Clerks on b.ClerkID equals c.ClerkID
             where b.BookingDate >= p_startDate && b.BookingDate <= p_EndDate
             orderby a.Name , b.BookingDate 
             select new { AuthorityName = a.Name, b.BookingDate, ClerkName = c.Name, c.Email });

Now, exposing anonymous types in public interfaces isn't ideal - but exposing an anonymous type wrapped up in a dynamic type is even worse; might as well expose it as an object!

Why not make a simple class to wrap up these properties?

public class ReportRecord
{
    public ReportRecord(string authority, DateTime date, string clerk, string email)
    {
        AuthorityName = authority;
        BookingDate = date; 
        ClerkName = clerk;
        Email = email;
    }

    public string AuthorityName { get; }
    public DateTime BookingDate { get; }
    public string ClerkName { get; }
    public string Email { get; }
}

And now you can return an IEnumerable<ReportRecord>:

return from b in db.Bookings
       join a in db.Authorities on b.AuthorityID equals a.AuthorityID
       join c in db.Clerks on b.ClerkID equals c.ClerkID
       where b.BookingDate >= p_startDate && b.BookingDate <= p_EndDate
       orderby a.Name, b.BookingDate 
       select new ReportRecord(a.Name, b.BookingDate, c.Name, c.Email);

---

It's not clear why Insert is a static method either, nor why it's not using the instance-level db field that's already there. The dbs local variable should be in a using block instead of a try/catch/finally, and I can't help mentioning that l is a terrible name for anything. Avoid single-letter identifiers; literally anything would be a better name for this: entity, booking, item, whatever. But not l.

DI and SOLID is about OOP, and static members belong to types, not objects.

Your code is all about concrete types and implementations, yet SOLID is all about interfaces and abstractions: the code that consumes a LinqDatabase can only work with a LinqDatabase, which makes writing unit tests for your business logic impossible without hitting the database.

Answer 2

LinqDatabase and DI

Well, the virtually hardcoded connection string and the timeout hurt testibility. The connection string should be a parameter of the constructor. You can use a default value but without having the ability to override it you cannot test it independantly with a different database. The timeout could be a property.

There is no need to use data tables here. You write this data layer to work with strongly typed structures like the Clerks class - that as a matter of fact should be called Clerk because it represents a single entity. Turning them into a data table is counterproductive.

Methods like ToDataTable violate the SRP (single responsibily principle) of your LinqDatabase class. It should only care about how to get and save data and not how to convert it to something else.

You could however create a conversion service and pass it via DI to the repository if you want to keep the data tables.

What are the three Clerks, Authorities and Bookings fields for? You don't use them anywhere. And the fact that they are public fields makes them even more evil. There is acutally only one kind of public field that we use - a static readonly field with some constant-like value.

BookingDB

It's not clear to me why you need this class. You already have your LinqDatabase that should know how to get bookings. Just add your queries there.

This one is really unnecassary and it's implementation is wrong or rather incomplete. If you added the Dispose method then you should also add the IDisposable interface to it.

You like data tables, don't you? ;-) Get rid of them. Create a new class for the result of the GetReport query and return it as a List<YourNewType>. It's much easier to work with objects then data tables.

using

You could use some more usings in the Insert method:

using(var dbs = new LinqDatabase(true))
{
    // insert
}

Parameter names

public static void Insert(Bookings l)

I wonder how did you come up with the l for bookings. You should give your parameters meaningful names because the will appear in the intellisense. If I saw only l I'd be really really confused. Why don't you call it simply booking?