3
\$\begingroup\$

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);

        }
    }
\$\endgroup\$
2
  • \$\begingroup\$ admin , could you please merge my account. \$\endgroup\$ – Umar Oct 30 '16 at 22:35
  • \$\begingroup\$ @user1263981 I've contacted the Community team to request the merge. You can also use the contact us link at the bottom of any page. \$\endgroup\$ – Mathieu Guindon Oct 30 '16 at 22:40
1
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ thank you for the reply. yes you are right naming conventions are not good. In UI , i am creating an object of Class Booking and setting their properties dynamically and then calling BookingDB class methind insert. Creating sepereate ReportRecord class makes sense, but where would i place your last query( IEnumerable<ReportRecord>) whould this go in ReportRecordDB class? \$\endgroup\$ – Umar Oct 30 '16 at 20:36
  • \$\begingroup\$ Hard to tell as I don't have the "Big Picture", but that query is meant to replace the GetReport method. \$\endgroup\$ – Mathieu Guindon Oct 30 '16 at 20:37
  • \$\begingroup\$ Also, is it okay to call dispose , insert and update in every single data layer class? \$\endgroup\$ – Umar Oct 30 '16 at 20:39
  • \$\begingroup\$ IMO an entity type should be nothing but an entity type; look into the repository pattern to see where CRUD belongs; looks like BookingsDb could work as a repository, but then it should implement IDisposable and be provided with an abstraction of the LinqDatabase in its constructor instead of newing it up. \$\endgroup\$ – Mathieu Guindon Oct 30 '16 at 20:43
  • \$\begingroup\$ i have been reading this article dotnetguts.blogspot.co.uk/2013/11/… , just wondering what does IDepartments repository = new DepartmentsImpl(); do? Its right at the bottom inside Presentation class. DepartmentsImpl repository = new DepartmentsImpl(); would do the same thing? \$\endgroup\$ – Umar Oct 31 '16 at 0:26
1
\$\begingroup\$

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?

\$\endgroup\$
3
  • \$\begingroup\$ In the web project, we have seperate classes for each entity in the object and database layer. Clerks, Authorities and Bookings are three tables. \$\endgroup\$ – Umar Oct 30 '16 at 20:01
  • \$\begingroup\$ @user1263981 are you the OP? Seems you're logged in under a different user than the one that posted the question. Let me know if your accounts should be merged. \$\endgroup\$ – Mathieu Guindon Oct 30 '16 at 20:24
  • \$\begingroup\$ yes i am, please merge my account , i can't see any option to do so. Many Thanks \$\endgroup\$ – Umar Oct 30 '16 at 20:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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