1
\$\begingroup\$

I have a WinForms project that uses the Passive View MVP pattern. In the Presenter of one of my Views I have a function that gets questions and from its Model. But I want to make sure these collections are not empty so that I can inform the user when they are. Right now my code seems very ugly and I wonder if anyone has any tips to make it shorter/better. Thank you.

public void InitializeInterview(object sender, EventArgs e)
    {
        List<List<string>> questionList = new List<List<string>>();
        List<List<string>> maturityAnswerList = new List<List<string>>();
        List<List<string>> complianceAnswerList = new List<List<string>>();

        questionList = _model.GetQuestions();
        maturityAnswerList = _model.GetMaturityAnswers();
        complianceAnswerList = _model.GetComplianceAnswers();

        if (questionList != null && questionList.Any())
        {
            _view.Questions = questionList;
        }
        else
        {
            _view.Message("No questions have been found, please add a new question standard");
            _view.CloseView();
        }
        if (maturityAnswerList != null && maturityAnswerList.Any())
        {
            _view.MaturityAnswers = maturityAnswerList;
        }
        else
        {
            _view.Message("No maturity answers have been found, please add a new maturity standard");
            _view.CloseView();
        }
        if (complianceAnswerList != null && complianceAnswerList.Any())
        {
            _view.ComplianceAnswers = complianceAnswerList;
        }
        else
        {
            _view.Message("No compliance answers have been found, please add a new compliance standard");
            _view.CloseView();
        }

    }
\$\endgroup\$
3
\$\begingroup\$

Don't call List<T> "somethingList" -- e.g. if they are questions, call it questions, not questionList.


Why do you do List<List<string>> questionList = new List<List<string>>(); if the next step is questionList = _model.GetQuestions();? Instead don't bother with the initialization and just assign it directly: var questionList = _model.GetQuestions();


List<List<string>> is a warning sign to me that your design is probably flawed, and when I look at the context -- a list of questions and a separate list of answers -- I fear that your code has far worse issues than the reason for this question.


Is there a reason you're doing this in an "outdated" technology like WinForms when WPF etc. exists? There is a strong link between data and UI in this method, which is a frequent issue with WinForms; if this was WPF you could implement MVVM to separate those concerns.

\$\endgroup\$
  • \$\begingroup\$ I am using WinForms for its simplicity and since it's a requirement, I am just a student on an internship, this is my first application and my first time using c#. I have this List<List<string>> because I have a bunch of questions contained in a list that belong to one standard. The questionnaire I am making require \$\endgroup\$ – Marthe Veldhuis Oct 14 '16 at 11:24
  • \$\begingroup\$ Ignore that last sentence. Could you tell me what you are hinting at with "I fear that your code has far worse issues than the reason for this question". I need the questions and answered to be seperate. Thank you for your tips. \$\endgroup\$ – Marthe Veldhuis Oct 14 '16 at 11:26
  • \$\begingroup\$ Nice tip for the initialization. And finally, about the naming of the list, my private members of the class are already called that, so it was just a quick decision that I will improve. \$\endgroup\$ – Marthe Veldhuis Oct 14 '16 at 11:36
  • \$\begingroup\$ @MartheVeldhuis WPF is not as good as they picture it ;-] and WinForms is by far not outdated. \$\endgroup\$ – t3chb0t Oct 14 '16 at 11:36
1
\$\begingroup\$
 maturityAnswerList != null

This null check shouldn't be there. It means that the GetMaturityAnswers method can return a null. The .net's convention is that in such a case an empty collection should be returned.


    _view.Message("No maturity answers have been found, please add a new maturity standard");
    _view.CloseView();

You method is doing more then just initializing an interview data. It's also manipulating views and probably showing messages. This should be separated.


And the last one:

InitializeInterview(object sender, EventArgs e)

Then event handler's name is wrong. It should be doSomething_ButtonClick and it shouldn't contain any logic. There should be another method that is called by the event handler. If you have more then one button/command/shortcut to intitialize an interview you would need to copy the entire code.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your time. This is a method of my Presenter class that is subscribed to an event that is raised in the View. When my View is activated, the only thing the event handler in the VIew does, is invoke the event that my Presenter is subscribed to. What do you mean by separating this? Create a new function? \$\endgroup\$ – Marthe Veldhuis Oct 14 '16 at 11:31
  • \$\begingroup\$ @MartheVeldhuis yes, it would be better to have two methods one for initializing the data and the other one for updating any views. \$\endgroup\$ – t3chb0t Oct 14 '16 at 11:35

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.