0
\$\begingroup\$

I'm trying to change how our system generates PDF files. Currently I have a class for each PDF that goes through the model for the PDF and fills in the properties. I want to change that to a self contained class that can be called in the background through Hangfire. To that end I've settled on decorating the properties that should be rendered to the PDF with attributes. I then reflect through the model object looking for those properties and rendering them on the PDF. I've been testing it out in LINQPad and the code I have works™, but I feel that it can be improved on, especially in the multiple foreach loops I have.

I'm looking for some input and suggestions if it can be improved for security, performance, reliability, etc.

void Main() {
    var obj = new AuthorizationPdfModel {
        Name = "John Doe",
        Phone = 8885555555,
        PhoneType = PhoneType.Mobile,
        AmountPaid = 378.92M,
        PaidWith = PaidWith.CreditCard,
        LineItems = new List<LineItem> {
                new LineItem {
                    Amount = 382.21M,
                    Quantity = 1,
                    Description = "Blargh!"
                },
                new LineItem {
                    Amount = 568.39M,
                    Quantity = 2,
                    Description = "More blargh!"
                },
                new LineItem {
                    Amount = 927.38M,
                    Quantity = 4,
                    Description = "EVEN MORE BLARGH!"
                }
            },
        Signature = "<svg></svg>"
    };

    GetAllFields(obj).Dump();
    GetSignatures(obj).Dump();
}

static IEnumerable<KeyValuePair<string, string>> GetAllFields(
    object obj) {
    var dictionary = new Dictionary<string, string>();
    var fields = GetFields(obj);

    foreach (var field in fields) {
        dictionary.Add(field.Key, field.Value);
    }

    var lists = GetLists(obj);

    foreach (var list in lists) {
        foreach (var item in list) {
            var i = list.IndexOf(item);
            var itemFields = GetFields(item, i);

            foreach (var itemField in itemFields) {
                dictionary.Add(itemField.Key, itemField.Value);
            }
        }
    }

    return dictionary.ToList();
}

static IEnumerable<KeyValuePair<string, string>> GetFields(
    object obj,
    int? i = null) {
    return GetProperties<PdfFieldAttribute>(obj).Select(
        p => {
            var display = p.Property.GetCustomAttribute<DisplayAttribute>();
            var format = p.Property.GetCustomAttribute<DisplayFormatAttribute>();

            var key = display == null ? p.Property.Name : display.Name;
            var value = GetValue(p.Property, format, p.Value);

            key = i.HasValue ? key + "." + i : key;

            return new KeyValuePair<string, string>(key, value);
        });
}

static IEnumerable<IList> GetLists(
    object obj) {
    return GetProperties<PdfListAttribute>(obj).Select(
        p =>
            p.Value).Cast<IList>();
}

static IEnumerable<KeyValuePair<string, string>> GetSignatures(
    object obj) {
    return GetProperties<PdfSignatureAttribute>(obj).Select(
        p => {
            var display = p.Property.GetCustomAttribute<DisplayAttribute>();

            var key = display == null ? p.Property.Name : display.Name;
            var value = GetValue(p.Property, null, p.Value);

            return new KeyValuePair<string, string>(key, value);
        });
}

static IEnumerable<PropertyDetails> GetProperties<TAttribute>(
    object obj)
    where TAttribute : PdfAttribute {
    return obj.GetType().GetProperties().Where(
        p =>
            p.GetGetMethod() != null).Select(
        p =>
            new {
                Attribute = p.GetCustomAttribute<TAttribute>(),
                Property = p,
                Value = p.GetValue(obj)
            }).Where(
        p =>
            p.Attribute != null
            && p.Value != null).Select(
        p =>
            new PropertyDetails {
                Property = p.Property,
                Value = p.Value
            });
}

internal sealed class PropertyDetails {
    public PropertyInfo Property { get; set; }
    public object Value { get; set; }
}

static readonly Type _booleanType = typeof(bool);

static string GetBooleanValue(
    Type type,
    object value) {
    if (type.UnderlyingSystemType != _booleanType) {
        return null;
    }

    return Convert.ToBoolean(value).ToString();
}

static string GetEnumValue(
    Type type,
    object value) {
    if (!type.IsEnum) {
        return null;
    }

    var name = Enum.GetName(type, value);
    var member = type.GetMember(name).SingleOrDefault();

    if (member == null) {
        return null;
    }

    var display = member.GetCustomAttribute<DisplayAttribute>();

    if (display == null) {
        return null;
    }

    return display.Name;
}

static string GetFormattedValue(
    Type type,
    DisplayFormatAttribute format,
    object value) {
    if (type.IsEnum
        || type.IsNullableEnum()
        || format == null) {
        return value.ToString();
    }

    return string.Format(format.DataFormatString, value);
}

