1
\$\begingroup\$

I have the following BLL (Business Logic Layer) class. It is the first class I have created that takes away all responsibility from the UI (where this class is instantiated/created/called) in terms of checking against business rules. My aim for this code is to be reusable and flexible in instances where I need to add a control or field and insert into the database.

public class MillInsController
    private MillInspectionTableAdapter _millDt = null;

    protected MillInspectionTableAdapter Adapter
    {
        get
        {
            if (_millDt == null)

                _millDt = new MillInspectionTableAdapter();
            _millDt.ConnectionString = Login.LoginController.UserConnString;
            return _millDt;
        }
    }

    public static void InitMillForm(Form millform)
    {
        switch (DateTime.Now.Year)
        {
            case 2016:
                millform.Controls["txtPrefEnd"].Text = "ES";
                millform.Controls["txtPrefStart"].Text = "ES";
                break;
            case 2017:
                millform.Controls["txtPrefEnd"].Text = "ET";
                millform.Controls["txtPrefStart"].Text = "ET";
                break;
            case 2018:
                millform.Controls["txtPrefEnd"].Text = "EU";
                millform.Controls["txtPrefStart"].Text = "EU";
                break;
            case 2019:
                millform.Controls["txtPrefEnd"].Text = "EV";
                millform.Controls["txtPrefStart"].Text = "EV";
                break;
            case 2020:
                millform.Controls["txtPrefEnd"].Text = "EW";
                millform.Controls["txtPrefStart"].Text = "EW";
                break;
            case 2021:
                millform.Controls["txtPrefEnd"].Text = "EX";
                millform.Controls["txtPrefStart"].Text = "EX";
                break;
            case 2022:
                millform.Controls["txtPrefEnd"].Text = "EY";
                millform.Controls["txtPrefStart"].Text = "EY";
                break;
            case 2023:
                millform.Controls["txtPrefEnd"].Text = "EZ";
                millform.Controls["txtPrefStart"].Text = "EZ";
                break;

        }

    }

    public DialogResult InsertMillData(Form form)
    {
        string inValue = "";
        bool TransactionStat = true;
        ComboBox cbCoil = form.Controls["cbCoilNumber"] as ComboBox;
        ComboBox cbShift = form.Controls["cbShift"] as ComboBox;

        string JobNumber = form.Controls["txtJobNumber"].Text;
        string PipeSize = form.Controls["txtDiameter"].Text + "X" + form.Controls["txtThickness"].Text + "X" + form.Controls["txtLength"].Text;
        string HeatNumber = form.Controls["txtHeatNumber"].Text;
        string CoilNumber = cbCoil.SelectedItem.ToString();
        string pipePrefix = form.Controls["txtPrefStart"].Text;
        string PipeNumberStart = form.Controls["txtPipeNumberStart"].Text;
        string PipeNumberEnd = form.Controls["txtPipeNumberEnd"].Text;
        string Length = form.Controls["txtLength"].Text;
        string Thickness = form.Controls["txtThickness"].Text;
        string Diameter = form.Controls["txtDiameter"].Text;
        string Mass = form.Controls["txtMass"].Text;
        string SNo = form.Controls["txtSNo"].Text;
        string SheetNumber = form.Controls["txtSheetNumber"].Text;
        string SteelGrade = form.Controls["txtSteelGrade"].Text;
        string Operator = form.Controls["txtOperator"].Text;

        InputBox("Coil Number", "Enter Coil Number used for Sheet:" + form.Controls["txtSheetNumber"].Text, ref inValue);
        if (inValue != cbCoil.SelectedItem.ToString().Trim())
        {
            TransactionStat = false;
            return MessageBox.Show("ERROR! Please check Coil Number entered for Sheet.");

        }
        else
        {
            if ((pipePrefix + PipeNumberStart).Length < 8 || (pipePrefix + PipeNumberEnd).Length < 8)
            {
                TransactionStat = false;
                return MessageBox.Show("Pipe Number range incorrect format.");
            }

            foreach (Control controls in form.Controls)
            {
                if (controls is TextBox)
                {
                    if (String.IsNullOrWhiteSpace(controls.Text))
                    {
                        controls.BackColor = Color.Red;
                        TransactionStat = false;
                    }
                    else
                    {
                        controls.BackColor = Color.White;
                    }
                }
                if (controls is ComboBox)
                {
                    var cb = controls as ComboBox;
                    if (String.IsNullOrWhiteSpace(cb.SelectedItem.ToString()))
                    {
                        cb.BackColor = Color.Red;
                        TransactionStat = false;
                    }
                }
            }

            if (TransactionStat == false)
            {
                return MessageBox.Show("PLEASE PROVIDE ALL REQUIRED FIELDS, CHECK HIGHLIGHTED (RED) FIELDS TO CORRECT THE ERROR. CAPTURE ABORTED.", "ERROR OCCURED");
            }
            else
            {
                List<string> pipes = new List<string>();

                int start = Int32.Parse(PipeNumberStart);
                int end = Int32.Parse(PipeNumberEnd);
                int endlen = end.ToString().Length;

                while (start <= end)
                {
                    int startlen = start.ToString().Length;
                    pipes.Add(pipePrefix.PadRight(8 - startlen, '0') + start);
                    start++;
                }
                MillInsController millinsctrl;
                millinsctrl = new MillInsController();
                //INSERT MILL 
                foreach (var pipe in pipes)
                {
                    Adapter.Insert(
                        Mass, Diameter, Thickness, SNo, JobNumber,
                       SheetNumber, SteelGrade, cbShift.SelectedItem.ToString(), 
                       Operator, HeatNumber, cbCoil.SelectedItem.ToString(), pipe, Length);
                }
                return MessageBox.Show("INSERT SUCCESSFULL");
            }
        }
    }

    public static DialogResult InputBox(string title, string promptText, ref string value)
    {
        Form form = new Form();
        Label label = new Label();
        TextBox textBox = new TextBox();
        Button buttonOk = new Button();
        Button buttonCancel = new Button();

        form.Text = title;
        label.Text = promptText;
        textBox.Text = value;

        buttonOk.Text = "OK";
        buttonCancel.Text = "Cancel";
        buttonOk.DialogResult = DialogResult.OK;
        buttonCancel.DialogResult = DialogResult.Cancel;

        label.SetBounds(9, 20, 372, 13);
        textBox.SetBounds(12, 36, 372, 20);
        buttonOk.SetBounds(228, 72, 75, 23);
        buttonCancel.SetBounds(309, 72, 75, 23);

        label.AutoSize = true;
        textBox.Anchor = textBox.Anchor | AnchorStyles.Right;
        buttonOk.Anchor = AnchorStyles.Bottom | AnchorStyles.Right;
        buttonCancel.Anchor = AnchorStyles.Bottom | AnchorStyles.Right;

        form.ClientSize = new Size(396, 107);
        form.Controls.AddRange(new Control[] { label, textBox, buttonOk, buttonCancel });
        form.ClientSize = new Size(Math.Max(300, label.Right + 10), form.ClientSize.Height);
        form.FormBorderStyle = FormBorderStyle.FixedDialog;
        form.StartPosition = FormStartPosition.CenterScreen;
        form.MinimizeBox = false;
        form.MaximizeBox = false;
        form.AcceptButton = buttonOk;
        form.CancelButton = buttonCancel;

        DialogResult dialogResult = form.ShowDialog();
        value = textBox.Text;
        return dialogResult;
    }


    public bool updateMill(string stat, string scrapl, string length, string pipenum)
    {
        ds_Mill.MillInspectionDataTable mill = Adapter.GetDataByPipe(pipenum);
        if (mill.Count == 0)
        {
            return false;
        }

        ds_Mill.MillInspectionRow millrw = mill[0];

        _millDt.Update(stat, scrapl, length, pipenum);

        int rowsAffected = Adapter.Update(millrw);

        // Return true if precisely one row was updated,
        // otherwise false
        return rowsAffected == 1;

    }

    public ds_Mill.MillInspectionDataTable getSheet(string sheet)
    {
        return Adapter.GetDataBySheetNum(sheet);
    }

    public System.Data.DataTable fillDataGridMillIns(string sheetnum)
    {
        try
        {
            MillInsController millIns = new MillInsController();
            return millIns.getSheet(sheetnum);

        }
        catch (SqlException ex)
        {
            MessageBox.Show(ex.ToString());
            return null;
        }
        catch (InvalidOperationException exOp)
        {
            MessageBox.Show(exOp.ToString());
            return null;
        }
    }
}
\$\endgroup\$

