Correct usage of MVVM and object for method

Question

I'm writing my first C# Windows Store app and am learning C# from scratch. I'm trying to implement MVVM as I understand it and object orientated patterns, obviously though I'm expecting to be doing it wrong so looking for pointers as to what I should improve.

Below is a method that gets called when a user either clicks a 'Search' XAML control, or hits return in the search textbox control.

The comments should explain the code. I feel like I probably have too much code in for a event method so please advise how I would restructure it.

 private void SubmitSearch(object sender, RoutedEventArgs e)
 {
 //New Validation object
 ViewModels.Validation validation = new ViewModels.Validation();
 // Capture user entered term and check is valid
 // submit appropriate message. 
 var searchValue = searchTerm.Text.ToString();
 dynamic validateInput = validation.inputNullCheck(searchValue);
 int? errorCode = validateInput.ErrorCode;
 if (errorCode == 0)
 {
 SubmitAction(searchValue);
 }
 else
 {
 DisplayTerms(validateInput);
 }
 }

My SubmitAction method:

public async Task<object> SubmitAction(string searchValue)
 {
 // Query webservice/database through Model and return response
 dynamic response = await new ViewModels.Search().QueryRequest(searchValue);
 DisplayTerms(response);
 //This is here only because I needed the method to be async, which in turn requires a
 // value to be returned. Ideally it would be async and end after DisplayTerms(response);
 return response;
 }

DisplayTerms method:

private void DisplayTerms(object value)
 {
 ListView termsList = termsListContainer;
 dynamic searchResponse = value;
 int count = searchResponse.Count;
 // This will eventually be a loop through the returned object
 // it is hardcoded at the moment because I am debugging something. 
 termsList.Items.Add(searchResponse[0].TermName);
 }

Validation class:

class Validation
{
 // Temporarily 0 = success, 1 = error
 public int? ErrorCode { get; set; }
 public string ErrorName { get; set; }
 public string ErrorMessage { get; set; }
 //Constructor 
 public Validation ()
 {
 }
 // Check if user input was empty
 public object inputNullCheck(string input)
 {
 if (input != "")
 {
 this.ErrorCode = 0;
 this.ErrorName = "Success";
 this.ErrorMessage = "Valid input received";
 return this;
 }
 else
 {
 this.ErrorCode = 1;
 this.ErrorName = "Input Empty";
 this.ErrorMessage = "You have not input anything.";
 return this; 
 }
 }
}

• Jeroen Vannevel: Could you clarify why you are using dynamic instead of a concrete type?
• James: I'm not on my PC right now, but fairly certain when I tried a concrete type it gave me an error. I will have to check my code tomorrow and let you know I'm unfortunately away from my PC until then.

Answer 1

Class Validation

• you can omit the empty constructor. If a class contains no constructor the compiler will add a default parameterless one.
• the properties setters should be either private or protected
• nullable int as error code does not make sense, at least based on your implementation.

• public object inputNullCheck(string input)

  • this method implies by it's name that it is checking if input == null. But it checks if input != "". Also for readability always check against String.Empty.
  • passing null to this method leads to Success because null != ""
  • instead of returning object you should return Validation
  • instead of checking if ErrorCode == 0 for success you should provide a property Boolean HasError.
  • for input which is not null and not empty you shouldn't set the properties

• Refactored

  class ErrorConstants
  {
   public const int EmptyInput = 1;
   public const int NullInput = 2;
  }
  class Validation
  {
   public int? ErrorCode { get; private set; }
   public string ErrorName { get; private set; }
   public string ErrorMessage { get; private set; }
   public Boolean HasError { get { return ErrorCode.HasValue; } }
   public Validation ValidateInput(string input)
   {
   if (input == null)
   {
   this.ErrorCode = ErrorConstants.NullInput;
   this.ErrorName = "Input null";
   this.ErrorMessage = "Given input is null.";
   }
   else if (input.Length == 0)
   {
   this.ErrorCode = ErrorConstants.EmptyInput;
   this.ErrorName = "Input Empty";
   this.ErrorMessage = "You have not input anything.";
   }
   return this;
   }
  }
  

private void SubmitSearch(object sender, RoutedEventArgs e)

As searchTerm has a Text property this property will return a String, so calling ToString() is obsolete.

Implementing the changes above this will look like

private void SubmitSearch(object sender, RoutedEventArgs e)
{
 ViewModels.Validation validation = new ViewModels.Validation();
 var searchValue = searchTerm.Text;
 if (validation.inputNullCheck(searchValue).HasError)
 {
 DisplayTerms(validation);
 }
 else
 {
 SubmitAction(searchValue); 
 }
}

You shouldn't call DisplayTerms() here, as this will for sure throw an exception, beacuse a string does not have a property ́Count`.

Naming

Based on the naming guidlines

• classes, methods and properties should be named using PascalCase casing -> inputNullCheck()
• method names should be made out of verbs or verb phrases -> inputNullCheck()

General

• Comments should describe why something is done. Describing what is done should be done by the code itself.

• James: Thank you for your help, I've just got around to reviewing this and it is very helpful. Everything seems to make sense to me, although until you mentioned it I though of null and empty as the same (which I now know is incorrect).

Answer 2

Minor note:

Here in your validation class there's this comment.

// Temporarily 0 = success, 1 = error

Which is fine. You don't want to use a boolean because you'll be adding more status codes in the future. Cool! But you shouldn't be hard coding the error code numbers. This is what enums are for.

enum ValidationStatus {Success,GenericError}
// Check if user input was empty
public object inputNullCheck(string input)
{
 if (input != "")
 {
 this.ErrorCode = ValidationStatus.Success;

At this point, it would be better to remove this code

 this.ErrorName = "Success";
 this.ErrorMessage = "Valid input received";

And create a class that manages what Message to return based off of the value of the ValidationStatus passed into its constructor.

Reducing the above code to

 if (input != "")
 {
 return new ValidationResult(ValidationStatus.Success);

• James: Thanks for this too, much appreciated. I haven't used enums before but I will look into their benefits for both this and further usage.
• RubberDuck: You're very welcome @James! I'm glad I could help.