6
\$\begingroup\$

I have created this function in my DetailsScreen.cs, but I have no idea if this is the correct file to put this kind of code in, or if coding it this way is the correct approach. I know that it works because I have tested it.

My application generates a questionnaire based on selected (cyber security) standards. I have a form DetailsScreen, which requires the user to fill in some details such as name, company, etc. They also need to select from a checkedlist (questionStandardInput), the question standard they wish to use in this interview. For each selected standard, two answer standards need to be selected from two drop down lists (maturity answer standard and compliance answers standard).

Therefore, I need to be able to dynamically add labels and comboxes for each question standard. I asked because most code concerning label font etc. is placed in .Designer.cs files instead of the .cs files. But I need certain logic from the .cs file in order to properly format the controls, which is why I put it in here.

// Load answer input according to the number of questions standards selected.
private void questionStandardInput_MouseUp(object sender, MouseEventArgs e)
{
    questionLabels.Clear();
    maturityInput.Clear();
    complianceInput.Clear();
    selectionPanel.Controls.Clear();

    foreach (var item in questionStandardInput.CheckedItems)
    {
        Label qs_label = new Label();
        qs_label.Location = new Point(0, 0 + (questionLabels.Count * 115));
        qs_label.Font = new Font("Arial", 12F, FontStyle.Regular);
        qs_label.Size = new Size(150, 18);
        qs_label.Text = item.ToString();
        questionLabels.Add(qs_label);
        this.selectionPanel.Controls.Add(qs_label);

        Label m_label = new Label();
        m_label.Location = new Point(0, 0 + (questionLabels.Count * 115 + 35));
        m_label.Font = new Font("Arial", 12F, FontStyle.Regular);
        m_label.Size = new Size(150, 18);
        m_label.Text = "Maturity standard";
        this.selectionPanel.Controls.Add(m_label);

        Label c_label = new Label();
        c_label.Location = new Point(0, 0 + (questionLabels.Count * 115 + 70));
        c_label.Font = new Font("Arial", 12F, FontStyle.Regular);
        c_label.Size = new Size(170, 18);
        c_label.Text = "Compliance standard";
        this.selectionPanel.Controls.Add(c_label);

        ComboBox m_input = new ComboBox();
        m_input.Location = new Point(170, 0 + (questionLabels.Count * 115 + 35));
        m_input.Font = new Font("Arial", 12F, FontStyle.Regular);
        m_input.Size = new Size(200, 22);
        m_input.DropDownStyle = ComboBoxStyle.DropDownList;
        maturityInput.Add(m_input);
        this.selectionPanel.Controls.Add(m_input);

        ComboBox c_input = new ComboBox();
        c_input.Location = new Point(170, 0 + (questionLabels.Count * 115 + 70));
        c_input.Font = new Font("Arial", 12F, FontStyle.Regular);
        c_input.Size = new Size(200, 22);
        c_input.DropDownStyle = ComboBoxStyle.DropDownList;
        complianceInput.Add(c_input);
        this.selectionPanel.Controls.Add(c_input);

        InitializeMaturityStandardInput(m_input);
        InitializeComplianceStandardInput(c_input);
    }
    this.saveAssessmentButton.Location = new Point(235, 0 + (questionLabels.Count * 115));
    this.selectionPanel.Controls.Add(saveAssessmentButton); 
}
\$\endgroup\$
7
\$\begingroup\$

In my eyes it is an appropriated way to build up your form that way. The *.Designer file is for designer generated code - but if you need to add custom logic to modify your GUI the *.cs file is the correct location.

Just a few points:

  • There are some numbers (115, 70, 35, ...) that seems to have a special meening (e.g. row height, margin, whatever). IMHO it makes sense to store that numbers as constants with a meaningful name and use that constants instead.

  • I am not sure if the MouseUp event is the right place to trigger that kind of logic. If there is another event (CheckBoxChanged, SelectedItemChanged, ...) I would suggest to use that.

  • There is some repeating logic for creating the ComboBox / Label and so on. That logic could be extracted to a separted method. For instance:

    private static ComboBox CreateComboBox()
    {
        return new ComboBox()
        {
            Font = new Font("Arial", 12F, FontStyle.Regular)
            Size = new Size(200, 22)
            DropDownStyle = ComboBoxStyle.DropDownList
        }
    }
    
\$\endgroup\$
6
\$\begingroup\$

Adding to @JanDotNet's answer I think you should create a UserControl that knows how to generate those items.

It is a very bad idea to put business logic inside an event handler (questionStandardInput_MouseUp).

If you had a specialized user control you could then very easy (re)place one user control with another according to the user choice and you wouldn't have to care about so many coordinates. They would be relative to the user control and in its context always correct.

Would you change the form layout now you would probably need to adjust all locations.

