N-tier application with BAL and DAL methods

I was just asking this question over here. I'm on a project in which I'm failing to see the point of how a previous developer made decisions. I HAVE taken over this project and wish to make changes.

• Same exact method names in DAL and BAL

• Static is EVERYWHERE

• What should I do with new methods to follow best practices?

Example of existing code:

Calling appliction (could be console app or web app etc. agnostic ):

DataSet DS = CreditMgr.GetCreditRqstInfo(ddlGEO.Text);

BAL

public class CreditMgr
{
   public static DataSet GetCreditRqstInfo(String GeoID)
    {
        try
        {
            DataSet DS = new DataSet();
            DS = CreditIntfDB.GetCreditRqstInfo(GeoID);
            return DS;
        }
        catch (Exception ex)
        {
            throw ex;
        }
    }
}

DAL

public class CreditIntfDB
{
     public static DataSet GetCreditRqstInfo(String GeoID)
    {
        try
        {
            Database DB = new SqlDatabase(Common.ConnectionString);
            String SQLCommand = Common.SPGetRqstInfo;
            DbCommand DBCommand = DB.GetStoredProcCommand(SQLCommand);
            DBCommand.CommandTimeout = Common.CommandTimeOut;
            DB.AddInParameter(DBCommand, "@a_geo_id", DbType.String, GeoID);
            DataSet DS = new DataSet();
            DB.LoadDataSet(DBCommand, DS, new String[] { "CreditRqstInfo" });

            return DS;
        }
        catch (Exception ex)
        {
            throw ex;
        }
    }
}

Yes, the whole point is to have layers of separation, but when the same method names are being used, and static, and each are simply doing the same exact thing with passing in string and returning a DataSet has "code smell" to me.

Any suggestions on better ways?

Let's start with this anti-pattern.

catch (Exception ex)
    {
        throw ex;
    }

You need to immediately search the code base for every instance of this pattern and remove it immediately. It serves no purpose but to destroy the stack trace. That makes it much harder to debug production issues, because you've no idea where the exception really came from.

(While you're at it, look for any place the previous developer may have been simply swallowing exceptions too.)

Next is this patten:

DataSet DS = new DataSet();
        DS = CreditIntfDB.GetCreditRqstInfo(GeoID);
        return DS;

This allocated memory, only to instantly overwrite it with the value returned from the static method. This whole thing could be simplified down to a single line.

return CreditInfDB.GetCreditRqstInfo(GeoID);

Now, it's tempting to get rid of the BAL when it's just passing through like this, and eventually you might, but for now, leave it in place. It'll give you some wiggle room while you're replacing that abomination of a data layer with something modern.

Next thing to go is the static declaration on all of the BAL methods. Just remove it (one at a time!), then compile. Follow the errors until you've instantiated instances of the class everywhere you need it. This is called Leaning on the compiler.

Once that's complete, you'll need to stop returning those DataSets from the BAL. This will be by far the most tedious and painful part, but well worth the effort. Start modeling the data and creating POCO's to be returned from the BAL. Databindings client side will need to be updated. This means invasive changes in your views. Use the BAL to map the DataSets coming out of the DAL to your POCOs.

For example:

public class CreditMgr
{
   public IEnumerable GetCreditRqstInfo(String GeoID)
    {
        DataSet ds = CreditIntfDB.GetCreditRqstInfo(GeoID);
        return ds.ToCreditRequestInfo();
    }
    //...
}

Where ToCreditRequestInfo() is an extension method that transforms a dataset into a list of your model class.

Now you're in a position to extract an interface for the BAL so it can faked during testing.

Finally, you'll be able to think about ditching the DAL entirely by replacing the guts of the BAL with calls to an ORM like Entity Framework.

public class CreditManager : ICreditManager
{
   public IEnumerable GetCreditRqstInfo(String GeoID)
    {
        using (var dbContext = new CreditDbContext())
        {
            return dbContext.GetCreditRequestInfo(GeoID).ToList();
         }
    }

    //...
}

This is not for the faint of heart, and I wouldn't want to tackle it without an excellent refactoring tool like ReSharper and a copy of Working Effectively with Legacy Code by Michael Feathers nearby.

Best of luck. Remember, keep your changes small and safe. Don't change logic if you can avoid it and drive as many tests as you can at the code before changing it. (Although, you'll have to make a few changes to make it testable first... So... Slow and steady friend. Slow and steady.)

StackExchange Code Review Q#132126, answer score: 5