3
\$\begingroup\$

The Country, State and City functions accepts the Integer as Parameter and returns the relevant error code if the validation fails in ValidateNotExist Class.

Therefore, I have enhanced the ValidateNotExist class with a slight different implementation (using switch) and named it as ValidateNotExistImproved.

But I think, the entire code can be improved further. Can someone help me figure out what mistakes am I doing and how can I improve it?

The entire code is provided with a little explanation for every class.

NotExistErrorCodeConfig

Error codes configuration.

public static class NotExistErrorCodeConfig
{
    public const string Country = "COUNTRY_NOT_EXIST";
    public const string State = "STATE_NOT_EXIST";
    public const string City = "CITY_NOT_EXIST";
}

EntityTypeEnum

Enum Configuration, this is being used in the ValidateNotExistImproved class.

public enum EntityTypeEnum
{
    Country = 1,
    State = 2,
    City = 3
}

ExistsHelper

The actual logic is implemented and it has dedicated functions.

public static class ExistsHelper
{
    public static bool Country(int id)
    {
        return (id > 10) ? true : false;
    }

    public static bool State(int id)
    {
        return (id > 10) ? true : false;
    }

    public static bool City(int id)
    {
        return (id > 10) ? true : false;
    }
}

ValidateNotExist

A helper class, the implementation calls this class. This class is responsible to check the value and return the relevant error message.

public static class ValidateNotExist
{
    public static string Country(int id)
    {
        return (!ExistsHelper.Country(id)) ? NotExistErrorCodeConfig.Country : string.Empty;
    }

    public static string State(int id)
    {
        return (!ExistsHelper.State(id)) ? NotExistErrorCodeConfig.State : string.Empty;
    }

    public static string City(int id)
    {
        return (!ExistsHelper.City(id)) ? NotExistErrorCodeConfig.City : string.Empty;
    }
}

ValidateNotExistImproved

I have combined the three functions into single function but using Switch statement. This is supposed to be improved version of ValidateNotExist.

public class ValidateNotExistImproved
{
    public static string Validate(EntityTypeEnum type, int id)
    {
        string errorCode = string.Empty;

        switch (type)
        {
            case EntityTypeEnum.Country:
                errorCode = (!ExistsHelper.Country(id)) ? NotExistErrorCodeConfig.Country : string.Empty;
                break;

            case EntityTypeEnum.State:
                errorCode = (!ExistsHelper.State(id)) ? NotExistErrorCodeConfig.State : string.Empty;
                break;

            case EntityTypeEnum.City:
                errorCode = (!ExistsHelper.City(id)) ? NotExistErrorCodeConfig.City : string.Empty;
                break;

            default:
                break;
        }

        return errorCode;
    }
}

ImplementationClass

The actual implementation class, it returns the same error code.

public class ImplementationClass
{
    public ImplementationClass()
    {
        var error = ValidateNotExist.Country(2);                                         //Throws an error COUNTRY_NOT_EXIST
        var extendedError = ValidateNotExistImproved.Validate(EntityTypeEnum.Country, 2);        //Throws an error COUNTRY_NOT_EXIST

        Console.WriteLine(error);
        Console.WriteLine(extendedError);
        Console.ReadKey();
    }
}
\$\endgroup\$
4
\$\begingroup\$
public static class ExistsHelper
{
    public static bool Country(int id)
    {
        return (id > 10) ? true : false;
    }

    public static bool State(int id)
    {
        return (id > 10) ? true : false;
    }

    public static bool City(int id)
    {
        return (id > 10) ? true : false;
    }
}  

The usage of a ternary here is senseless, you can just return the evaluated condition like so

public static class ExistsHelper
{
    public static bool Country(int id)
    {
        return (id > 10);
    }

    public static bool State(int id)
    {
        return (id > 10);
    }

    public static bool City(int id)
    {
        return (id > 10);
    }
}  

but looking at it, it seems it doesn't matter what "type" you are validating because they all have the same condition. You could simply use only one method and extract the magic number 10 into a meaningful constant.


ValidateNotExistImproved

Here you are using a ternary as well which isn't needed because a simple if statement would be enough and more readable like so

public static string Validate(EntityTypeEnum type, int id)
{
    switch (type)
    {
        case EntityTypeEnum.Country:
            if (!ExistsHelper.Country(id))
            {
                return NotExistErrorCodeConfig.Country;
            }
            break;

        case EntityTypeEnum.State:
            if (!ExistsHelper.State(id))
            {
                return NotExistErrorCodeConfig.State;
            }
            break;

        case EntityTypeEnum.City:
            if (!ExistsHelper.City(id))
            {
                return NotExistErrorCodeConfig.City ;
            }
            break;
    }

    return string.Empty;
}
\$\endgroup\$
  • \$\begingroup\$ I have decided to go ahead with 'ValidateNotExist' implementation with some refactoring so that I can have overloading if multiple parameters are required for validation. \$\endgroup\$ – Coder Absolute Jul 14 '16 at 13:34
2
\$\begingroup\$

1.First of all .Net Name convension is violated a lot of times. Look here
2.As for me there are too much noise in your code. The following simple and straightforward solution seems to me better then yours

public class Validator
{
   public bool ValidateCountry(int countryCode)
   {
      if(countryCode >10)
      return true;

      return false;
   }
//the same functions for City and State
}
\$\endgroup\$
  • \$\begingroup\$ I agree with the violation, but I prefer Heslacher's answer. The condition itself returns bool value. \$\endgroup\$ – Coder Absolute Jul 14 '16 at 13:32

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.