0
\$\begingroup\$

As the above title states I have a function that I use to populate a collection's status names, currency names and other information that I do not store in the database.

This is my function:

    public static List<T> PopulateStatusNames<T>(List<T> items) where T : class
    {

        if (typeof(IStatusResult).IsAssignableFrom(typeof(T)))
        {
            Dictionary<int, string> statuses = new Dictionary<int, string>();

            if (typeof(IActivityStatusResult).IsAssignableFrom(typeof(T))) statuses = GlobalConstants.GetActivityStatuses();
            if (typeof(IPurchaseStatusResult).IsAssignableFrom(typeof(T))) statuses = GlobalConstants.GetPurchaseStatuses();
            if (typeof(IQuotationStatusResult).IsAssignableFrom(typeof(T))) statuses = GlobalConstants.GetQuotationStatuses();
            if (typeof(IInvoiceStatusResult).IsAssignableFrom(typeof(T))) statuses = GlobalConstants.GetInvoiceStatuses();

            foreach (var item in items.OfType<IStatusResult>())
            {
                item.StatusName = statuses[item.Status];
            }
        }

        if (typeof(ICurrencyResult).IsAssignableFrom(typeof(T)))
        {
            Dictionary<int, string> currencies = GlobalConstants.GetCurrencies();

            foreach (var item in items.OfType<ICurrencyResult>())
            {
                item.CurrencyName = currencies[item.Currency];
            }
        }

        return items;
    }

So if a collection implements an interface of type IStatusResult I look up the approapriate status names and loop through the collection to set the status name.

As you also notice in the above that I loop through my collection twice because if my collection implements another type of interface I need to get a different set key value pairs so that I can look up the name

Is there a way to re-write the below function to populate status names, currency names in a single for loop

Sample POCO that implement these interfaces

Invoice:

public class InvoiceSearchResult : IInvoiceStatusResult, ICurrencyResult
{
    public int Id { get; set; }
    public string InvoiceNo { get; set; }
    public DateTime InvoiceDate { get; set; }
    public string Project { get; set; }
    public string Location { get; set; }
    public string Customer { get; set; }
    public int Currency { get; set; }
    public string CurrencyName { get; set; }
    public int Status { get; set; }
    public string StatusName { get; set; }
    public decimal Subtotal { get; set; }
    public decimal TotalTax { get; set; }
    public decimal Total { get; set; }
}

Location:

public class LocationSearchResult : IActivityStatusResult
{
    public int Id { get; set; }
    public string Name { get; set; }
    public int Status { get; set; }
    public string StatusName { get; set; }
}

So as you can see I have a different combination of collections that implement these interfaces, based on the interfaces I am trying to figure out what needs to be populated

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

If you look closely there is actually four loops in your code. Two is explicit and other two from code like items.OfType<IStatusResult>() where if you look at the implementation of OfType<T> is looks something like below

foreach (object obj in source)
      {
        if (obj is TResult)
          yield return (TResult) obj;
      }

I would rather prefer to have one foreach loop to loop through all the items once and based on the type of each item, assign the appropriate properties something like below

foreach (var item in items)
            {
                if (typeof(ICurrencyResult).IsAssignableFrom(item.GetType()))
                {
                    item.CurrencyName = currencies[item.Currency];
                }
                else if (typeof(IStatusResult).IsAssignableFrom(item.GetType()))
                {
                    item.StatusName = statuses[item.Status];
                }
            }
\$\endgroup\$
  • \$\begingroup\$ i am not sure if that is going to work, will test it though. Without the implicit OfType<IStatusResult>() i was failing to access the Status and StatusName properties in the loop \$\endgroup\$ – JustLearning Mar 8 at 7:29
  • 1
    \$\begingroup\$ If you have actual instances then rather than calling GetType() and IsAssignableFrom you can just test is. As a bonus, with recent language versions you can do the coercion which gives you the ability to set the property: if (item is CurrencyResult currencyResult) currencyResult.CurrencyName = currencies[currencyResult.Currency]; But this still has some ugliness in fetching the status lookup, and it's pretty much guaranteed to do wasted work on type checks whose results should be known at compile-time. \$\endgroup\$ – Peter Taylor Mar 8 at 9:08
  • \$\begingroup\$ @JustLearning you can access StatusName property by casting it to be of type interface like ((IStatusResult)item).StatusName. assuming statusname is a property of IStatusResult. \$\endgroup\$ – Asif Mar 8 at 9:15
0
\$\begingroup\$

Ugh. Not only does this effectively push method resolution to runtime rather than compile-time, but it doesn't handle ambiguities. What if someone foolishly implements IActivityStatusResult and IPurchaseStatusResult in the same class?

I would consider handling this with T4 code generation. Something like

<#@ template debug="false" hostspecific="true" language="C#" #>
<#@ assembly name="System.Core" #>
<#@ output extension=".g.cs" #>
using System.Collections.Generic;

namespace Foo
{
    public static partial class Bar
    {
<# WritePopulateStatusNames("IActivityStatusResult",  "GlobalConstants.GetActivityStatuses()",  "Status",   "StatusName"); #>
<# WritePopulateStatusNames("IPurchaseStatusResult",  "GlobalConstants.GetPurchaseStatuses()",  "Status",   "StatusName"); #>
<# WritePopulateStatusNames("IQuotationStatusResult", "GlobalConstants.GetQuotationStatuses()", "Status",   "StatusName"); #>
<# WritePopulateStatusNames("IInvoiceStatusResult",   "GlobalConstants.GetInvoiceStatuses()",   "Status",   "StatusName"); #>
<# WritePopulateStatusNames("ICurrencyResult",        "GlobalConstants.GetCurrencies()",        "Currency", "CurrencyName"); #>
    }
}

<#+ public void WritePopulateStatusNames(string type, string dict, string intProp, string strProp)
    {
#>      public static List<<#=type#>> PopulateStatusNames(List<<#=type#>> items)
        {
            Dictionary<int, string> lookup = <#=dict#>;

            foreach (var item in items)
            {
                item.<#=strProp#> = lookup[item.<#=intProp#>];
            }

            return items;
        }

<#+ }#>

That way you get compile-time resolution and a compile-time error in case of ambiguities.


On the actual code itself, why List<>? Code to the interface, not the implementation: here that means preferring IList<> or even IEnumerable<>.

\$\endgroup\$

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.