Good way to define a method

Question

What is the best / good way to implement method calls.

For eg: From the below which is generally considered as best practice. If both are bad, then what is considered as best practice.

Option 1 :

   private void BtnPostUpdate_Click(object sender, EventArgs e)
    {
        getValue();
    }

    private void getValue()
    {
        String FileName = TbxFileName.Text;
        int PageNo = Convert.ToInt32(TbxPageNo.Text);

        // get value from Business Layer
        DataTable l_dtbl = m_BLL.getValue(FileName, PageNo);

        if (l_dtbl.Rows.Count == 1)
        {
            TbxValue.Text = Convert.ToInt32(l_dtbl.Rows[0]["Value"]);
        }
        else
        {
            TbxValue.Text = 0;
        }
    }

Option 2 :

    private void BtnPostUpdate_Click(object sender, EventArgs e)
    {
        String FileName = TbxFileName.Text;
        int PageNo = Convert.ToInt32(TbxPageNo.Text);

        int Value = getValue(FileName, PageNo);

        TbxValue.Text = Value.ToString();

    }

    private int getValue(string FileName, int PageNo)
    {
        // get value from Business Layer
        DataTable l_dtbl = m_BLL.getValue(FileName, PageNo);

        if (l_dtbl.Rows.Count == 1)
        {
            return Convert.ToInt32(l_dtbl.Rows[0]["Value"]);
        }
        return 0;
    }

I understand we can pass parameters directly without assigning to a local variable... My question is more about the method definition and the way it is handled.

Answer 1

According to your example, option 2 is the way to go. Option 1 knows about your form and how to display data on it, which violates the SRP.

Answer 2

here is what i like to to if i don't implement mvc. and i'm assuming web here.

I'd do option 2 first but instead of having the buttons code set the text id create a property to set the text boxs value.

I do this because if something else sets the textbox value then you are going to duplicate code. bad if you change a name or control type.

Answer 3

If you're subscribing to the event automatically, I don't think it's particularly bad to have a method with the event handler signature which just delegates to a method which has the "real" signature you need (in this case, no parameters).

If you're subscribing manually, you can use a lambda expression instead:

postUpdateButton.Click += (sender, args) => PostUpdate();

and then do the work in PostUpdate. Whether you then split up the PostUpdate into two methods, one to deal with the UI interaction and one to deal with the BLL interaction is up to you. In this case I don't think it matters too much.

How you structure UI logic to make it testable is a whole different matter though. I've recently become a fan of the MVVM pattern, but I don't know how applicable that would be to your particular scenario (it's really designed around Silverlight and WPF).

A couple of other comments though:

• Conventionally, parameters should be camelCased, not PascalCased
• Do you genuinely believe you're getting benefit from prefixing local variables with l_? Isn't it obvious that they're local? Personally I'm not keen on most of the variable names shown here - consider naming variables after their meaning rather than their type.
• Using a DataTable to return information is a somewhat error-prone way of doing things. Why can the BLL not return an int? to indicate the value (or a lack of value)?