2

Recently, I made a post about the developers I'm working with not using try catch blocks properly, and unfortuantely using try... catch blocks in critical situations and ignoring the exception error all together. causing me major heart ache. Here is an example of one of the several thousand sections of code that they did this (some code left out that doesn't particuarly matter:

public void AddLocations(BOLocation objBllLocations)
{
    try
    {
        dbManager.Open();
        if (objBllLocations.StateID != 0)
        {
             // about 20 Paramters added to dbManager here
        }
        else
        {
           // about 19 Paramters added here
        }
        dbManager.ExecuteNonQuery(CommandType.StoredProcedure, "ULOCATIONS.AddLocations");
    }
    catch (Exception ex)
    {
    }
    finally
    {
        dbManager.Dispose();
    }
}

This is absolutely discusting, in my eyes, and does not notify the user in case some potential problem occurred. I know many people say that OOP is evil, and that adding multiple layers adds to the number of lines of code and to the complexity of the program, leading to possible issues with code maintainence. Much of my programming background, I personally, have taken almost the same approach in this area. Below I have listed out a basic structure of the way I normally code in such a situation, and I've been doing this accross many languages in my career, but this particular code is in C#. But the code below is a good basic idea of how I use the Objects, it seems to work for me, but since this is a good source of some fairly inteligent programming mines, I'd like to know If I should re-evaluate this technique that I've used for so many years. Mainly, because, in the next few weeks, i'm going to be plunging into the not so good code from the outsourced developers and modifying huge sections of code. i'd like to do it as well as possible. sorry for the long code reference.

// *******************************************************************************************
/// <summary>
/// Summary description for BaseBusinessObject
/// </summary>
/// <remarks>
/// Base Class allowing me to do basic function on a Busines Object
/// </remarks>
public class BaseBusinessObject : Object, System.Runtime.Serialization.ISerializable
{
    public enum DBCode
    {   DBUnknownError,
        DBNotSaved,
        DBOK
    }

    // private fields, public properties
    public int m_id = -1;
    public int ID { get { return m_id; } set { m_id = value; } }
    private int m_errorCode = 0;
    public int ErrorCode { get { return m_errorCode; } set { m_errorCode = value; } }
    private string m_errorMsg = "";
    public string ErrorMessage { get { return m_errorMsg; } set { m_errorMsg = value; } }
    private Exception m_LastException = null;
    public Exception LastException { get { return m_LastException; } set { m_LastException = value;} }

    //Constructors
    public BaseBusinessObject()
    {
        Initialize();
    }
    public BaseBusinessObject(int iID)
    {
        Initialize();
        FillByID(iID);
    }
    // methods
    protected void Initialize()
    {
        Clear();
        Object_OnInit();
        // Other Initializable code here
    }
    public void ClearErrors()
    {
        m_errorCode  = 0; m_errorMsg = ""; m_LastException = null;
    }

    void System.Runtime.Serialization.ISerializable.GetObjectData(
         System.Runtime.Serialization.SerializationInfo info, 
        System.Runtime.Serialization.StreamingContext context)
    {
      //Serialization code for Object must be implemented here
    }
    // overrideable methods
    protected virtual void Object_OnInit()     
    {
        // User can override to add additional initialization stuff. 
    }
    public virtual BaseBusinessObject FillByID(int iID)
    {
        throw new NotImplementedException("method FillByID Must be implemented");
    }
    public virtual void Clear()
    {
        throw new NotImplementedException("method Clear Must be implemented");
    }
    public virtual DBCode Save()
    {
        throw new NotImplementedException("method Save Must be implemented");
    }
}
// *******************************************************************************************
/// <summary>
/// Example Class that might be based off of a Base Business Object
/// </summary>
/// <remarks>
/// Class for holding all the information about a Customer
/// </remarks>
public class BLLCustomer : BaseBusinessObject
{
    // ***************************************
    // put field members here other than the ID
    private string m_name = "";
    public string Name { get { return m_name; } set { m_name = value; } }
    public override void Clear()
    {
        m_id = -1;
        m_name = "";
    }
    public override BaseBusinessObject FillByID(int iID)
    {
        Clear();
        try
        {
            // usually accessing a DataLayerObject, 
            //to select a database record
        }
        catch (Exception Ex)
        {
            Clear();
            LastException = Ex;
            // I can have many different exception, this is usually an enum
            ErrorCode = 3;
            ErrorMessage = "Customer couldn't be loaded";
        }
        return this;
    }
    public override DBCode Save()
    {
        DBCode ret = DBCode.DBUnknownError;
        try
        {
            // usually accessing a DataLayerObject, 
            //to save a database record
            ret = DBCode.DBOK;
        }
        catch (Exception Ex)
        {
            LastException = Ex;
            // I can have many different exception, this is usually an enum
            // i do not usually use just a General Exeption
            ErrorCode = 3;
            ErrorMessage = "some really weird error happened, customer not saved";
            ret = DBCode.DBNotSaved;
        }
        return ret;
    }
}
// *******************************************************************************************
// Example of how it's used on an asp page.. 
    protected void Page_Load(object sender, EventArgs e)
    {
        // Simplifying this a bit, normally, I'd use something like, 
        // using some sort of static "factory" method
        // BaseObject.NewBusinessObject(typeof(BLLCustomer)).FillByID(34);
        BLLCustomer cust = ((BLLCustomer)new BLLCustomer()).FillByID(34);
        if (cust.ErrorCode != 0)
        {
            // There was an error.. Error message is in 
            //cust.ErrorMessage
            // some sort of internal error code is in
            //cust.ErrorCode

            // Give the users some sort of message through and asp:Label.. 
            // probably based off of cust.ErrorMessage
            //log can be handled in the data, business layer... or whatever
            lab.ErrorText = cust.ErrorMessage;
        }
        else
        {
            // continue using the object, to fill in text boxes, 
            // literals or whatever. 
            this.labID = cust.ID.toString();
            this.labCompName = cust.Name;
        }
    }

