6
\$\begingroup\$

Our team is developing an educational website using ASP.NET webforms. We designed DAL & BAL classes and now I have some questions:

  1. Are DAL & BAL designed well?

  2. Would it be better using ORM (like EF) instead of using our own classes? (ORM is slow, but I'm not sure)

  3. Should we ignore BAL and only have DAL (2 layers instead of 3 layers)?

Notice that:

  • there are many users and hits.
  • every page needs a database and there are many queries.
  • performance and speed are important for our team.

DAL

// These lines will import the needed namespaces
using System;
using System.Data;

// Declare our datalayer namespace
namespace DataLayer
{

#region "Cat_Table Class"
// Create the class, based on our table and make it Serializable()
// Making it Serializable means that we will be able to store an instance
// of our class in a session variable or save and load it from disk
[Serializable()] public partial class Cat_Table
{

#region "Instance Variables"
    protected int m_CatID;
    protected string m_CatName;
    // The IsDirty flag is set whenever a property is changed
    protected bool m_IsDirty;
    // The m_ConnectionString holds the connection string
    protected string m_ConnectString;
    // The m_IsUpdate determines if, when we save a record 
    // it is an insert or update
    protected bool m_IsUpdate;

#endregion

#region "Constructors"
    // This is the default empty constructor. Basically
    // all it does is setup the connectionstring
    public Cat_Table()
    {
        // Call SetConnectionString() to set our connection string
        // for connecting to the database
        SetConnectString();
        // The m_IsUpdate flag holds weather we are doing an insert or an update
        // Since no primary key was passed, this is not an update. Its an insert.
        m_IsUpdate = false;
    }

    // This is the default constructor called when passing a primary key.
    // It basically does the same thing as the empty constructor, but it also
    // loads the record passed and fills the instances variables
    public Cat_Table(int pkPrimaryKey)
    {
        // The m_IsUpdate flag holds weather we are doing an insert or an update
        // Since no primary key was passed, this is not an update. Its an insert.
        m_IsUpdate = true;
        // Call SetConnectionString() to set our connection string
        // for connecting to the database
        SetConnectString();
        // Call the Get subroutine to load the record and fill the member variables
        this.Get(pkPrimaryKey);
    }

    // This constructor is the default constructor called when
    // a datarow is passed. It loads the member variables with the row data
    public Cat_Table(DataRow dtrRow)
    {
        // The m_IsUpdate flag holds weather we are doing an insert or an update
        // Since no primary key was passed, this is not an update. Its an insert.
        m_IsUpdate = true;
        // Call SetConnectionString() to set our connection string
        // for connecting to the database
        SetConnectString();
        // Call the Fill subroutine to fill the member variables with the row passed
        this.Fill(dtrRow);
    }
#endregion

#region "Properties"
    // This property sets the CatID Field
    public int CatID {
        get {           return this.m_CatID; }
        set {
            // The m_IsDirty flag is set because this row now needs to be saved
            this.m_IsDirty = true;
            this.m_CatID = value;
        }
    }

    // This property sets the CatName Field
    public string CatName {
        get {           return this.m_CatName; }
        set {
            // The m_IsDirty flag is set because this row now needs to be saved
            this.m_IsDirty = true;
            this.m_CatName = value;
        }
    }

#endregion

#region "Connection Routines"
    // This sub sets our member m_connectString to the value of the connection string,
    // As pulled from our Universal file
    protected void SetConnectString()
    {
        m_ConnectString = DataLayer.Universal.GetConnectionString();
    }

    // This function returns the value of our m_ConnectString variable
    protected string GetConnectString()
    {
        return m_ConnectString;
    }

    // This function returns our Universal Connect string. It is a shared function
    // meaning you can call it without instantiating an instance of this class
    protected static string GetConnectionString()
    {
        return DataLayer.Universal.GetConnectionString();
    }
#endregion

#region "Copy SubRoutine"
    // The Copy subroutine copies the values from
    // the DataRow passed into the member variables
    public void Copy(DataRow dtrRow)
    {
        try {
            this.m_CatID = (int)dtrRow["CatID"];
        } catch (Exception) {
            // TODO: You might want to add code here to trap any
            // errors that might occur
            // Usually the only way an error would occur at this particular
            // spot is if a field was removed from the database and this file was not
            // updated by LayerGen MMX
        }

        try {
            this.m_CatName = (string)dtrRow["CatName"];
        } catch (Exception) {
            // TODO: You might want to add code here to trap any
            // errors that might occur
            // Usually the only way an error would occur at this particular
            // spot is if a field was removed from the database and this file was not
            // updated by LayerGen MMX
        }

    }
#endregion

#region "Fill SubRoutine"
    // Fill Subroutine fills the member variables with a datarow passed to the sub
    protected void Fill(DataRow dtrRow)
    {
        try {
            if (((!object.ReferenceEquals(dtrRow["CatID"],  DBNull.Value)))) {
                this.m_CatID = (int)dtrRow["CatID"];
                m_IsUpdate = true;
            }
        } catch (Exception) {
                    m_IsUpdate = false;
            // TODO: You might want to add code here to trap any
            // errors that might occur
            // Usually the only way an error would occur at this particular
            // spot is if a field was removed from the database and this file was not
            // updated by LayerGen MMX or you changed the data type for a field
        }
        try {
            if (((!object.ReferenceEquals(dtrRow["CatName"],  DBNull.Value)))) {
                this.m_CatName = (string)dtrRow["CatName"];
            }
        } catch (Exception) {
            // TODO: You might want to add code here to trap any
            // errors that might occur
            // Usually the only way an error would occur at this particular
            // spot is if a field was removed from the database and this file was not
            // updated by LayerGen MMX or you changed the data type for a field
        }
    }
#endregion

#region "Get SubRoutine"

    // The Get Subroutine retreives a single record based on the
    // primary key passed to it. It then fills the member variables
    // with the data retrieved
    private void Get(int primID)
    {
        // This variable holds the name of the stored procedure that
        // is called to retrieve the record
        string storedProcedure = "spCat_TableSelect";
        System.Data.SqlClient.SqlCommand cmd = new System.Data.SqlClient.SqlCommand();
        // This dataset will hold the results of the stored procedure
        DataSet ds;
        // This holds our connection string to the database
        string ConnectionString = GetConnectString();
        System.Data.SqlClient.SqlConnection conn = new System.Data.SqlClient.SqlConnection();
        System.Data.SqlClient.SqlDataAdapter da;

        conn.ConnectionString = ConnectionString;
        cmd.CommandType = CommandType.StoredProcedure;
        cmd.Connection = conn;
        cmd.CommandText = storedProcedure;
        // This sets up the SQL stored procedure parameter
        cmd.Parameters.Add("@CatID", SqlDbType.Int);
        cmd.Parameters["@CatID"].Value = primID;

        try {
            conn.Open();
            da = new System.Data.SqlClient.SqlDataAdapter();
            da.SelectCommand = cmd;

            ds = new DataSet();
            da.Fill(ds);

            if (ds.Tables.Count > 0) {
                if (ds.Tables[0].Rows.Count > 0) {
                    Fill(ds.Tables[0].Rows[0]);
                }
            }
         } catch (Exception ex) {
            // TODO: You may want to add code here to handle any kind
            // error that could occur. If the stored procedures were NOT
            // modified, then only one of two things usually can throw an error here.
            // Possibility number one: The SQL Server is down or cannot be accessed for some reason
            // or possibility number two: The stored procedure can not be found
            throw ex;
        }
        conn.Close();
    }
#endregion

#region "Delete SubRoutine"
    // This is the Delete Sub. It deletes the currently loaded record
    public void Delete()
    {
        // This variable holds the name of the stored procedure that
        // is called to retrieve the record
        string storedProcedure = "spCat_TableDelete";
        System.Data.SqlClient.SqlCommand cmd = new System.Data.SqlClient.SqlCommand();
        // This holds our connection string to the database
        string ConnectionString = GetConnectString();
        System.Data.SqlClient.SqlConnection conn = new System.Data.SqlClient.SqlConnection();

        conn.ConnectionString = ConnectionString;
        cmd.CommandType = CommandType.StoredProcedure;
        cmd.Connection = conn;
        cmd.CommandText = storedProcedure;
        // This sets up the SQL stored procedure parameter
        cmd.Parameters.Add("@CatID", SqlDbType.Int);
        cmd.Parameters["@CatID"].Value = this.m_CatID;

        try {
            conn.Open();
            cmd.ExecuteNonQuery();
        } catch (Exception ex) {
            throw ex;
            // TODO: You may want to add code here to handle any kind
            // error that could occur. If the stored procedures were NOT
            // modified, then only one of three things usually can throw an error here.
            // Possibility number one: The SQL Server is down or cannot be accessed for some reason
            // or possibility number two: The stored procedure can not be found
            // or possibility number three: the record cannot be deleted, possibly because
            // of a foreign key conflict
        }
        conn.Close();
    }
#endregion

#region "Save Subroutine"

    // This sub will either insert or update the current record into the table
    public void Save()
    {
        // This variable holds the name of the stored procedure that
        // is called to retrieve the record
        string storedProcedure = null;
        System.Data.SqlClient.SqlCommand cmd = new System.Data.SqlClient.SqlCommand();
        // This holds our connection string to the database
        string ConnectionString = GetConnectString();
        System.Data.SqlClient.SqlConnection conn = new System.Data.SqlClient.SqlConnection();

        // Only update if this record is "dirty"
        if (m_IsDirty) {
            conn.ConnectionString = ConnectionString;
            cmd.Connection = conn;
            cmd.CommandType = CommandType.StoredProcedure;

            cmd.Parameters.Add("@CatID", SqlDbType.Int);

            // Add CatName as a parameter
            cmd.Parameters.Add("@CatName", SqlDbType.NVarChar);
            if (this.m_CatName != null) {
                cmd.Parameters["@CatName"].Value = this.m_CatName;
            } else {
                cmd.Parameters["@CatName"].Value = "";
            }

            // Test to see if we are updating or inserting
            if (m_IsUpdate == true) {
                storedProcedure = "spCat_TableUpdate";
                cmd.CommandText = storedProcedure;

                cmd.Parameters["@CatID"].Value = this.m_CatID;
                cmd.Parameters["@CatID"].Direction = ParameterDirection.Input;
                try {
                    conn.Open();
                    cmd.ExecuteNonQuery();
                } catch (Exception ex) {
                    throw ex;
                    // TODO: Write code here to handle any error that could occur.
                    // There are many reasons why an error could occur here, such
                    // as SQL Server not being found or the stored procedure not
                    // being found or perhaps the primary key is a duplicate.
                }
                conn.Close();
            } else {
                storedProcedure = "spCat_TableInsert";
                cmd.CommandText = storedProcedure;

                cmd.Parameters["@CatID"].Value = this.m_CatID;
                cmd.Parameters["@CatID"].Direction = ParameterDirection.Output;
                try {
                    conn.Open();
                    cmd.ExecuteNonQuery();
                } catch (Exception ex) {
                    throw ex;
                    // TODO: Write code here to handle any error that could occur.
                    // There are many reasons why an error could occur here, such
                    // as SQL Server not being found or the stored procedure not
                    // being found or perhaps the primary key is a duplicate.
                }
                this.m_CatID = (int)cmd.Parameters["@CatID"].Value;
                conn.Close();
            }
        }
    }
#endregion

#region "GetAll SubRoutine"
    // This function will retrieve ALL the rows in the table
    // Use common sense when using this function. Obviously this 
    // should not be used on large tables. Usually this function is
    // used just before populating a drop down list.
    public static DataTable GetAll()
    {
        string storedProcedure = "spCat_TableGetAll";
        System.Data.SqlClient.SqlCommand cmd = new System.Data.SqlClient.SqlCommand();
        string ConnectionString = GetConnectionString();
        System.Data.SqlClient.SqlConnection conn = new System.Data.SqlClient.SqlConnection();

        conn.ConnectionString = ConnectionString;
        cmd.CommandType = CommandType.StoredProcedure;
        cmd.Connection = conn;
        cmd.CommandText = storedProcedure;

        try {
            DataSet ds;
            System.Data.SqlClient.SqlDataAdapter da;
            DataTable Table = null;

            conn.Open();
            da = new System.Data.SqlClient.SqlDataAdapter();
            da.SelectCommand = cmd;

            ds = new DataSet();
            da.Fill(ds);

            if (ds.Tables.Count > 0) {
                if (ds.Tables[0].Rows.Count > 0) {
                    Table = ds.Tables[0];
                }
            }
            conn.Close();
            return Table;
        } catch (Exception ex) {
            throw ex;
            // TODO: Add code here to catch any kind of error.
            // An error would only occur here if 1) The SQL server is down
            // or cannot be reached or 2) The GetAll stored procedure cannot
            // be found or 3) You are timing out because you are trying to pull too much data
        }
    }
#endregion

#region "GetBySQLStatement SubRoutine"
    public static DataTable GetBySQLStatement(string SQLText)
    {
        System.Data.SqlClient.SqlCommand cmd = new System.Data.SqlClient.SqlCommand();
        string ConnectionString = GetConnectionString();
        System.Data.SqlClient.SqlConnection Conn = new System.Data.SqlClient.SqlConnection();

        Conn.ConnectionString = ConnectionString;
        cmd.CommandType = CommandType.Text;
        cmd.Connection = Conn;
        cmd.CommandText = SQLText;

        try {
            DataSet ds;
            System.Data.SqlClient.SqlDataAdapter da;
            DataTable Table = null;

            Conn.Open();
            da = new System.Data.SqlClient.SqlDataAdapter();
            da.SelectCommand = cmd;

            ds = new DataSet();
            da.Fill(ds);

            if (ds.Tables.Count > 0) {
                if (ds.Tables[0].Rows.Count > 0) {
                    Table = ds.Tables[0];
                }
            }
            Conn.Close();
            return Table;
        } catch (Exception ex) {
            throw ex;
        }
        Conn.Close();
    }
#endregion

}
#endregion
}

BAL

using System;
using System.Data;
using System.Collections;

namespace BusinessLayer
{

#region "Cat_Table Class"
[Serializable()]
public class Cat_Table : DataLayer.Cat_Table, BusinessLayer.IBusiness
{

#region "Constructors"
    public Cat_Table() : base()
    {
    }

    public Cat_Table(int pkPrimaryKey) : base(pkPrimaryKey)
    {
    }

    public Cat_Table(DataRow dtrRow) : base(dtrRow)
    {
    }

#endregion

#region "Interface Implementation"
    public void Rollback()
    {
    }

    public void Validate()
    {
        // TODO: Write your own validation code
    }
#endregion

}
#endregion

#region "Cat_Tables Class"
[Serializable()]
public partial class Cat_Tables : CollectionBase, System.Collections.Generic.IEnumerable<BusinessLayer.Cat_Table>
{

#region "LINQ implementation"
    public new System.Collections.Generic.IEnumerator<BusinessLayer.Cat_Table> GetEnumerator()
    {
        foreach (BusinessLayer.Cat_Table Cat_TableObj in this.List)
        {
            yield return Cat_TableObj;
        }
    }
#endregion

#region "Enumerated Sort Fields"
    public enum SortFields
    {
        Sort_CatID,
        Sort_CatName
    }

    public enum SortType
    {
        Ascending = 1,
        Descending = 2
    }

#endregion

#region "Sort Comparers"
      private class Comp_CatID : IComparer
      {
        public int Compare(object x, object y)
        {
            BusinessLayer.Cat_Table Cat_Table1 = (BusinessLayer.Cat_Table)x;
            BusinessLayer.Cat_Table Cat_Table2 = (BusinessLayer.Cat_Table)y;

            try {
                return Cat_Table1.CatID.CompareTo(Cat_Table2.CatID);
            } catch (Exception) {
                return 0;
            }
        }
    }

    private class Comp_CatID_D : IComparer
    {

        public int Compare(object x, object y)
        {
            BusinessLayer.Cat_Table Cat_Table1 = (BusinessLayer.Cat_Table)x;
            BusinessLayer.Cat_Table Cat_Table2 = (BusinessLayer.Cat_Table)y;
            int j = 0;

            try {
                j = Cat_Table1.CatID.CompareTo(Cat_Table2.CatID);
                if (j==1) return -1;
                if (j==-1) return 1;
            } catch (Exception) {
                j = 0;
            }
            return j;
        }
    }

      private class Comp_CatName : IComparer
      {
        public int Compare(object x, object y)
        {
            BusinessLayer.Cat_Table Cat_Table1 = (BusinessLayer.Cat_Table)x;
            BusinessLayer.Cat_Table Cat_Table2 = (BusinessLayer.Cat_Table)y;

            try {
                return Cat_Table1.CatName.CompareTo(Cat_Table2.CatName);
            } catch (Exception) {
                return 0;
            }
        }
    }

    private class Comp_CatName_D : IComparer
    {

        public int Compare(object x, object y)
        {
            BusinessLayer.Cat_Table Cat_Table1 = (BusinessLayer.Cat_Table)x;
            BusinessLayer.Cat_Table Cat_Table2 = (BusinessLayer.Cat_Table)y;
            int j = 0;

            try {
                j = Cat_Table1.CatName.CompareTo(Cat_Table2.CatName);
                if (j==1) return -1;
                if (j==-1) return 1;
            } catch (Exception) {
                j = 0;
            }
            return j;
        }
    }

#endregion

#region "Sort Functions"
    public void Sort(SortFields SortField, SortType SortMethod)
    {
        if (SortField == SortFields.Sort_CatID) {
            if (SortMethod == SortType.Ascending) {
                this.InnerList.Sort(new Comp_CatID());
            } else {
                this.InnerList.Sort(new Comp_CatID_D());
            }
        }

        if (SortField == SortFields.Sort_CatName) {
            if (SortMethod == SortType.Ascending) {
                this.InnerList.Sort(new Comp_CatName());
            } else {
                this.InnerList.Sort(new Comp_CatName_D());
            }
        }

    }

    public void Sort(SortFields SortField)
    {
        Sort(SortField, SortType.Ascending);
    }

#endregion

    #region "Constructors"
        public Cat_Tables()
        {

        }

        public Cat_Tables(DataRowCollection datarows) : this()
        {
            this.Load(datarows);
        }
    #endregion

#region "Member Functions"
    protected void Load(DataRowCollection dataRows)
    {
        foreach (DataRow dr in dataRows) {
            this.Add(new Cat_Table(dr));
        }
    }

    public Cat_Table this[int index] {
        get { return (Cat_Table)base.InnerList[index]; }
        set { base.InnerList[index] = value; }
    }

    public int Add(Cat_Table val)
    {
        return base.InnerList.Add(val);
    }
#endregion

#region "Save Function"
    public void Save()
    {
        foreach (Cat_Table a in this.InnerList) {
            a.Save();
        }
    }
#endregion

#region "Rollback Sub"
    public void Rollback()
    {
        foreach (Cat_Table a in this.InnerList) {
            a.Rollback();
        }
    }
#endregion

         public void GetBySQLStatement(string SQLText)
         {
             DataTable dt = DataLayer.Cat_Table.GetBySQLStatement(SQLText);
             if (dt != null) {
                 this.Load(dt.Rows);
             }
         }

#region "GetAll Subroutine"
// Loads all records from the table into the collection
         public void GetAll()
         {
             DataTable dt = DataLayer.Cat_Table.GetAll();
             if (dt != null) {
                 this.Load(dt.Rows);
             }
         }

#endregion

    }
#endregion
}
\$\endgroup\$
7
\$\begingroup\$
private class Comp_CatName_D : IComparer
{

    public int Compare(object x, object y)
    {
        BusinessLayer.Cat_Table Cat_Table1 = (BusinessLayer.Cat_Table)x;
        BusinessLayer.Cat_Table Cat_Table2 = (BusinessLayer.Cat_Table)y;
    }
}

You see how ugly this is. Use the generic IComparer<T> so you can have strongly typed comparisons which don't require casts.

MSDN


try {
    return Cat_Table1.CatName.CompareTo(Cat_Table2.CatName);
} catch (Exception) {
    return 0;
}

If something went wrong so badly it throws an exception, we'll just pretend they're the same? This is logically incorrect and will cause very hard to track down bugs.


try {
    j = Cat_Table1.CatName.CompareTo(Cat_Table2.CatName);
    if (j==1) return -1;
    if (j==-1) return 1;
} catch (Exception) {
    j = 0;
}
return j;

Also known as

return -Cat_Table1.CatName.CompareTo(Cat_Table2.CatName);

class Cat_Tables

Classes use UpperCamelCase as naming convention.


foreach (BusinessLayer.Cat_Table Cat_TableObj in this.List)

use more descriptive names. This will turn the previous line of code in this:

foreach(var table in tables)

public enum SortFields
{
    Sort_CatID,
    Sort_CatName
}

Enums are UpperCamelCase as well.


protected int m_CatID;

No hungarian notation!


Last but not least:

USE AN ORM

Getting a proper ORM up and running is very hard to get right and by creating your own you are

  • Introducing buggy code
  • Using a very limited subset of a full blown ORM
  • Losing performance (yes, losing): you're missing out on caching and other optimization techniques
  • Hardcoding the database dependencies
\$\endgroup\$
5
\$\begingroup\$

Comments

You have a lot of useless comments in your code which should be deleted because thy don't add any value to the code but reduce the readability.

A very good answer about the usage of comments can be found here: https://codereview.stackexchange.com/a/90113/29371

Bottom line, though, is that the comments in your code should fill in the blanks that your code does not. In addition, your comments should give details on the motivation, and not the application of your code. You should, in general, comment only on why your code does things, not what your code is doing.


Regions

Using regions is mostly a code smell and should be avoided.

A very good answer about the usage of regions can be found here: https://softwareengineering.stackexchange.com/a/53114

The reviewed code also contained a lot of regions grouping all the fields together, all the properties together, etc. This had an obvious problem: source code growth.

When you open a file and see a huge list of fields, you are more inclined to refactor the class first, then work with code. With regions, you take an habit of collapsing stuff and forgetting about it.

Another problem is that if you do it everywhere, you'll find yourself creating one-block regions, which doesn't make any sense. This was actually the case in the code I reviewed, where there were lots of #region Constructor containing one constructor.


Constructor

Currently you have duplicated code in your constructors which can be reduced by using constructor chaining. Constructor chaining is an approach where a constructor calls another constructor in the same or base class.

This would lead to

private Cat_Table(bool isUpdate)
{
    SetConnectString();
    m_IsUpdate = isUpdate;
}

public Cat_Table()
    : this(false)
{ }

// This is the default constructor called when passing a primary key.
// It basically does the same thing as the empty constructor, but it also
// loads the record passed and fills the instances variables
public Cat_Table(int pkPrimaryKey)
    : this(true)
{
    this.Get(pkPrimaryKey);
}

// This constructor is the default constructor called when
// a datarow is passed. It loads the member variables with the row data
public Cat_Table(DataRow dtrRow)
    : this(true)
{
    this.Fill(dtrRow);
}  

Instead of using "default" comments you should consider to use correct xml documentation for commenting the purpose of the methods. In this way you will see the comments in intellisense too.

So this

// This is the default constructor called when passing a primary key.
// It basically does the same thing as the empty constructor, but it also
// loads the record passed and fills the instances variables
public Cat_Table(int pkPrimaryKey)  

should look like

/// <summary>
/// This is the default constructor called when passing a primary key....
/// </summary>
/// <param name="pkPrimaryKey">The primary key to fill the instance variables</param>
public Cat_Table(int pkPrimaryKey)  

The usage of the m_IsDirty flag can be optimized. Right now you don't check wether the new value is different from the current value. If the value which is set is the same, then you shouldn't need to set this flag and therefor you won't need to save the current row either.


protected void SetConnectString()  

this does not do what the method name implies. It does not Set the connection string but read it from a file. You should consider to rename it to e.g FillConnectionString() or ReadConnectionString().


\$\endgroup\$
5
\$\begingroup\$

Jeroen has already covered some of the problems with Comp_CatID/Comp_CatID_D/Comp_CatName/Comp_CatName_D, but there's another one:

try {
    j = Cat_Table1.CatID.CompareTo(Cat_Table2.CatID);
    if (j==1) return -1;
    if (j==-1) return 1;
} catch (Exception) {
    j = 0;
}
return j;

IComparable<T>.CompareTo(T) does not guarantee that it will only ever return -1, 0, or 1. So if j == 2, this method will return 2.

You also don't want to do this:

return -Cat_Table1.CatName.CompareTo(Cat_Table2.CatName);

(try running int x = int.MinValue; Console.WriteLine(x == -x);)

I would suggest that you remove the classes and instead use List<T>.Sort(Comparison<T>) like so:

// sort by CatID ascending
this.InnerList.Sort((x, y) => x.CatID.CompareTo(y.CatID));

// sort by CatID descending
this.InnerList.Sort((x, y) => y.CatID.CompareTo(x.CatID));

// sort by CatName ascending
this.InnerList.Sort((x, y) => string.Compare(x.CatName, y.CatName));

etc.

Please see the list of string.Compare overloads to see which one is right for your use-case.

\$\endgroup\$
3
\$\begingroup\$

Just two very quick notes.

  1. I'm not going to argue about brace styles one way or the other, but be consistent!
    public void Copy(DataRow dtrRow)
    {
        try {
            this.m_CatID = (int)dtrRow["CatID"];
        } catch (Exception) {
            // TODO: You might want to add code here to trap any
            // errors that might occur
            // Usually the only way an error would occur at this particular
            // spot is if a field was removed from the database and this file was not
            // updated by LayerGen MMX
        }

Most editors have a keyboard shortcut for this. Learn it and use it.

  1. That TODO comment... Instead of saying you might want to catch Blah blah blah, just don't catch at all. You better have a really good reason to swallow every possible exception that could ever occur. It doesn't look like you do. If you do, you need a better comment.

  2. Don't leave empty methods without explaining yourself.

    #region "Interface Implementation"
    public void Rollback()
    {
    }
    
     public void Validate()
    {
        // TODO: Write your own validation code
    }
    

    You really need to explain to your future self why this hasn't been implemented.

\$\endgroup\$

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.