\$\endgroup\$
  • 1
    \$\begingroup\$ Can you explain what is considered business logic in my code? And do you mean i should create a user control that contains each label and combobox, which i add to my form with each loop? \$\endgroup\$ – Marthe Veldhuis Oct 6 '16 at 10:26
  • \$\begingroup\$ @MartheVeldhuis to me the entire content of the event hander is business logic because you build the UI dynamically based on what the user selects. \$\endgroup\$ – t3chb0t Oct 6 '16 at 10:30
  • \$\begingroup\$ @MartheVeldhuis nope, not exactly, I mean your user control should contain the loop that creates other controls. \$\endgroup\$ – t3chb0t Oct 6 '16 at 10:30
  • \$\begingroup\$ So i should call a user control from my event handler with the checkeditems as a parameter and in that user control add the other controls? I'm sorry, i'm not really familiar with user controls (visual studio in general). \$\endgroup\$ – Marthe Veldhuis Oct 6 '16 at 10:35
  • \$\begingroup\$ @MartheVeldhuis you could create a class derived from UserControl, and put that control on the form. \$\endgroup\$ – Mathieu Guindon Oct 6 '16 at 11:31
-1
\$\begingroup\$

I have come up with my own solution with your help and some research. I have implemented the Model View Controller Passive View so that no logic is handled in the View. I still have a few loose digits to clean up, but that was not the main focus.

In the View's MouseUp event handler just two events are fired. One for the Presenter that will get data from the Model (that gets it from the database) which is passed to the User Control through the View, and one for the User Control to initialize itsself. The User Control is then added to the View.

View MouseUp event handler

// Fires the initializing user control and initializing user control data events.
private void questionStandardInput_MouseUp(object sender, MouseEventArgs e)
{
    InitializingUserControl?.Invoke(this, EventArgs.Empty);
    InitializingUserControlData?.Invoke(this, EventArgs.Empty);
}

User Control constructor and Initializing function

    // Initialize user control with IDetailScreenView. Subscribe to necessary events.
    public DetailScreenUserControl(IDetailScreenView view)
    {
        InitializeComponent();
        _view = view;
        _view.InitializingUserControl += InitializeUserControl;
    }

    // Initializes the user control for the detail screen.
    public void InitializeUserControl(object sender, EventArgs e)
    {
        List<string> qStandards = _view.SelectedQuestionStandards;
        Controls.Clear();
        maturityInput.Clear();
        complianceInput.Clear();

        int inputSeparation = 115;
        int secondInputX = 35;
        int inputHeight = 20;
        int labelWidth = 200;
        int inputWidth = 170;

        for (int  i = 0; i < qStandards.Count; i++)
        {
            Panel inputPanel = new Panel();
            inputPanel.Location = new Point(0, i * inputSeparation);
            inputPanel.AutoSize = true;
            inputPanel.BackColor = Color.AliceBlue;
            this.Controls.Add(inputPanel);

            Label qs_label = new Label();
            qs_label.Location = new Point(0, 0);
            qs_label.Font = new Font("Arial", 12F, FontStyle.Bold);
            qs_label.Size = new Size(labelWidth, inputHeight);
            qs_label.AutoSize = true;
            qs_label.Text = qStandards[i].ToString();
            inputPanel.Controls.Add(qs_label);

            Label m_label = new Label();
            m_label.Location = new Point(0, secondInputX);
            m_label.Font = new Font("Arial", 12F, FontStyle.Regular);
            m_label.Size = new Size(labelWidth, inputHeight);
            m_label.Text = "Maturity standard";
            inputPanel.Controls.Add(m_label);

            Label c_label = new Label();
            c_label.Location = new Point(0, secondInputX * 2);
            c_label.Font = new Font("Arial", 12F, FontStyle.Regular);
            c_label.Size = new Size(labelWidth, inputHeight);
            c_label.Text = "Compliance standard";
            inputPanel.Controls.Add(c_label);

            ComboBox m_input = new ComboBox();
            m_input.Location = new Point(labelWidth, secondInputX);
            m_input.Font = new Font("Arial", 10F, FontStyle.Regular);
            m_input.Size = new Size(inputWidth, inputHeight);
            m_input.DropDownStyle = ComboBoxStyle.DropDownList;
            maturityInput.Add(m_input);
            inputPanel.Controls.Add(m_input);

            ComboBox c_input = new ComboBox();
            c_input.Location = new Point(labelWidth, secondInputX * 2);
            c_input.Font = new Font("Arial", 10F, FontStyle.Regular);
            c_input.Size = new Size(inputWidth, inputHeight);
            c_input.DropDownStyle = ComboBoxStyle.DropDownList;
            complianceInput.Add(c_input);
            inputPanel.Controls.Add(c_input);

        }

        saveAssessmentButton.Location = new Point(195, qStandards.Count * inputSeparation);
        saveAssessmentButton.Size = new Size(130, 25);
        saveAssessmentButton.Text = "Save assessment";
        saveAssessmentButton.BackColor = SystemColors.ButtonHighlight;
        this.Controls.Add(saveAssessmentButton);

        _view.AddUserControl();
    }
\$\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.