static string GetNullableEnumValue(
    Type type,
    object value) {
    if (!type.IsNullableEnum()) {
        return null;
    }

    var underlyingType = Nullable.GetUnderlyingType(type);

    return GetEnumValue(underlyingType, value);
}

static string GetValue(
    PropertyInfo property,
    DisplayFormatAttribute format,
    object value) {
    var type = property.PropertyType;
    var normalizedValue = GetFormattedValue(type, format, value);

    normalizedValue = GetBooleanValue(type, value) ?? normalizedValue;
    normalizedValue = GetEnumValue(type, value) ?? normalizedValue;
    normalizedValue = GetNullableEnumValue(type, value) ?? normalizedValue;

    return normalizedValue;
}

public abstract class DocumentPdfModel {
    public abstract string TemplateName { get; }
}

public class AuthorizationPdfModel : DocumentPdfModel {
    public override string TemplateName {
        get {
            return "Authorization";
        }
    }

    #region PDF Fields
    [PdfField, Display(Name = "NameOfCustomer")]
    public string Name { get; set; }
    [PdfField, DisplayFormat(DataFormatString = "{0:(###) ###-####}")]
    public long Phone { get; set; }
    [PdfField]
    public PhoneType PhoneType { get; set; }
    [PdfField, DisplayFormat(DataFormatString = "{0:c}")]
    public decimal? AmountDue {
        get {
            return LineItems.Sum(li => li.TotalAmount);
        }
    }
    [PdfField, DisplayFormat(DataFormatString = "{0:c}")]
    public decimal? AmountPaid { get; set; }
    [PdfField, DisplayFormat(DataFormatString = "{0:c}")]
    public decimal? BalanceRemaining {
        get {
            if (!AmountDue.HasValue
                || !AmountPaid.HasValue) {
                return null;
            }

            return AmountDue - AmountPaid;
        }
    }
    [PdfField]
    public PaidWith PaidWith { get; set; }
    [PdfList]
    public IList<LineItem> LineItems { get; set; }
    [PdfSignature, Display(Name = "SignatureOfCustomer")]
    public string Signature { get; set; }
    #endregion
}

public sealed class LineItem {
    [PdfField]
    public string Description { get; set; }
    [PdfField, DisplayFormat(DataFormatString = "{0:c}")]
    public decimal Amount { get; set; }
    [PdfField]
    public int Quantity { get; set; }
    [PdfField, DisplayFormat(DataFormatString = "{0:c}")]
    public decimal TotalAmount {
        get {
            return Amount * Quantity;
        }
    }
}

public enum PhoneType {
    None,
    Mobile,
    Work
}

public enum PaidWith {
    None,
    Cash,
    Check,
    [Display(Name = "Credit Card")]
    CreditCard
}

[AttributeUsage(AttributeTargets.Property)]
public class PdfAttribute : Attribute {
}

public sealed class PdfFieldAttribute : PdfAttribute {
}

public sealed class PdfListAttribute : PdfAttribute {
}

public sealed class PdfSignatureAttribute : PdfAttribute {
}

