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 1 down vote favorite

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();
        }

    }
share|improve this question
up vote 3 down vote

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.

share|improve this answer
    
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 – Marthe Veldhuis 2 days ago
    
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. – Marthe Veldhuis 2 days ago
    
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. – Marthe Veldhuis 2 days ago
    
@MartheVeldhuis WPF is not as good as they picture it ;-] and WinForms is by far not outdated. – t3chb0t 2 days ago
up vote 1 down vote 
 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.

share|improve this answer
    
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? – Marthe Veldhuis 2 days ago
    
@MartheVeldhuis yes, it would be better to have two methods one for initializing the data and the other one for updating any views. – t3chb0t 2 days ago

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.