4
\$\begingroup\$

The code finds marked attributes and parses them according to the attribute type.

Is it possible to simplify this logic? Is there generic way to do it? Please suggest some way to optimise this code. All the data type of Product class will be string. I'm getting product input as XML directly converting serialized data to a class with decimal, int, float will not give proper error message.

If there is list of items it will throw error in XMl and we won't know which row has caused the error.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Text;
using System.Threading.Tasks;

namespace TestSolution
{
    public interface ICustomParser
    {
        bool Parse(string input);
    }

    public class DecimalParserAttribute : Attribute, ICustomParser
    {
        public bool Parse(string input)
        {
            if (input == null) return false;

            decimal decimalValue;

            return decimal.TryParse(input, out decimalValue);
        }
    }

    public class intParserAttribute : Attribute, ICustomParser
    {
        public bool Parse(string input)
        {
            if (input == null) return false;

            int intValue;

            return int.TryParse(input, out intValue);
        }
    }

    public class Product
    {
        [DecimalParser]
        public string Weight { get; set; }

        [intParser]
        public string NoOfItems { get; set; }

        [intParser]
        public string GRodes { get; set; }

        [intParser]
        public string WRodes { get; set; }
    }

    class Program
    {
        static void Main(string[] args)
        {

            var sb = Validate(new Product() { NoOfItems = "1", GRodes = "4", Weight = "5", WRodes = "23" });

            Console.WriteLine(sb);
            sb = Validate(new Product() { NoOfItems = "1", GRodes = "4w", Weight = "5", WRodes = "23" });

            Console.WriteLine(sb);
            Console.ReadKey();
        }

        private static string Validate(Product product)
        {
            var sb = new StringBuilder();

            foreach (var property in product.GetType().GetProperties())
            {
                var value = Convert.ToString(property.GetValue(product, null));

                var sel = property.GetAttribute<ICustomParser>();

                if (sel == null) continue;

                var parserinstance = (ICustomParser)sel;

                if (parserinstance.Parse(value)) continue;

                sb.AppendLine(string.Format("{0} Has invalid value", property.Name));
            }
            return sb.ToString();
        }
    }

    public static class Extensions
    {
        public static T GetAttribute<T>(this PropertyInfo property)
        {
            return (T)property.GetCustomAttributes(false).Where(s => s is T).FirstOrDefault();
        }
    }
}
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

I think you kind of reinventing the wheel a bit here. You could use the ValidationAttribute.

Since C# doesn't allow generics in Attribute classes you either have to create an attribute per type or use an enum to specific what type. I personally prefer an attribute per type.

An Example of use the ValidationAttrubte for DecimalParser

public class DecimalParserAttribute : ValidationAttribute
{
    public DecimalParserAttribute() : base("Cannot convert to decimal")
    {
    }

    public override bool IsValid(object value)
    {
        if (value == null)
        {
            return false;
        }

        var stringValue = Convert.ToString(value);

        decimal decimalResult;
        return decimal.TryParse(stringValue, out decimalResult);
    }
}

Now to perform the validation you need to use the Validator Class

var p = new Product() {NoOfItems = "1", GRodes = "4w", Weight = "5", WRodes = "2q3"};
var validationResults = new Collection<ValidationResult>();
if (!Validator.TryValidateObject(p, new ValidationContext(p), validationResults, true))
{
    // If we are here there was validation errors 
    var x = string.Join(Environment.NewLine,
        validationResults.SelectMany(r => r.MemberNames.Select(n => n + " " + r.ErrorMessage)));
}

This to me fits more in with what you are doing, validation, with the benefit of using the built in .Net Framework classes.

\$\endgroup\$
1
\$\begingroup\$

I believe Charles' answer is what you need but I still want to point a few things out which might be useful for you in the future.

You don't need a cast here

var parserinstance = (ICustomParser) sel;

Because the type argument here GetAttribute<ICustomParser>() is of type ICustomParser the return type will be of the same type since they are using the same generic parameter.

If you can use C# 6 you might prefer interpolated over formatted string here :

sb.AppendLine($"{property.Name} Has invalid value");

//sb.AppendLine(string.Format("{0} Has invalid value", property.Name));

You can negate your if statements to shorten the code. Using continue wont improve performance. After you are done with all of this your code should look like this :

ICustomParser sel = property.GetAttribute<ICustomParser>();
if (sel != null && !sel.Parse(value))
{
    sb.AppendLine($"{property.Name} Has invalid value");
}

Which is a lot more cleaner, shorter and easier to read.

Your extension method can is using a redundant .Where() call. FirstOrDefault() also accepts predicates.

public static T GetAttribute<T>(this PropertyInfo property)
{
    return (T) property.GetCustomAttributes(false).FirstOrDefault(s => s is T);

  //return (T) property.GetCustomAttributes(false).Where(s => s is T).FirstOrDefault();
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your valuable comments, Is using 'continue' bad coding? \$\endgroup\$ Commented Dec 24, 2016 at 4:43
  • \$\begingroup\$ I wouldn't call it bad practice because it can reduce the nesting of your if statements but it's harder to read inverse logic at least in my opinion. \$\endgroup\$
    – Denis
    Commented Dec 24, 2016 at 10:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.