current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 6 down vote favorite

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. Please help me, i am only a student doing an internship and i can not get any proper help.

More details: My application generates a questionnare 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, that's 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); 
    }
share|improve this question
up vote 6 down vote

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.

share|improve this answer
1  
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? – Marthe Veldhuis 2 days ago
    
@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. – t3chb0t 2 days ago
    
@MartheVeldhuis nope, not exactly, I mean your user control should contain the loop that creates other controls. – t3chb0t 2 days ago
    
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). – Marthe Veldhuis 2 days ago
    
@MartheVeldhuis you could create a class derived from UserControl, and put that control on the form. – Mat's Mug♦ 2 days ago
up vote 6 down vote

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
    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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