7
\$\begingroup\$

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; 
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Could you clarify why you are using dynamic instead of a concrete type? \$\endgroup\$ – Jeroen Vannevel Dec 11 '14 at 18:12
  • \$\begingroup\$ 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. \$\endgroup\$ – James Dec 11 '14 at 18:37
4
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ 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). \$\endgroup\$ – James Dec 13 '14 at 13:55
4
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – James Dec 13 '14 at 13:56
  • \$\begingroup\$ You're very welcome @James! I'm glad I could help. \$\endgroup\$ – RubberDuck Dec 13 '14 at 14:01

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.