closed as off-topic by πάντα ῥεῖ, Toby Speight, IEatBagels, t3chb0t, Graipher Feb 27 at 10:46

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – πάντα ῥεῖ, t3chb0t, Graipher
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 4
    \$\begingroup\$ What does it actually do? "Business Logic" is a very vague description; if you can be more specific, you'll likely get better answers! \$\endgroup\$ – Toby Speight Feb 26 at 15:51
  • \$\begingroup\$ If you don't want to catch yourself guessing what this code is a decade from now, comment your code. (I find it easier to start typing each source code file with a description of what it shall be good for.) \$\endgroup\$ – greybeard Feb 26 at 18:22
  • \$\begingroup\$ Also, be prepared for this code to break in 2024... \$\endgroup\$ – Graipher Feb 27 at 10:46
3
\$\begingroup\$

The class is not a "business logic layer" class, because it is still deeply rooted in the user interface. You cannot execute the logic of this class without a valid form control, and therefore you need a user interface to execute the logic.

The whole point of a "business logic layer" is to centralize the logic that does not require the user interface into its own collection of classes.

What the MillInsController appears to behave like is a UserControl, which can be used to build a reusable user interface component. This is still a valid pattern, but not as a separate class. Usually a UserControl is created as a pairing of view + controller code that can be embedded within other user controls.

