4
\$\begingroup\$

I'm comparing two distinct instances and if they are not the same I save to the dB, so I've given snippet of code below of the way I am comparing these instances, I realize I could use reflection and generalize but short of using reflections is there any other way I could optimize the code or make it neater?

 public class QuestionModel
 {

    private string _selectedId = string.Empty;
    public string SelectedId
    {
        get { return _selectedId; } set { _selectedId = value; }
    }

    private List<UserResponseModel> _userResponse;
    public List<UserResponseModel> UserResponses 
    {
        get { return _userResponse ?? (_userResponse = new List<UserResponseModel>()); }
        set { _userResponse = value; }
    }

 }

 public class UserResponseModel 
 {
    public string QuestionId { get; set; }
    public string QuestionText { get; set; }
    public bool IsChecked { get; set; }


    public int? Value { get; set; }
    public string TextValue { get; set; }
    public Byte[] Content { get; set; }
    public string FilePath { get; set; }


    public int? Points { get; set; }
    public int? InputType { get; set; }

    public bool? IsCheckedRadioButton1 { get; set; }
    public bool? IsCheckedRadioButton2 { get; set; }
    public bool? IsCheckedRadioButton3 { get; set; }
    public bool? IsCheckedRadioButton4 { get; set; }
    public bool? IsCheckedRadioButton5 { get; set; }

    public bool IsCheckedCheckbox1 { get; set; }
    public bool IsCheckedCheckbox2 { get; set; }

    private string _selectedId = string.Empty;
    public string SelectedId
    {
        get { return _selectedId; }
        set { _selectedId = value; }
    }

}          



       //snippet of code that compares instances of QuestionModel

        QuestionModel questionModelInDb = null;

        if (existingResponsesInDb.Any())
            questionModelInDb = UtilityFunctions.MapResponsestoUserResponses(existingResponsesInDb);

        if (questionModelInDb == null) return false;

        var isSame = questionModelInDb.SelectedId == questionModel.SelectedId;

        if (!isSame) return false;

        var index = 0;
        foreach (var response in questionModel.UserResponses)
        {
            if (isSame)
            {
                isSame = response.IsChecked == questionModelInDb.UserResponses[index].IsChecked;

                isSame = response.IsCheckedCheckbox1 ==
                         questionModelInDb.UserResponses[index].IsCheckedCheckbox1 && isSame;

                isSame = response.IsCheckedCheckbox2 ==
                         questionModelInDb.UserResponses[index].IsCheckedCheckbox2 &&
                         isSame;

                isSame = response.IsCheckedRadioButton1 ==
                         questionModelInDb.UserResponses[index].IsCheckedRadioButton1 &&
                         isSame;
                isSame = response.IsCheckedRadioButton2 ==
                         questionModelInDb.UserResponses[index].IsCheckedRadioButton2 &&
                         isSame;
                isSame = response.IsCheckedRadioButton3 ==
                         questionModelInDb.UserResponses[index].IsCheckedRadioButton3 &&
                         isSame;
                isSame = response.IsCheckedRadioButton4 ==
                         questionModelInDb.UserResponses[index].IsCheckedRadioButton4 &&
                         isSame;
                isSame = response.IsCheckedRadioButton5 ==
                         questionModelInDb.UserResponses[index].IsCheckedRadioButton5 &&
                         isSame;

                isSame = response.SelectedId ==
                         questionModelInDb.UserResponses[index].SelectedId &&
                         isSame;

                isSame = (response.TextValue ?? String.Empty) ==
                         (questionModelInDb.UserResponses[index].TextValue ?? String.Empty) &&
                         isSame;

                isSame = (response.Value ?? 0) ==
                         (questionModelInDb.UserResponses[index].Value ?? 0) &&
                         isSame;

                index++;
            }
            else
                break; 
        }

        return isSame;
\$\endgroup\$
3
\$\begingroup\$
  1. Property names like IsCheckedRadioButton1 are completely useless as they in no way indicate the purpose of it's intended usage or what it represents. Given that it's probably bound to a radio button it means it's very likely some sort of option to chose from. My guess you could replace all those IsCheckedRadioButtonX properties with a single enum property which states the option which has been chosen. At the very least all those properties should be renamed to something meaningful.

  2. You should move the comparison into a method on theUserResponseModel like IsSameAs(UserReponseModel other). Then your main loop becomes:

    foreach (var response in questionModel.UserResponses)
    {
        isSame = isSame && response.IsSameAs(questionModelInDb.UserResponses[index]);
    
        if (!isSame) break;  
    }
    
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I thought about having descriptive property names - but here the same model is used across multiple views, this is a survey application with close to 100 questions - each question having options, text boxes - numeric &/or alphanumeric - quick followup question would reflection be a better solution here? - in terms of elegance/performance? - or is it more of personal choice for elegance \$\endgroup\$ – user38158 Mar 6 '14 at 15:52
4
\$\begingroup\$

If you want to compare two Lists, you can use Enumerable.SequenceEqual

public class UserResponseModelComparer : IEqualityComparer<UserResponseModel> {

    public bool Equals(UserResponseModel userResponse1, UserResponseModel userResponse2) {
        //... compare the two responses
    }

    public int GetHashCode(UserResponseModel userResponse) {
        // .. calculate hash code... 
        // see guidelines here: 
        // http://stackoverflow.com/questions/462451/gethashcode-guidelines-in-c-sharp
    }
}

return Enumerable.SequenceEqual(questionModel.UserResponses, 
                                questionModelInDb.UserResponses, 
                                new UserResponseModelComparer());
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Ups sorry, haven't had my coffee yet \$\endgroup\$ – ChrisWue Mar 6 '14 at 19:19
  • 1
    \$\begingroup\$ +1. Very Object Oriented; hits OO buttons: "single responsibility", "push details down", "encapsulation". And I expect lots of that busy code in the question will simply melt away. \$\endgroup\$ – radarbob Mar 8 '14 at 18:57

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.