2
\$\begingroup\$

I am designing a program that handles many types of grooves TKY, VSingle under GrooveType.

Each of this groove is paired with its specific parameter SimulationParam (e.g. TKY will pair with SimulationParamTKY, which inherits from SimulationParam) under the Dictionary simParams. As such, if i want to include more types of grooves, I can add it under this dictionary.

SimulationParam contains ProcessParam, which has the parameters probeStartAngle and probeStartDist. These parameters are similar(in meaning) for both TKY and VSingle.

However, in the dialog, I want the user to be able to select which particular groove they want. When that particular option is selected, the respective values for probeStartAngle and probeStartDist is read in from their respective text box. For example, if Use TKY is selected, the ProcessParam.probeStartAngle value will be read from tbTKYStartAngle.

As such, when I extend this program to even more grooves (say 10 more), under btnProcess_Click(), a lot of code will actually be repeated.

Is there a better way to write this code to reduce the repetitions? Assuming the requirement is that the textboxes for each groove cannot be merged into a single one. (i.e. meaning i cannot read from the same textbox for say ProcessParam.probeStartAngle)

enter image description here

public partial class GrooveDialog : Form
    {
        public enum GrooveType : int
        {
            TKY = 0,
            VSingle = 1,
        }

        Dictionary<GrooveType, SimulationParam> simParams = new Dictionary<GrooveType, SimulationParam>();

        public GrooveDialog()
        {
            simParams.Add(GrooveType.TKY, new SimulationParamTKY());
            simParams.Add(GrooveType.VSingle, new SimulationParamVSingle());
        }

        private void btnProcess_Click(object sender, EventArgs e)
        {
            if (rbUseTKY.Checked)
            {
                var simParamTKY = (SimulationParamTKY)simParams[GrooveType.TKY];

                simParamTKY.processParam.probeStartAngle = Convert.ToDouble(tbTKYAngle.Text);
                simParamTKY.processParam.probeStartDist = Convert.ToDouble(tbTKYDist.Text);
            }
            else if (rbUseVSingle.Checked)
            {
                var simParamVSingle = (SimulationParamVSingle)simParams[GrooveType.VSingle];

                simParamVSingle.processParam.probeStartAngle = Convert.ToDouble(tbVSingleAngle.Text);
                simParamVSingle.processParam.probeStartDist = Convert.ToDouble(tbVSingleDist.Text);
            }
    }

public class ProcessParam 
    {
        public ProcessParam() { }
        public double probeStartAngle { get; set; }
        public double probeStartDist { get; set; }
    }

    public class SimulationParam
    {
        public ProcessParam processParam { get; set; }
    }

    public class SimulationParamTKY : SimulationParam
    {
        public double alpha, theta;

        public SimulationParamTKY()
        {
            this.processParam = new ProcessParam();
        }

        public SimulationParamTKY(ProcessParam parameters)
            : this()
        {
            this.processParam = parameters;
        }
    }

    public class SimulationParamVSingle : SimulationParam
    {
        public double deta, eta;

        public SimulationParamVSingle()
        {
            this.processParam = new ProcessParam();
        }

        public SimulationParamVSingle(ProcessParam parameters)
            : this()
        {
            this.processParam = parameters;
        }
    }
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Just scatching the surface of your code, but

  1. By default, the first enum constant has a default value of 0 , so this code
       public enum GrooveType : int
            {
                TKY = 0,
                VSingle = 1,
            }

could be written as

       public enum GrooveType : int
            {
                TKY,
                VSingle
            }
  1. Your ProcessParam was implemented twice. When things like this occur you should change ProcessParam into an abstract class. This is called Dependency Inversion

    Dependency Inversion: In object-oriented programming, the dependency inversion principle refers to a specific form of decoupling software modules. When following this principle, the conventional dependency relationships established from high-level, policy-setting modules to low-level, dependency modules are reversed, thus rendering high-level modules independent of the low-level module implementation details. The principle states:1

    • High-level modules should not depend on low-level modules. Both should depend on abstractions.
    • Abstractions should not depend on details. Details should depend on abstractions. More details of this Dependency Inversion

This line of code should have been

public class ProcessParam 
    {
        public ProcessParam() { }
        public double probeStartAngle { get; set; }
        public double probeStartDist { get; set; }
    }

should have been

abstract class ProcessParam   // Abstract class
{
    public abstract void AbstractMethod();   // Abstract method
    public abstract double probeStartAngle { get; set; }
    public abstract double probeStartDist { get; set; }
}
  1. Dependency Injection : Inject dependency rather than creating an object in the constructor. This is good

    public SimulationParamVSingle(ProcessParam parameters)
        : this()
    {
        this.processParam = parameters;
    }
    

This is bad

 public SimulationParamVSingle()
        {
            this.processParam = new ProcessParam();
        }
\$\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.