I really don't see much business logic in this class. It runs basic form validations (did you put something in the field) and does basic data conversions from strings to numeric types. Beyond that it calls to a data access layer.

Additional observations:

  • The PipeSize variable is declared and initialized, but never used.

  • The InitMillForm method could be cleaned up by using a dictionary of values to set the form fields where the key in the dictionary is the year, and the value could be the prefix:

    var year = DateTime.Now.Year;
    var preferences = new Dictionary<int, string>()
    {
        { 2016, "ES" },
        { 2017, "ET" },
        { 2018, "EU" },
        { 2019, "EV" },
        { 2020, "EW" },
        { 2021, "EX" },
        { 2022, "EY" },
        { 2023, "EZ" }
    };
    
    millform.Controls["txtPrefEnd"].Text = preferences[year];
    millform.Controls["txtPrefStart"].Text = preferences[year];
    
  • Inconsistent variable naming conventions. You will see camelCase used for local variables more than PascalCase (e.g. pipeSize instead of PipeSize)

  • Stringly typed code. When inserting a new ... whatever it is that you are inserting (inside the foreach (var pipe in pipes) loop) you are passing a bunch of string arguments. Consider creating a class to represent the thing you are inserting, and using proper data types. For instance, the Diameter should most likely be a decimal instead of a string.

  • Needless instantiation of MillInsController. The InsertMillData method creates a new instance of itself, and then never uses it. This code should be removed. I'm not sure why an instance method of a class would need to create another instance of itself, when it is already able to call instance methods on itself.

  • Refactor code in if statements. Code that checks conditions inside an if statement could be refactored into its own method, and given a proper name so you can come back to this code in 6 months and know that:

    if ((pipePrefix + PipeNumberStart).Length < 8 || (pipePrefix + PipeNumberEnd).Length < 8)
    

    Really means:

    if (IsInvalidPipeNumberRange(pipePrefix, PipeNumberStart, PipeNumberEnd))
    
    ...
    
    private bool IsInvalidPipeNumberRange(string pipePrefix, string pipeNumberStart, string pipeNumberEnd)
    {
        return (pipePrefix + pipeNumberStart).Length < 8
            || (pipePrefix + pipeNumberEnd).Length < 8
    }
    
\$\endgroup\$
2
\$\begingroup\$

I'm just gonna add some observations on top of already mentioned suggestions:

  • If there's any way you can incorporate automatic layouting and some sort of declarative UI, or use a UI builder, methods like InputBox might be a little bit easier: The tedious stuff should be done by the machine (well, the computer) really. Additionally that would in many cases also allow you to rescale windows and dialogs more easily while keeping proportions etc.

    That is, FlowLayoutPanel or TableLayoutPanel pop up in the search for me when I enter winforms, plus this post.

  • if (x == false) ... - that's just if (!x); ... == true - that's just ... itself, no need to compare to true or false.
  • The error messages don't seem to give the user a lot of context. Like, if there's already a check in place, then just tell the user what exactly went wrong in the check, otherwise they'll still have to consult with a manual, or the programmer, or a colleague? While the source of truth for what's actually being checked is right there!

    E.g. instead of saying ERROR! Please check Coil Number entered for Sheet. it could instead say something like ERROR! Input '%s' doesn't match Coil Number '%s' entered for Sheet. (with the appropriate parameters of course).

    Same for the Pipe Number range incorrect format. ... just tell me what I have to enter directly and where I went wrong. With the right library that could be auto-generated, but I suspect with just these few checks that'd be easy enough to do manually too.

  • The validation should be generalised if you have more of these dialogs. If not, then still, make that a separate method so that InsertMillData can all be read on one screen!
  • InitMillForm - how long exactly are you maintaining this? Maybe add a default case and show a helpful message if we're past 2023 ...
  • The comment in updateMill should be the docstring of the method. It's not useful where it is right now, I already know what it says by just reading the line below it and that like is much shorter. However, as a docstring for the method I'd then actually know what the contract for updateMill is. Well, part of it, I still have no idea what the parameters mean of course. That is to say, I value self-describing code ... but then you also have to make it more readable. E.g. scrapl, it wouldn't hurt to spend a few more characters to spell it out and make the reader immediately aware of what that parameter contains (apart from that it's a string, yes, but that was mentioned. Domain-specific data types are quite good for understanding and preventing errors because parameter order was accidentally flipped etc.).
  • There's also unused variables, just remove them.
  • TransactionStat is updated and never read when a return follows immediately after, just remove those too.
  • The pipePrefix check uses the magic number 8, as does the formatting a bit below, with PadRight - make that number a constant so those don't get out of sync.
  • fillDataGridMillIns - I trust that catching those exceptions is the most useful thing to do. The block can be simplified a bit if you're on C# 6 or higher.
\$\endgroup\$

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