Bottom line, my question is, Am I over complicating things with the muliple layers, and the inherited classes or is my old concept illustrated still working good and stable? Is there a better way now a days to accomplish these things? Should I go to just making straight SQL calls from the asp.net page code behind pages as fellow work associate developer suggested (though that last solution makes me feel icky), instead of going through a business object, and data layer (data layer not shown, but basically holds all the stored proc calls). Yeah, another developer did ask me why i go through the effort of layering things, when you can just type what you need straight in a *.aspx.cs code behind page, and then I can have the joys of over 1k lines of code behind. What is some advice here?

Community
  • 1
  • 1
stephenbayer
  • 12,373
  • 15
  • 63
  • 98

6 Answers6

1

Have you considered using an ORM like NHibernate? There's no point in re-inventing the wheel.

To me this is a code smell:

BLLCustomer cust = ((BLLCustomer)new BLLCustomer()).FillByID(34);

Too many brackets!

I've found that using the active record pattern in a language like C# always ends in tears because it's hard(er) to unit test.

jonnii
  • 28,019
  • 8
  • 80
  • 108
  • I'm sorry. :( I did a lot of lisp programming back in the day, and it's influence shows through in other languages. – stephenbayer Oct 27 '08 at 12:44
1

The jump from the first bit of code to the next is huge. Whether a complicated business object layer is necessary will depend on the size of the app in question. At the very least though our policy is that exceptions are logged where they are handled. How you present to the user is up to you but having logs is essential so that developers can get more information if necessary.

Luke
  • 18,585
  • 24
  • 87
  • 110
0

Why not just catch the exception in the Page_Load event? Some exception you might expect and know how to deal with, other exceptions should be handled by a global exception handler.

Magnus Lindhe
  • 7,157
  • 5
  • 48
  • 60
0

My rule of thumb is to only catch errors that I can handle or give the user something useful so that if they do whatever it was they did again, it is likely to work for them. I catch database exceptions; but only to add some more information onto the error about the data being used; then I re-throw it. The best way to handle errors in general is to not catch them at all anywhere but at the top of the UI stack. Just having one page to handle the errors and using the global.asax to route to it handles almost all situations. Using status codes is definitely out of style all together. It is a remnant of COM.

DancesWithBamboo
  • 4,176
  • 1
  • 19
  • 20
0

Is it possible to use an abstract base class instead of a concrete class? this would force the implementation of your methods at development time rather than runtime exceptions.

The best comment here to agree with is from dances, where you should only handle exceptions you can recover from at that point. Catching others and rethrowing is the best method (although i think its rarely done). Also, make sure they are logged.... :)

Simon P
  • 1,196
  • 1
  • 12
  • 26
0

YOur way of errorhandling seems very outdated. Just make a new exeption and inherit from exeption that way you have the callstack at least. Then you should log with something like nlog or log4net. And this is the year 2008 so use generics. You will have to do a lot less casting that way.

ANd use an ORM like someone said before. Don't try to reinvent the wheel.

chrissie1
  • 5,014
  • 3
  • 26
  • 26