7
\$\begingroup\$

Both functions look fairly similar to each other, but they are different in the type of string. I have 6 functions like this and all differ only in the "string cmd". Any ideas on how I can put it all into one?

public void getFrmSingProgTbl(int flg)
    {
        if (flg == 1)
        {
            object[] astatus = new object[1];
            astatus = td.CheckOk(td.t1snd);
            string status = (string)astatus[0];
            if (status  == "true")
            {
                string cmd = getCurrTime() + "|" + "part-type" + "|" + t1prtNameTxt.Text + "\n";
                cmd += getCurrTime() + "|" + "operation-type" + "|" + t1oprNameTxt.Text + "\n";

                string showCmd = "Part Type - " + t1prtNameTxt.Text + "\nOperation - " + t1oprNameTxt.Text;
                if (MessageBox.Show(showCmd, "Confirm", MessageBoxButtons.YesNo) == DialogResult.Yes)
                {
                    HandleAfterSend(td.t1snd, td.t1stp, flg, td.t1btn);
                    sndData(cmd,1);
                }                   
            }
            else
            {
                this.ActiveControl = (TextBox)astatus[1];
                MessageBox.Show(status, "Error");
            }
        }
        else if (flg == 2)
        {
            object[] astatus = new object[1];
            astatus = td.CheckOk(td.t1stp);
            string status = (string)astatus[0];

            if (status == "true")
            {
                string cmd = getCurrTime() + "|" + "part-count-good" + "|" + t1gpTxt.Text + "\n";
                cmd += getCurrTime() + "|" + "part-count-bad" + "|" + t1bpTxt.Text + "\n";
                string showCmd = "Good Parts - " + t1gpTxt.Text + "\nBad Parts - " + t1bpTxt.Text;
                if (td.CheckNumeric(td.t1stp))
                {
                    if (MessageBox.Show(showCmd, "Confirm", MessageBoxButtons.YesNo) == DialogResult.Yes)
                    {
                        HandleAfterSend(td.t1stp, td.t1snd, flg, td.t1btn);
                        sndData(cmd,0);
                    }
                }
                else
                {
                    MessageBox.Show("Enter only numbers for Part Count","Error");
                }
            }
            else
            {
                this.ActiveControl = (TextBox)astatus[1];
                MessageBox.Show(status, "Error");
            }
        }
    }

public void getFrmSingFixtTbl(int flg)
    {
        if (flg == 1)
        {
            object[] astatus = new object[1];
            astatus = td.CheckOk(td.t3snd);
            string status = (string)astatus[0];
            if (status == "true")
            {
                string cmd = getCurrTime() + "|" + "part-type" + "|" + t3prtNameTxt.Text + "\n";
                cmd += getCurrTime() + "|" + "operation-type" + "|" + t3oprNameTxt.Text + "\n";
                cmd += getCurrTime() + "|" + "fixture-positions" + "|" + t3fixPosnTxt.Text + "\n";
                string showCmd = "Part Type" + " - " + t3prtNameTxt.Text + "\n";
                showCmd += "Operation" + " - " + t3oprNameTxt.Text + "\n";
                showCmd += "Parts per Fixture " + " - " + t3fixPosnTxt.Text + "\n";
                if (MessageBox.Show(showCmd, "Confirm", MessageBoxButtons.YesNo) == DialogResult.Yes)
                {
                    HandleAfterSend(td.t3snd, td.t3stp, flg, td.t3btn);
                    sndData(cmd,1);
                }
            }
            else
            {
                this.ActiveControl = (TextBox)astatus[1];
                MessageBox.Show(status, "Error");
            }
        }
        else if (flg == 2)
        {
            object[] astatus = new object[1];
            astatus = td.CheckOk(td.t3stp);
            string status = (string)astatus[0];
            if (status == "true")
            {
                string cmd = getCurrTime() + "|" + "part-count-good" + "|" + t3gpTxt.Text + "\n";
                cmd += getCurrTime() + "|" + "part-count-bad" + "|" + t3bpTxt.Text + "\n";
                string showCmd = "Good Parts" + " - " + t3gpTxt.Text + "\n";
                showCmd += "Bad Parts" + " - " + t3bpTxt.Text + "\n";
                if (td.CheckNumeric(td.t3stp))
                {
                    if (MessageBox.Show(showCmd, "Confirm", MessageBoxButtons.YesNo) == DialogResult.Yes)
                    {
                        HandleAfterSend(td.t3stp, td.t3snd, flg, td.t3btn);
                        sndData(cmd,0);
                    }
                }
                else
                {
                    MessageBox.Show("Enter only numbers for Part Count","Error");
                }
            }
            else
            {
                this.ActiveControl = (TextBox)astatus[1];
                MessageBox.Show(status, "Error");
            }
        }
    }
