Is decoupling necessary for very small applications?

Question

I threw together a small solution for my organization today for some basic data review and approval procedures. This particular application will likely not change or add functionality at any time.

My question as I was coding the application was, does this need to be decoupled? Are there any opportunities for a Factory Pattern or Dependency Injection? Is following those patterns just adding too much to a program that doesn't really need it? (YAGNI?)

I kept coming to the conclusion that adding any of the above would be overkill, but I am a Lone Ranger developer at my company so I thought I would get the opinion of the community at large.

The application consists of two domain level classes and a Form.

MAIN FORM

using System;
using System.Drawing;
using System.Linq;
using System.Windows.Forms;

namespace StockoutReasonReview
{
public partial class StockoutReasonReviewForm : Form
{
    private ReasonData ReasonData { get; set; }

    public StockoutReasonReviewForm()
    {
        InitializeComponent();
        this.ReasonData = new ReasonData();

        SetupDgv();
    }

    private void CloseButton_Click(object sender, EventArgs e)
    {
        this.ReasonData.ReasonTable.Dispose();
        this.Close();
    }

    private void SubmitButton_Click(object sender, EventArgs e)
    {
        this.ReasonData.Submit();
    }

    private void SetupDgv()
    {
        dgvReasons.DataSource = this.ReasonData.ReasonTable;

        dgvReasons.DefaultCellStyle.BackColor = Color.LightBlue;
        dgvReasons.AlternatingRowsDefaultCellStyle.BackColor = Color.LightCyan;
        dgvReasons.AutoSizeRowsMode = DataGridViewAutoSizeRowsMode.AllCells;

        var approvedColumn = new DataGridViewCheckBoxColumn
        {
            Name = "approved_check",
            DataPropertyName = "approved",
            TrueValue = "Y",
            FalseValue = "N"
        };

        var changeReasonColumn = new DataGridViewComboBoxColumn
        {
            Name = "approved_category_combo",
            DataPropertyName = "approved_category",
            DataSource = this.ReasonData.DropDownItems
        };


        if (dgvReasons.Columns["approved_check"] == null)
        {
            dgvReasons.Columns.Insert(0, approvedColumn);
        }

        if (dgvReasons.Columns["approved_category_combo"] == null)
        {
            dgvReasons.Columns.Insert(1, changeReasonColumn);
        }

        dgvReasons.Columns["note"].HeaderText = "Note";
        dgvReasons.Columns["note"].DefaultCellStyle.WrapMode = DataGridViewTriState.True;
        dgvReasons.Columns["note"].AutoSizeMode = DataGridViewAutoSizeColumnMode.Fill;

        dgvReasons.Columns["buyer_name"].HeaderText = "Buyer";
        dgvReasons.Columns["buyer_name"].Width = 80;

        dgvReasons.Columns["location_id"].HeaderText = "Loc";
        dgvReasons.Columns["location_id"].Width = 50;

        dgvReasons.Columns["item_id"].HeaderText = "Item Id";

        dgvReasons.Columns["reason_category"].HeaderText = "Original Category";

        dgvReasons.Columns["approved_category_combo"].HeaderText = "Approved Category";
        dgvReasons.Columns["approved_category_combo"].Width = 125;

        dgvReasons.Columns["approved_check"].HeaderText = "Approve";
        dgvReasons.Columns["approved_check"].Width = 50;

        dgvReasons.Columns["stockout_reason_uid"].Visible = false;
        dgvReasons.Columns["date_created"].Visible = false;
        dgvReasons.Columns["buyer_id"].Visible = false;
        dgvReasons.Columns["date_last_modified"].Visible = false;
        dgvReasons.Columns["approved"].Visible = false;
        dgvReasons.Columns["approved_category"].Visible = false;

        foreach (var col in dgvReasons.Columns.Cast<DataGridViewColumn>().Where(col => !col.Name.Contains("approved")))
        {
            col.ReadOnly = true;
        }
    }
}
}

BUSINESS LOGIC

using System;
using System.Data;

namespace StockoutReasonReview
{
    class ReasonData
    {
        public DataTable ReasonTable { get; set; }
        public readonly string[] DropDownItems =
        {
            "",
            "New Item",
            "Supplier Issue",
            "Carrier Issue",
            "Didn't Order/Transfer in time",
            "Sales Spike",
            "Large Qty Order",
            "Other"
        }; 

        public ReasonData()
        {
            this.ReasonTable = new DatabaseTransaction().GetReasons();
            GenerateAdditionalColumns();
            PopulateCategories();
        }

