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. It's 100% free, no registration required.

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

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();
    }
}
share|improve this question
up vote 4 down vote accepted 
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;
}
share|improve this answer
    
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. – Coder Absolute 2 days ago
up vote 1 down vote

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
}
share|improve this answer
    
I agree with the violation, but I prefer Heslacher's answer. The condition itself returns bool value. – Coder Absolute 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.