\$\endgroup\$
2
  • \$\begingroup\$ How much control do you have over the structure of the td object's type? \$\endgroup\$ – Brian Reichle Apr 29 '11 at 23:53
  • 1
    \$\begingroup\$ Please only state the code purpose in the title. I downvoted for the meaningless title \$\endgroup\$ – Caridorc May 4 '15 at 17:34
15
\$\begingroup\$

Quick thoughts off the top of my head.

  1. Use the standard .Net naming convention and don't abbreviate
  2. Don't change the whole processing of a method by a flag
  3. If you control td.CheckOk(...) why does it return an array when it isn't used that way? Also, it should return a bool rather than a string "true". This should return a specific type rather than a bunch of data in various array indexes.
  4. Why are you creating a new array astatus = new object[1]; and then immediately replacing it with the value from td.CheckOk?
  5. Use StringBuilder rather than lots of string concats
  6. Seems like you could replace each function with (ignoring all the changes above):

    public void GetResult(Func<string> generateCommand, Func<string> generateSendData) {
    object[] astatus = td.CheckOk(td.t1snd);
    if (astatus != null && astatus.Length > 0 && (string)astatus[0] == "true") {
        string cmd = generateCommand();
        string data = generateSendData();
        if (MessageBox.Show(showCmd, "Confirm", MessageBoxButtons.YesNo) == DialogResult.Yes) {
            HandleAfterSend(td.t1snd, td.t1stp, flg, td.t1btn);
            sndData(cmd,1);
        } 
    } else {
        this.ActiveControl = (TextBox)astatus[1];
        MessageBox.Show(astatus[0].ToString(), "Error");
    }
    }
    

And then call it with:

GetResult(
    () => getCurrTime() + "|" + "part-type" + "|" + t1prtNameTxt.Text + "\n" + getCurrTime() + "|" + "operation-type" + "|" + t1oprNameTxt.Text + "\n",
    () => "Part Type - " + t1prtNameTxt.Text + "\nOperation - " + t1oprNameTxt.Text
);
GetResult(
    () => getCurrTime() + "|" + "part-count-good" + "|" + t1gpTxt.Text + "\n" + getCurrTime() + "|" + "part-count-bad" + "|" + t1bpTxt.Text + "\n",
    () => "Good Parts - " + t1gpTxt.Text + "\nBad Parts - " + t1bpTxt.Text
);

//etc
\$\endgroup\$
14
\$\begingroup\$
  1. Do not use int for flag. Use bool or enum
  2. Do not use string for status. Use bool
  3. Do not use object[] for astatus. Write own class for that
  4. Rewrite everything keeping first 3 points in mind
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Especially the 1, 2, 3 and 4. But it could be nice if you explain the reason for those, isn't? \$\endgroup\$ – Luca May 2 '11 at 19:59
5
\$\begingroup\$

To add to akmad's answer you can simplify your string handling by using a string mask and the String.Format() function.

For example:

string cmd = getCurrTime() + "|" + "part-count-good" + "|" + t1gpTxt.Text + "\n";
cmd += getCurrTime() + "|" + "part-count-bad" + "|" + t1bpTxt.Text + "\n";
string showCmd = "Good Parts - " + t1gpTxt.Text + "\nBad Parts - " + t1bpTxt.Text;

could be replaced with:

string cmdFormat = "{0}|part-count-good|{1}\n{0}|part-count-bad|{2}\n";
string cmd = String.format(cmdFormat, getCurrTime(), tlgp.Text, tlbp.Text);

string showFormat = "Good Parts - {0}\nBad Parts - {1}";
string showCmd = String.Format(showFormat, tlgpTxt.Text, tlbpTxt.Text);
\$\endgroup\$
5
\$\begingroup\$

Method names that start with "Get" just sound wrong for methods that return void.

Depending on how these methods are used, you may be able to extract them to command objects with the common portion implemented in a common base class and the differences further extracted to abstract methods.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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