2
\$\begingroup\$

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.

  1. Same exact method names in DAL and BAL
  2. Static is EVERYWHERE
  3. 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?

\$\endgroup\$
  • \$\begingroup\$ Please remove the hold as this is code I need to modify and maintain and create new methods with same type of pattern, but I wish to improve about this type of code I'm working on currently. thx \$\endgroup\$ – Jeremy Miller Jun 15 '16 at 22:32
  • \$\begingroup\$ We don't review code written by anyone else, unless you're a maintainer of it. \$\endgroup\$ – Jamal Jun 15 '16 at 22:38
  • 1
    \$\begingroup\$ 1. I maintain / took over 2. I'm asked to create new methods extremely similar to those I mentioned and thus I asked the question as I do not feel that the DAL / BAL should have the same signatures nor objective \$\endgroup\$ – Jeremy Miller Jun 15 '16 at 23:13
  • \$\begingroup\$ It’s been a few years. I’m curious how this ended up for you. Were you able to make improvements? \$\endgroup\$ – RubberDuck Dec 8 '18 at 13:05
5
\$\begingroup\$

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<CreditRequestInfo> 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<CreditRequestInfo> 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.)

\$\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.