        public void Submit()
        {
            new DatabaseTransaction().SubmitReasons(this.ReasonTable);
        }

        private void GenerateAdditionalColumns()
        {
            this.ReasonTable.Columns.Add("approved"); //Temp column to be used during approval process
            //this.ReasonTable.Columns.Add("approved_category"); //Testing Only - Need to add the column to the actual database after today's reviews are complete
        }

        private void PopulateCategories()
        {
            foreach (DataRow row in this.ReasonTable.Rows)
            {
                row["approved_category"] = row["approved_category"] == DBNull.Value
                    ? row["reason_category"]
                    : row["approved_category"];
            }
        }
    }
}

DATA ACCESS

using System;
using System.Data.SqlClient;
using System.Data;
using System.Linq;
using StockoutReasonReview.Properties;
using WPD_Common_Library;

namespace StockoutReasonReview
{
    public class DatabaseTransaction
    {
        public DataTable GetReasons()
        {
            using (var dt = new DataTable())
            using (var conn = new SqlConnection(ConnectionStrings.P21ConnectionString))
            using (var cmd = new SqlCommand(Resources.GetReasonString, conn))
            {
                conn.Open();
                dt.Load(cmd.ExecuteReader());
                return dt;
            }
        }

        public void SubmitReasons(DataTable dtSubmit)
        {
            foreach (var row in dtSubmit.Rows.Cast<DataRow>().Where(row => row["approved"].ToString() == "Y"))
            {
                using (var conn = new SqlConnection(ConnectionStrings.P21ConnectionString))
                using (var cmd = new SqlCommand(Resources.SubmitReasonString, conn))
                {
                    cmd.Parameters.AddWithValue("@Uid", row["stockout_reason_uid"]);
                    cmd.Parameters.AddWithValue("@Category", row["approved_category"]);
                    cmd.Parameters.AddWithValue("@Modified", DateTime.Now);

                    conn.Open();
                    cmd.ExecuteNonQuery();
                }
            }
        }

    }
}

Obviously I am open to other suggestions in a more general sense, but I am specifically focused on my question regarding further decoupling.

Answer 1

Is decoupling necessary for very small applications?

YES!!

However I'd correct your statement here:

Is following those patterns just adding too much to a program that doesn't really need it? (YAGNI?)

It's not so much about YAGNI, than it is about Cargo Cult Programming. However in practice, any small application has the potential to grow and spaghettify (I know, that's not quite a verb) beyond repair; building small applications with clean, testable code as a goal can be great practice for when the day comes where you need to work on a large system where untested/untestable code isn't an option.

</opinion>

---

Actual Review

Main Form

I think the name should be InitializeDataGridView(), but that's not the reason why I don't like the SetupDgv method. There has to be a way to leverage some databinding here, isn't there? Also I think ReasonData should be assigned from the outside, probably property-injected (since constructor injection would either not be pretty, or would break the designer).

Business Logic

I don't think it's normal to see new DatabaseTransaction() more than once, and even if it were only once, you've coupled your "business logic" with a very specific piece of code that the ReasonData class is instanciating at will.

Data Access

Your class is lying. When I first saw new DatabaseTransaction() I expected a database transaction to actually happen. But what you have here is really just a service class that works with boilerplate-level ADO.NET constructs... which is probably fine, but if this application grows you'll want to reduce the boilerplate clutter and perhaps use an ORM, like Entity Framework.. or at least Linq-to-SQL, the goal being to abstract all these IDisposable instances away.

I think SubmitReasons() can afford to use the same connection for all commands:

    using (var conn = new SqlConnection(ConnectionStrings.P21ConnectionString))
    {
        conn.Open();
        foreach (var row in dtSubmit.Rows.Cast<DataRow>().Where(row => row["approved"].ToString() == "Y"))
        {
            using (var cmd = new SqlCommand(Resources.SubmitReasonString, conn))
            {
                cmd.Parameters.AddWithValue("@Uid", row["stockout_reason_uid"]);
                cmd.Parameters.AddWithValue("@Category", row["approved_category"]);
                cmd.Parameters.AddWithValue("@Modified", DateTime.Now);

                cmd.ExecuteNonQuery();
            }
        }
    }

---

Bottom line, always strive to write clean code, regardless of app size. That doesn't mean to blindly implement a whole IRepository<TEntity> "thing" with injected IContextManagerFactory<TContext> (</sarcasm>) just for the heck of it! If ADO.NET suits your small app needs, it's a totally valid approach! However by writing your small apps in testable code (decoupled), you're making your own life easier, and you can always consider those as a "practice" for larger systems.

