HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMajor

Is decoupling necessary for very small applications?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
necessaryapplicationsdecouplingsmallforvery

Problem

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_c

Solution

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.

`

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().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 "thing" with injected IContextManagerFactory (`) 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.

Code Snippets

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();
            }
        }
    }

Context

StackExchange Code Review Q#42023, answer score: 25

Revisions (0)

No revisions yet.