Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
add comment

3 Answers

up vote 18 down vote accepted

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.

share|improve this answer
    
Great Answer! Thank you so much! The majority of your suggestions are clear, but can you expand a bit on "There has to be a way to leverage some data binding"? The SetupDGV() method sets hard coded values for the grid, is there a better place to do this? Thanks! –  Evan L Feb 19 at 14:52
    
@EvanL Thanks! I'm not very familiar with data binding in WinForms, but I'm sure there's a way to just set the grid's DataSource to some IBindingList, so you'd get all the columns for free... but this might be just me getting used to WPF's data bindings, anyway check this out: stackoverflow.com/questions/16695885/… –  Mat's Mug Feb 19 at 15:24
    
Unfortunately I think we are getting WPF mixed up with WinForms. In my case the columns are a result of the columns in the database, I am just setting header text and size properties on each, as well as making several read only. I'm not actually adding any columns except for the custom controls. –  Evan L Feb 19 at 15:33
    
Final bit of curiosity. You mentioned cargo cult programming, were you saying that my code seems to follow this paradigm? Or that deciding to implement further decoupling elements could potentially lead to that? If the former, what makes you think that? –  Evan L Feb 19 at 15:39
1  
I mentioned Cargo Cult Programming because you mentioned YAGNI, referring to further refinement of the code. It's the trap one can fall into when implementing design patterns for the sake of implementing design patterns - can lead to having a Factory-Factory-Aggregation-Disposer-Delegate-Handler... I didn't imply that's necessarily what you would end up with though :) –  Mat's Mug Feb 19 at 15:51
add comment

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

share|improve this answer
2  
+1 for no other reason that using the word "hassle" ...ok for sportsmanship, too :) –  Mat's Mug Feb 19 at 1:43
1  
Funny thing? When I walked into the office this morning, the manager said: "I think it would be nice to add more categories" ;) –  Evan L Feb 19 at 14:55
add comment

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.

share|improve this answer
    
Lol at the Factory-Facotry-Aggreation-Disposer-Delegate-Handler ;) –  Evan L Feb 19 at 14:54
1  
spend the effort on some good automated tests - this is exactly where low coupling / high cohesion, DI and IoC come into play. Not all "works fine" code is testable. With the business logic tightly coupled with the data access, in my opinion "No - decoupling is not necessary at this stage" contradicts the idea of spending the effort on writing some good automated tests. –  Mat's Mug Feb 19 at 15:58
1  
It's a fair point Mat's Mug - decoupling is not justified for the code itself, but may be helpful for testability. In that case, writing the tests might lead to the refactoring - and yes - I accept that you probably want to design for testing in the first place, but then you have something that triggers the need for the extra complexity. –  Dominic Cronin Feb 19 at 16:27
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.