Answer 2

I think the answer to you question mainly lies in the level of faith you have in your statement that "this particular application will likely not change or add functionality at any time." In my experience, its only a matter of time before somebody asks "wouldn't it be nice if..." or the business requirements that drive the application change. That said, it doesn't look like there is a lot of immediate gain in decoupling this much further at this point. There may be in the future, but if it does the job right now the addition effort in refactoring might be a little bit premature. I would ask yourself how many use cases there conceivably are at this point - if this is basically just a one-off office automation type project I'm guessing there aren't that many right now.

That said, what has bitten me on projects like this in the past are "simple" changes that are easy to implement but require the source to be recompiled and redeployed. This makes the following code section leap out:

public readonly string[] DropDownItems =
{
    "",
    "New Item",
    "Supplier Issue",
    "Carrier Issue",
    "Didn't Order/Transfer in time",
    "Sales Spike",
    "Large Qty Order",
    "Other"
}; 

This is a maintenance hassle waiting to happen, because every manager that I've ever worked with would ask at some point for another item to be added to this list. I would be to pull these items out of a table in the database (if the DB is normalized they should be in a separate table anyway).

Answer 3

No - decoupling is not necessary at this stage. The very fact that you have doubts about its usefulness indicates that your application will work fine as it is.

Instead, spend the effort on some good automated tests that will support you if the need comes in the future to refactor to a more complex implementation.

Don't forget that every line of code you write brings with it future maintenance costs. By adding unnecessary complexity at this stage, it is quite possible that the overall cost is greater than if you only add the complexity when it's needed. Furthermore, if you ever actually get a use-case that justifies the complexity, it will come with requirements that allow you to do a better job of deciding what complexity to add.

Trust your own judgement on this... or you could just use a Factory-Factory-Aggregation-Disposer-Delegate-Handler ... that should work.

Answer 4

Mainly not about decoupling:

I agree with the idea that SubmitReasons should reuse the same connection, but it should possibly even set up a new SqlTransaction that includes all ExecuteNonQuery calls. I would go further and re-use the same SqlCommand, prepare the statement and re-use the same parameters.

public void SubmitReasons(DataTable dtSubmit)
{
    using (var conn = new SqlConnection(ConnectionStrings.P21ConnectionString))
    {
        conn.Open();
        using (var transaction = conn.BeginTransaction())
        using (var cmd = new SqlCommand(Resources.SubmitReasonString, conn, transaction))
        {
            // I assume that ids are ints and that a category is referenced by an id.
            var uid = cmd.Parameters.Add("@Uid", SqlDbType.Int);
            var category = cmd.Parameters.Add("@Category", SqlDbType.Int);
            var modified = cmd.Parameters.Add("@Modified", SqlDbType.DateTime2);

            cmd.Prepare();

            foreach (var row in dtSubmit.Rows.Cast<DataRow>().Where(row => row.Field<string>("approved") == "Y"))
            {
                uid.Value = row.Field<int>("stockout_reason_uid");
                category.Value = row.Field<int>("approved_category");
                modified.Value = DateTime.Now;

                cmd.ExecuteNonQuery();
            }

            transaction.Commit();
        }
    }
}

What I still do not like about the design is that you leak fake booleans into your data layer by using "Y/N" instead of true/false. I really do not see why using a string would have any benefit over a real bool, especially since the actual value is not event surfaced to the user. Having a == "Y" check in a data access method should be avoided IMHO.

ReasonData should be the class in charge of disposing it's ReasonTable if you want it to be disposed (in general, that does not seem to be necessary). Also, there is currently no reason to have that set accessor to be public.

class ReasonData : IDisposable
{
    public DataTable ReasonTable { get; private set; }
    // …    
    public ReasonData()
    {
        this.ReasonTable = new DatabaseTransaction().GetReasons();
        GenerateAdditionalColumns();
        PopulateCategories();
    }

    public void Dispose()
    {
        this.Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing && this.ReasonTable != null)
        {
            this.ReasonTable.Dispose();
        }
     }

     // …
    }
}

Do not dispose ReasonData.ReasonTable or ReasonData in CloseButton_Click, but in the form's Dispose method.

You should not wrap the returned DataTable in GetReasons in a using block. Even though that probably does not do anything, you are actually returning a fully disposed object, so it is not really obvious that the dt object is supposed to be used after it is returned.