internal static class TypeExtensions {
    public static bool IsNullableEnum(
        this Type type) {
        var underlyingType = Nullable.GetUnderlyingType(type);

        return underlyingType != null
            && underlyingType.IsEnum;
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Do you mind if the answer contains C# 6 syntax? \$\endgroup\$
    – Xiaoy312
    Apr 19, 2016 at 22:10
  • \$\begingroup\$ I am sadly still on C# 5 (VS 2013), so if possible, please try to avoid it. \$\endgroup\$
    – Gup3rSuR4c
    Apr 20, 2016 at 0:06

1 Answer 1

1
\$\begingroup\$

Coding Style

Brackets

Generally, this is more a matter of taste. However, you should try to follow the conventions in most of the case, unless your department says otherwise.

The follow piece of code is particularly frustrating to read. The argument list and method block are not so obvious at first glance, and the argument is actually align on the same level as the method body. While this is okay, the lack of visual clue(blank line) that separates them is frustrating.

static IEnumerable<KeyValuePair<string, string>> GetAllFields(
    object obj) {
    var dictionary = new Dictionary<string, string>();
    var fields = GetFields(obj);

    // ...

}

static string GetEnumValue(
    Type type,
    object value) {
    if (!type.IsEnum) {
        return null;
    }

    // ...

}

LINQ

As for LINQ, I would recommend to limit yourself to one operation per line(everything until your first linq method can be in one line, but again you may want to switch line when it gets too long).

Taking for example your GetProperties<TAttribute>(object) implemetation :

return obj.GetType().GetProperties().Where(
    p =>
        p.GetGetMethod() != null).Select(
    p =>
        new {
            Attribute = p.GetCustomAttribute<TAttribute>(),
            Property = p,
            Value = p.GetValue(obj)
        }).Where(
    p =>
        p.Attribute != null
        && p.Value != null).Select(
    p =>
        new PropertyDetails {
            Property = p.Property,
            Value = p.Value
        });

First, the linq methods are not aligned, while the lambda parts are. This makes the reading difficult, as the reader would have find the x => ... and then backtract to previous line to find out whether this is a filter, projection or something else. Secondly, since you mentioned that you use LINQPad(wonderful tool), writing each linq method on a new line will also have the benefit of easily inserting a .Dump() to test the result of each filter, projection, etc...

I would write it like this :

return obj.GetType().GetProperties()
    .Where(p => p.GetGetMethod() != null)
    .Select(p => new
    {
        Attribute = p.GetCustomAttribute<TAttribute>(),
        Property = p,
        Value = p.GetValue(obj)
    })
    //.Dump("Properties with a getter")
    .Where(p => p.Attribute != null && p.Value != null)
    //.Dump("... that is tagged and that has a value")
    .Select(p => new PropertyDetails
    {
        Property = p.Property,
        Value = p.Value
    });

IEnumerable<KeyValuePair<string, string>> GetFields(object, int?)

GetFields should not care about the index of current object. As its name indicates, it should only get the PdfFields of the object. The higher up should be the only to taking care of index.

IEnumerable<KeyValuePair<string, string>> is a really clunky type, use a IDictionary<string, string>. The conversion can be made with a .ToDictionary(x => x.Key, x => x.Value).


Final Solution

public static class PdfDataHelper
{
    public static IDictionary<string, string> ExtractModelData(object o)
    {
        if (o == null)
            throw new ArgumentNullException();

        var type = o.GetType();
        var propertyInfos = type.GetProperties()
            .Select(p => new
            {
                Name = p.GetCustomAttribute<DisplayAttribute>() != null ? p.GetCustomAttribute<DisplayAttribute>().Name : p.Name,
                FieldAttribute = p.GetCustomAttribute<PdfFieldAttribute>(),
                ListAttribute = p.GetCustomAttribute<PdfListAttribute>(),
                FormatAttribute = p.GetCustomAttribute<DisplayFormatAttribute>(),
                Property = p,
                Type = p.PropertyType,
                Value = p.GetValue(o),
            })
            // only keep properties with appropriate attributes
            .Where(x => x.FieldAttribute != null || x.ListAttribute != null);

        // throw exception for name conflict while we still have the context to debug
        var nameConflicts = propertyInfos
            .GroupBy(x => x.Name)
            .Where(x => x.Count() > 1)
            .ToList();
        if (nameConflicts.Any())
        {
            var exception = new Exception("Conflicting property name : " + string.Join(",", nameConflicts.Select(x => x.Key)));
            foreach (var conflict in nameConflicts)
                exception.Data.Add(conflict.Key, conflict.ToList());

            throw exception;
        }

        var dictionary = new Dictionary<string, string>();
        foreach (var property in propertyInfos.Where(p => p.Value != null))
        {
            if (property.FieldAttribute != null)
            {
                if (property.Type.IsValueType || property.Type == typeof(string))
                {
                    dictionary.Add(property.Name, FormatValue(property.Value, property.Type, property.FormatAttribute));
                }
                else if (property.Type.IsClass)
                {
                    foreach (var row in ExtractModelData(property.Value))
                        dictionary.Add(property.Name + "." + row.Key, row.Value);
                }
                // there might be some overlooked cases
                else throw new NotSupportedException(string.Format("Property of type `{0}` is not supported", property.Type));
            }
            else if (property.ListAttribute != null)
            {
                if (!typeof(IEnumerable).IsAssignableFrom(property.Type))
                    throw new NotSupportedException(string.Format("Property `{0}` is tagged with PdfList, but is not enumerable.", property.Name));

                var source = ((IEnumerable)property.Value)
                    .Cast<object>()
                    .Select((x, i) => new { Index = i, Item = x });
                foreach (var element in source)
                    foreach (var row in ExtractModelData(element.Item))
                        dictionary.Add(row.Key + "." + element.Index, row.Value);
            }
        }

        return dictionary;
    }

    private static string FormatValue(object value, Type type, DisplayFormatAttribute format)
    {
        if (value == null)
            return null;

        if (type.IsEnum || (Nullable.GetUnderlyingType(type) != null && Nullable.GetUnderlyingType(type).IsEnum))
        {
            if (!Enum.IsDefined(type, value))
                return value.ToString();

            var display = type.GetField(value.ToString()).GetCustomAttribute<DisplayAttribute>();
            if (display != null)
                return display.Name;

            return value.ToString();
        }

        if (type == typeof(string))
            return (string)value;

        if (format != null)
            return string.Format(format.DataFormatString, value);

        return value.ToString();
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ I have made changes (improvements?) to my code and have updated my example if you want to take a look. \$\endgroup\$
    – Gup3rSuR4c
    Apr 20, 2016 at 0:13

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.