6
\$\begingroup\$

I have made some code that operates on multiple entities and creates 'time windows' for them. Basically, the entities will contain states, usually in boolean or int. They will be timestamped and in order to reduce the number of rows when creating a graph for a large date range, I am calculating time windows of 5 mins, 15 mins and 60 mins. If I'm calculating the 5 minute time windows, and a state is true from 00:00:00 to 00:02:59 and false from 00:03:00 onwards, the time window for the first 5 minutes of the day would be true, since the state was true for 3 out of 5 minutes.

I have used custom attributes, generics and reflection in order to check what properties the entities have and which of them I need to process.

The code works, but I'm sure it can be improved.

Sometimes, the code works fast for a couple of thousands of rows across 16 entities, i.e. 16 calls to the CalculateTimeWindows<T>() method (less than a second) but sometimes it's really slow (takes some 45 seconds). Any ideas how I can optimise this code?

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Linq;
using System.Reflection;

namespace Services.Helpers
{
    #region Custom Attributes
    [AttributeUsage(AttributeTargets.Property)]
    public class DoNotCopyIntoTimeWindow : System.Attribute { } // leave default

    [AttributeUsage(AttributeTargets.Property)]
    public class IsProcessedIntoTimeWindow : System.Attribute { } // calculate time window for this property

    [AttributeUsage(AttributeTargets.Property)]
    public class IsTimeWindowDate : Attribute { } // attribute to mark property as the datetime

    [AttributeUsage(AttributeTargets.Property)]
    public class IsTimeWindowIdentifier : Attribute { } // this is the time window property
    #endregion

    public class TimeWindow
    {
        #region Structs
        public struct TimeWindowDictionary
        {
            public string Name { get; set; }
            public Dictionary<NullObject<dynamic>, int> Dictionary { get; set; }
        }

        public struct NullObject<T>
        {
            [DefaultValue(true)]
            private readonly bool isnull;// default property initializers are not supported for structs

            private NullObject(T item, bool isnull) : this()
            {
                this.isnull = isnull;
                Item = item;
            }

            public NullObject(T item) : this(item, item == null)
            {
            }

            public static NullObject<T> Null()
            {
                return new NullObject<T>();
            }

            public T Item { get; private set; }

            public bool IsNull()
            {
                return isnull;
            }

            public static implicit operator T(NullObject<T> nullObject)
            {
                return nullObject.Item;
            }

            public static implicit operator NullObject<T>(T item)
            {
                return new NullObject<T>(item);
            }

            public override string ToString()
            {
                return (Item != null) ? Item.ToString() : "NULL";
            }

            public override bool Equals(object obj)
            {
                if (obj == null)
                    return IsNull();

                if (!(obj is NullObject<T>))
                    return false;

                var no = (NullObject<T>)obj;

                if (IsNull())
                    return no.IsNull();

                if (no.IsNull())
                    return false;

                return Item.Equals(no.Item);
            }

            public override int GetHashCode()
            {
                if (IsNull())
                    return 0;

                var result = Item.GetHashCode();

                if (result >= 0)
                    result++;

                return result;
            }
        }
        #endregion

        public static IEnumerable<T> CalculateTimeWindows<T>(DateTime dateFrom, DateTime dateTo, List<T> stateModels) where T : new()
        {
            if (stateModels.Count() == 0)
                return new List<T>();

            dateFrom = GetPropertiesAndDictionaries(dateFrom, stateModels, out PropertyInfo datePropertyInfo, out List<PropertyInfo> copyProperties, out PropertyInfo timeWindowIdentifier, out int size, out TimeWindowDictionary[] dictionaries, out int i);

            return CalculateTimeWindow(dateFrom, dateTo, stateModels, 5, datePropertyInfo, copyProperties, timeWindowIdentifier, size, dictionaries, i)
                        .Concat(CalculateTimeWindow(dateFrom, dateTo, stateModels, 15, datePropertyInfo, copyProperties, timeWindowIdentifier, size, dictionaries, i))
                        .Concat(CalculateTimeWindow(dateFrom, dateTo, stateModels, 60, datePropertyInfo, copyProperties, timeWindowIdentifier, size, dictionaries, i));
        }

        public static IEnumerable<T> CalculateTimeWindow<T>(DateTime dateFrom, DateTime dateTo, List<T> stateModels, byte timeWindowMinutes, PropertyInfo datePropertyInfo, List<PropertyInfo> copyProperties, PropertyInfo timeWindowIdentifier, int size, TimeWindowDictionary[] dictionaries, int i) where T : new()
        {
            if (stateModels.Count() > 0)
            {
                DateTime currentWindowFrom, currentWindowTo, nextWindowFrom;
                nextWindowFrom = dateFrom;
                int itemPointer = 0;
                T prevItem = default;
                T prevTimeWindow = default;

                KeyValuePair<NullObject<dynamic>, int> maxValue = new KeyValuePair<NullObject<dynamic>, int>();
                int j = 1;

                do // one time window
                {
                    for (i = 0; i < size; i++)
                        dictionaries[i].Dictionary = new Dictionary<NullObject<dynamic>, int>();

                    currentWindowFrom = nextWindowFrom;
                    nextWindowFrom = currentWindowFrom.AddMinutes(timeWindowMinutes);
                    currentWindowTo = nextWindowFrom.AddSeconds(-1);

                    var calculateTime = currentWindowFrom;

                    for (itemPointer = itemPointer; itemPointer < stateModels.Count(); itemPointer++)
                    {
                        var item = stateModels.ElementAt(itemPointer);

                        var date = (DateTime)datePropertyInfo.GetValue(item);

                        if (date >= currentWindowTo)
                            break;

                        var endDate = (date > currentWindowTo) ? nextWindowFrom : date; // state might extend more than the end of the time window
                        CalculateStateSeconds(prevItem, dictionaries, calculateTime, endDate);

                        prevItem = item;
                        calculateTime = (date < currentWindowFrom) ? currentWindowFrom : date; // to fix the 'yesterday' date
                    }

                    if (calculateTime < currentWindowTo)
                        CalculateStateSeconds(prevItem, dictionaries, calculateTime, nextWindowFrom);

                    if (dictionaries[0].Dictionary.Count > 0)
                    {
                        bool sameAsPrevious = (prevTimeWindow != null);
                        var output = new T();

                        foreach (var dictionary in dictionaries)
                        {
                            //var valToSet = dictionary.Dictionary.FirstOrDefault(x => x.Value.Equals(dictionary.Dictionary.Values.Max())).Key.Item;

                            for (i = 0; i < dictionary.Dictionary.Count; i++)
                            {
                                maxValue = dictionary.Dictionary.First();
                                for (j = 1; j < dictionary.Dictionary.Count; j++)
                                {
                                    var valuePair = dictionary.Dictionary.ElementAt(j);

                                    if (valuePair.Value > maxValue.Value)
                                        maxValue = valuePair;
                                }
                            }
                            var valToSet = maxValue.Key.Item;

                            if (sameAsPrevious)
                            {
                                var prevVal = GetValue(prevTimeWindow, dictionary.Name);

                                if (valToSet == null && prevVal == null)
                                { }
                                else if ((valToSet == null && prevVal != null) || (valToSet != null && prevVal == null) || !valToSet.Equals(prevVal))
                                    sameAsPrevious = false;
                            }

                            SetValue(output, dictionary.Name, valToSet);
                        }

                        if (!sameAsPrevious)
                        {
                            foreach (var copyProperty in copyProperties)
                                SetValue(output, copyProperty.Name, copyProperty.GetValue(prevItem));

                            timeWindowIdentifier.SetValue(output, timeWindowMinutes);

                            datePropertyInfo.SetValue(output, currentWindowFrom);

                            prevTimeWindow = output;

                            yield return output;
                        }
                    }
                }
                while (nextWindowFrom <= dateTo);
            }
        }

        private static DateTime GetPropertiesAndDictionaries<T>(DateTime dateFrom, List<T> stateModels, out PropertyInfo datePropertyInfo, out List<PropertyInfo> copyProperties, out PropertyInfo timeWindowIdentifier, out int size, out TimeWindowDictionary[] dictionaries, out int i) where T : new()
        {
            Type tType = typeof(T);
            var propInfos = tType.GetProperties();

            datePropertyInfo = propInfos.Single(p => p.GetCustomAttributes(typeof(IsTimeWindowDate), true).Any());
            var firstDate = (DateTime)datePropertyInfo.GetValue(stateModels.First());

            if (firstDate < dateFrom)
                dateFrom = new DateTime(firstDate.Year, firstDate.Month, firstDate.Day, firstDate.Hour, 0, 0, DateTimeKind.Utc);

            var properties = propInfos.Where(p => p.GetCustomAttributes(typeof(IsProcessedIntoTimeWindow), true).Any()).Select(x => x.Name);
            copyProperties = propInfos.Where(p => !p.GetCustomAttributes(typeof(IsTimeWindowIdentifier), true).Any() && !p.GetCustomAttributes(typeof(DoNotCopyIntoTimeWindow), true).Any() && !p.GetCustomAttributes(typeof(IsTimeWindowDate), true).Any() && !p.GetCustomAttributes(typeof(IsProcessedIntoTimeWindow), true).Any() && p.CanWrite && !p.GetMethod.IsVirtual).ToList();
            timeWindowIdentifier = propInfos.Single(p => p.GetCustomAttributes(typeof(IsTimeWindowIdentifier), true).Any());

            size = properties.Count();
            dictionaries = new TimeWindowDictionary[size];

            i = 0;
            foreach (var property in properties)
            {
                dictionaries[i] = new TimeWindowDictionary() { Name = property };
                i++;
            }

            return dateFrom;
        }

        private static dynamic GetValue(object inputObject, string propertyName)
        {
            Type type = inputObject.GetType();
            PropertyInfo propertyInfo = type.GetProperty(propertyName);
            return propertyInfo.GetValue(inputObject);
        }

        private static void SetValue(object inputObject, string propertyName, object propertyVal)
        {
            //find out the type
            Type type = inputObject.GetType();

            //get the property information based on the type
            PropertyInfo propertyInfo = type.GetProperty(propertyName);

            if (propertyVal != null)
            {
                //find the property type
                Type propertyType = propertyInfo.PropertyType;

                //Convert.ChangeType does not handle conversion to nullable types
                //if the property type is nullable, we need to get the underlying type of the property
                var targetType = IsNullableType(propertyType) ? Nullable.GetUnderlyingType(propertyType) : propertyType;

                //Returns an System.Object with the specified System.Type and whose value is
                //equivalent to the specified object.
                propertyVal = Convert.ChangeType(propertyVal, targetType);
            }

            //Set the value of the property
            propertyInfo.SetValue(inputObject, propertyVal, null);

        }

        private static bool IsNullableType(Type type)
        {
            return type.IsGenericType && type.GetGenericTypeDefinition().Equals(typeof(Nullable<>));
        }

        private static void CalculateStateSeconds<T>(T prevItem, IEnumerable<TimeWindowDictionary> dictionaries, DateTime calculateTime, DateTime endDate)
        {
            if (prevItem != null)
            {
                var seconds = Convert.ToInt32(endDate.Subtract(calculateTime).TotalSeconds);

                Type tType = typeof(T);

                foreach (var dictionary in dictionaries)
                {
                    PropertyInfo propertyInfo = tType.GetProperty(dictionary.Name);
                    var key = propertyInfo.GetValue(prevItem);

                    dictionary.Dictionary.TryGetValue(key, out int existingSeconds);
                    dictionary.Dictionary[key] = existingSeconds + seconds;
                }
            }
        }
    }
}

Here's a sample class and Unit test:

public abstract class MyBaseModel : ICloneable
{
    [DoNotCopyIntoTimeWindow]
    public int Id { get; set; }

    public short fk_TenantId { get; set; }

    [IsTimeWindowIdentifier]
    public byte TimeWindow { get; set; }

    [IsTimeWindowDate]
    public DateTime Date { get; set; }

    [IsProcessedIntoTimeWindow]
    public byte ActuallyAnEnum { get; set; }

    public abstract bool CalculatorOn { get; }

    public object Clone()
    {
        return this.MemberwiseClone();
    }
}

public class MyModel : MyBaseModel
{
    public short ShortId { get; set; }
    public short AnotherShortId { get; set; }

    [IsProcessedIntoTimeWindow]
    public decimal Value { get; set; }

    public override bool CalculatorOn
    {
        get
        {
            throw new NotImplementedException();
        }
    }
}

[TestMethod]
public void TestLowerResolution()
{
    /* generate test data */
    DateTime dateFrom = new DateTime(2018, 1, 28, 15, 0, 0, DateTimeKind.Utc);
    var dateTo = dateFrom.AddDays(1).AddSeconds(-1);

    var myValues = new List<MyModel>();

    myValues.Add(new MyModel()
    {
        Date = new DateTime(2018, 1, 9, 15, 48, 46, DateTimeKind.Utc),
        ShortId = 5,
        AnotherShortId = 0,
        TimeWindow = 0,
        Value = 30,
        fk_TenantId = 1006
    });

    myValues.Add(new MyModel()
    {
        Date = new DateTime(2018, 1, 29, 10, 11, 31, DateTimeKind.Utc),
        ShortId = 5,
        AnotherShortId = 0,
        TimeWindow = 0,
        Value = 14336,
        fk_TenantId = 1006
    });

    myValues.Add(new MyModel()
    {
        Date = new DateTime(2018, 1, 29, 10, 11, 59, DateTimeKind.Utc),
        ShortId = 5,
        AnotherShortId = 0,
        TimeWindow = 0,
        Value = 30,
        fk_TenantId = 1006
    });

    var windows = TimeWindow.CalculateTimeWindows(dateFrom, dateTo, myValues).ToList();
}

EDIT: Updated code at Performance issue with empty lists

\$\endgroup\$
  • 3
    \$\begingroup\$ Interesting question. You could make it easier for reviewers to follow, if you provide a simple working test case also showing the expected output. \$\endgroup\$ – Henrik Hansen Jun 12 at 10:47
  • 1
    \$\begingroup\$ Thanks @HenrikHansen, I've just done so. \$\endgroup\$ – Mark Cilia Vincenti Jun 12 at 11:54
  • 1
    \$\begingroup\$ Could you clarify this: If I'm calculating the 5 minute time windows, and a state is true from 00:00:00 to 00:02:59 and false from 00:03:00 onwards - why isn't the sate valid until 00:04:59 which would be almost 5 minutes but some other random number? Why do you just have 5, 15, and 60 steps instead of any number spanning between two timestamps? Why do you need all these attributes? It'd be much easier to use an interface and where the properties don't match it just create dummy properties on your entities to map to the actual ones. I find this solution is currently super complex o_O \$\endgroup\$ – t3chb0t Jun 13 at 12:41
  • 1
    \$\begingroup\$ I went through your code and like you... I have absolutely no idea what you are doing there and why. There are so many classes, structs and properties and logic that you have not described that it is virtually incomprihensible without reverse-engineering it for a couple of hours. I wish you luck in optimizing it, you'll need a lot of it :-P \$\endgroup\$ – t3chb0t Jun 15 at 13:39
  • 1
    \$\begingroup\$ @t3chb0t I feel the same way. There is no beginning this task. Respect to the reviewers that take a shot at this. en.wikipedia.org/wiki/Sisyphus \$\endgroup\$ – dfhwze Jun 15 at 18:33
4
+50
\$\begingroup\$
    var firstDate = (DateTime)datePropertyInfo.GetValue(stateModels.First());

    if (firstDate < dateFrom)
      dateFrom = new DateTime(firstDate.Year, firstDate.Month, firstDate.Day, firstDate.Hour, 0, 0, DateTimeKind.Utc);

You take the first stateModel, but what if they are not ordered by date?


for (itemPointer = itemPointer; itemPointer < stateModels.Count(); itemPointer++)

itemPointer = itemPointer isn't neccesary:

for (; itemPointer < stateModels.Count(); itemPointer++)

The last parameter to CalculateTimeWindow<T>(..., ..., int i) is initialized with the out int i parameter from GetPropertiesAndDictionaries(), but the values is actually not used and i is used as an iterator index set to 0 the first time used, so why have it as a parameter and not just a local variable? Get rid of that, if it's not used.


This:

    public static IEnumerable<T> CalculateTimeWindows<T>(DateTime dateFrom, DateTime dateTo, List<T> stateModels) where T : new()
    {
        if (stateModels.Count() == 0)
            return new List<T>();

        dateFrom = GetPropertiesAndDictionaries(dateFrom, stateModels, out PropertyInfo datePropertyInfo, out List<PropertyInfo> copyProperties, out PropertyInfo timeWindowIdentifier, out int size, out TimeWindowDictionary[] dictionaries, out int i);

        return CalculateTimeWindow(dateFrom, dateTo, stateModels, 5, datePropertyInfo, copyProperties, timeWindowIdentifier, size, dictionaries, i)
                    .Concat(CalculateTimeWindow(dateFrom, dateTo, stateModels, 15, datePropertyInfo, copyProperties, timeWindowIdentifier, size, dictionaries, i))
                    .Concat(CalculateTimeWindow(dateFrom, dateTo, stateModels, 60, datePropertyInfo, copyProperties, timeWindowIdentifier, size, dictionaries, i));
    }

I would write as:

  public static IEnumerable<T> CalculateTimeWindows<T>(DateTime dateFrom, DateTime dateTo, List<T> stateModels) where T : new()
  {
    if (stateModels == null || stateModels.Count() == 0)
      yield break; // return new List<T>();

    dateFrom = GetPropertiesAndDictionaries(
      dateFrom, 
      stateModels, 
      out PropertyInfo datePropertyInfo, 
      out List<PropertyInfo> copyProperties, 
      out PropertyInfo timeWindowIdentifier, 
      out int size, 
      out TimeWindowDictionary[] dictionaries, 
      out int i);

    byte[] windowDurations = { 5, 15, 60 };

    foreach (byte duration in windowDurations)
    {
      foreach (T result in CalculateTimeWindow(dateFrom, dateTo, stateModels, duration, datePropertyInfo, copyProperties, timeWindowIdentifier, size, dictionaries, i))
      {
        yield return result;
      }
    }

or as:

  public static IEnumerable<T> CalculateTimeWindows<T>(DateTime dateFrom, DateTime dateTo, List<T> stateModels) where T : new()
  {
    if (stateModels.Count() == 0)
      return new List<T>();

    dateFrom = GetPropertiesAndDictionaries(
      dateFrom,
      stateModels,
      out PropertyInfo datePropertyInfo,
      out List<PropertyInfo> copyProperties,
      out PropertyInfo timeWindowIdentifier,
      out int size,
      out TimeWindowDictionary[] dictionaries,
      out int i);

    byte[] windowDurations = { 5, 15, 60 };

    return windowDurations.SelectMany(wd =>
      CalculateTimeWindow(
        dateFrom,
        dateTo,
        stateModels,
        wd,
        datePropertyInfo,
        copyProperties,
        timeWindowIdentifier,
        size,
        dictionaries,
        i));
   }

It is much more readable and easy to maintain if you want to add a new time window.


    copyProperties = propInfos
      .Where(
        p => 
          !p.GetCustomAttributes(typeof(IsTimeWindowIdentifier), true).Any() 
          && !p.GetCustomAttributes(typeof(DoNotCopyIntoTimeWindow), true).Any() 
          && !p.GetCustomAttributes(typeof(IsTimeWindowDate), true).Any() 
          && !p.GetCustomAttributes(typeof(IsProcessedIntoTimeWindow), true).Any() 
          && p.CanWrite 
          && !p.GetMethod.IsVirtual).ToList();

In the above you determine which properties to fetch data from in a negative way by checking what they are not. I think, I would define a ValuePropertyAttribute to mark the properties to get data from with. It is easier to maintain, because you'll always be able to see from this attribute which properties values are copied from on an object.


When handling properties (setting and getting) there is a little too much conversion between PropertyInfos and strings (names) IMO.

You can reduce that by changing the definition of:

  public struct TimeWindowDictionary
  {
    public string Name { get; set; }
    public Dictionary<NullObject<dynamic>, int> Dictionary { get; set; }
  }

to

  public struct TimeWindowDictionary
  {
    public PropertyInfo PropertyInfo { get; set; }
    public Dictionary<NullObject<dynamic>, int> Dictionary { get; set; }
  }

and then change everything to accomodate to that - for instance:

    var properties = propInfos.Where(p => p.GetCustomAttributes(typeof(IsProcessedIntoTimeWindow), true).Any()).Select(x => x.Name);
    size = properties.Count();

    dictionaries = new TimeWindowDictionary[size];
    i = 0;

    foreach (var property in properties)
    {
      dictionaries[i] = new TimeWindowDictionary() { Name = property };
      i++;
    }

could be:

    dictionaries =
      propInfos
      .Where(p => p.GetCustomAttributes(typeof(IsProcessedIntoTimeWindow), true).Any())
      .Select(p => new TimeWindowDictionary { PropertyInfo = p })
      .ToArray();

    size = dictionaries.Length;

and GetValue(...) could be:

  private static dynamic GetValue(object inputObject, PropertyInfo propertyInfo)
  {
    return propertyInfo.GetValue(inputObject);
    //Type type = inputObject.GetType();
    //System.Reflection.PropertyInfo propertyInfo = type.GetProperty(propertyName);
    //return propertyInfo.GetValue(inputObject);
  }

and SetValue(...) could likewise have the signature:

  private static void SetValue(object inputObject, PropertyInfo propertyInfo, object propertyVal)

and in CalculateStateSeconds<T>(...):

        //PropertyInfo propertyInfo = tType.GetProperty(dictionary.Name);
        var key = dictionary.PropertyInfo.GetValue(prevItem);
        dictionary.Dictionary.TryGetValue(key, out int existingSeconds);
        dictionary.Dictionary[key] = existingSeconds + seconds;

  private static DateTime GetPropertiesAndDictionaries<T>(
    DateTime dateFrom,
    List<T> stateModels,
    out PropertyInfo datePropertyInfo,
    out List<PropertyInfo> copyProperties,
    out PropertyInfo timeWindowIdentifier,
    out int size,
    out TimeWindowDictionary[] dictionaries,
    out int i) where T : new()
  {

When having all these arguments passed from method to method, I think I would make a container object for them and pass that instead. Alternatively you could make the class statefull with these parameters as members instead, so you can avoid having arguments to the methods at all. The methods should then be non static members of course. Your one and only static method could then be something like:

  public static IEnumerable<T> GetTimeWindows<T>(DateTime dateFrom, DateTime dateTo, List<T> stateModels) where T : new()
  {
    TimeWindow timeWindow = new TimeWindow(dateFrom, dateTo, stateModels);
    return timeWindow.Calculate();
  }

where the TimeWindow class should have an appropriate generric type parameter.

\$\endgroup\$
  • 1
    \$\begingroup\$ Wow thanks! I'll go through them one by one but I have an answer to your first question. You take the first stateModel, but what if they are not ordered by date? Good observation, but I am sure that they are sorted by date, so (purely) for optimization purposes I'm not re-checking. \$\endgroup\$ – Mark Cilia Vincenti Jun 14 at 13:20
  • 1
    \$\begingroup\$ I'm going to be testing your changes. One thing I'm going to add right afterwards (after I've taken benchmarks) is to store the properties in a singleton dictionary where the key is the Type, such that getting the properties through reflection happens only once for each entity. \$\endgroup\$ – Mark Cilia Vincenti Jun 14 at 14:25
  • \$\begingroup\$ @MarkCiliaVincenti: Sound good, if I understand you right. When done, you could consider to post an self-answer, with the updated code. \$\endgroup\$ – Henrik Hansen Jun 14 at 14:34
  • \$\begingroup\$ Ah that's what you usually do? I was going to edit my original post. OK. \$\endgroup\$ – Mark Cilia Vincenti Jun 14 at 14:53
  • 2
    \$\begingroup\$ @MarkCiliaVincenti: Yes, It's done now and then that way. On the other hand you are not allowed to edit the question, when answers are posted, because that would invalidate them. \$\endgroup\$ – Henrik Hansen Jun 14 at 15:16
4
\$\begingroup\$

Alternative approach

Instead of using attributes and reflection, I'd go for a set of generic methods that take type-specific date and value-selector methods as parameters.

First, a method that returns items grouped by time segment:

public static IEnumerable<TimeSegmentItems<T>> GetItemsPerTimeSegment(
    IEnumerable<T> items,
    DateTime from,
    DateTime to,
    TimeSpan segmentDuration,
    Func<T, DateTime> getTimestamp) { ... }

// Contains all items within a given time segment,
// as well as the last item from the preceding time segment
// (that item determines the initial state):
public class TimeSegmentItems<T>
{
    public DateTime Start { get; }
    public DateTime End { get; }
    public IEnumerable<T> Items { get; }
}

Then, a utility method for obtaining the dominant value for a given time segment:

public static TValue GetDominantValue<TItem, TValue>(
    IEnumerable<TItem> items,
    DateTime from,
    DateTime to,
    Func<TItem, DateTime> getTimestamp,
    Func<TItem, TValue> getValue) { ... }

Together, they allow you to do the following:

GetItemsPerTimeSegment(myValues, fromDate, toDate, TimeSpan.FromMinutes(5), m => m.Date)
    .Select(segment => new MyModel {
        Value = GetDominantValue(segment.Items, segment.Start, segment.End, m => m.Date, m => m.Value),
        ActuallyAnEnum = GetDominantValue(segment.Items, segment.Start, segment.End, m => m.Date, m => m.ActuallyAnEnum),
    });

Which can further be streamlined, but that's the gist of it.

Comparison

Comparing this to your original approach:

  • Reflection and dynamic are both relatively slow, and especially with dynamic you lose compile-time checking. This approach should be more succinct, more robust and (much) faster.
  • With attributes, you are limited to types that you can add attributes to. This approach lets you work with any type, and even create different 'views' for the same type, at the cost of having to write type-specific logic (the date and value selectors, and the final result-building code).
  • Instead of a single public method that does everything, these utility methods each focus on a specific task, which makes them more reusable. You can swap the dominant-value logic for average-value logic without having to touch the time-segment grouping logic. You can also build an attribute/reflection-based layer on top of this that removes the need for type-specific code, if necessary.
\$\endgroup\$
  • \$\begingroup\$ I would also focus on an alternative than to analyse the OP's code for refactoring. \$\endgroup\$ – dfhwze Jun 15 at 18:35
  • \$\begingroup\$ It depends. If you want to include another property then yes, you'll need to add a GetDominantValue call for that - similar to how, with your approach, you'd need to add an attribute to that property. If you rename a property or change its type then no manual changes are required. \$\endgroup\$ – Pieter Witvoet Jun 18 at 7:54
  • \$\begingroup\$ Interesting, but I'm not sure I understand. I have fixed the problems with reflection (I think) by storing the results in a concurrent dictionary (code posted at codereview.stackexchange.com/questions/222505/…). What I don't understand with your solution is whether or not I would need to edit some code whenever the database models change. That was the idea behind using reflection, to avoid problems with developers forgetting to also do changes elsewhere. \$\endgroup\$ – Mark Cilia Vincenti Jun 18 at 7:55
  • \$\begingroup\$ I see @PieterWitvoet, thanks, I'll give it some thought, though to be honest I still think it's more difficult for myself and colleagues to forget making a call to GetDominantValue than adding an attribute to a property given that all the lines above and below it would have attributes. \$\endgroup\$ – Mark Cilia Vincenti Jun 18 at 7:58
  • 1
    \$\begingroup\$ That makes sense. As I said, you can still put an attribute-based layer on top of these methods if necessary. You'd still benefit from a better separation of concerns. \$\endgroup\$ – Pieter Witvoet Jun 18 at 8:25
4
\$\begingroup\$

I found something that looks really weird to me

    for (i = 0; i < dictionary.Dictionary.Count; i++)
    {
        maxValue = dictionary.Dictionary.First();
        for (j = 1; j < dictionary.Dictionary.Count; j++)
        {
            var valuePair = dictionary.Dictionary.ElementAt(j);

            if (valuePair.Value > maxValue.Value)
                maxValue = valuePair;
        }
    }
    var valToSet = maxValue.Key.Item;

This looks like it could be replaced with

var valToSet = dictionary.Values.Max();

if you are trying to get the key with the max value you could use this

var valToSet = dictionary.OrderByDescending(x => x.Value).First().Key;

found on https://stackoverflow.com/a/42508315/1214743 be sure to test around values that are the same.


I also found something else that I would do differently as well

  if (sameAsPrevious)
  {
      var prevVal = GetValue(prevTimeWindow, dictionary.Name);

      if (valToSet == null && prevVal == null)
      { }
      else if ((valToSet == null && prevVal != null) || (valToSet != null && prevVal == null) || !valToSet.Equals(prevVal))
          sameAsPrevious = false;
  }

your if/else statement could be written like this:

if (valToSet == null && prevVal == null)
{ 
}
else
{
    sameAsPrevious = (valToSet == preval);
}

because

  • if valToSet is null and the other has a value it won't be the same, it also works the other way around. This would make sameAsPrevious == false
  • if they are both null they get caught in the initial if statement
  • if they are both have a value and it is the same we want the sameAsPrevious value to be true anyway. if valToSet == preval will set sameAsPrevious to true, which it should be in that situation.
\$\endgroup\$
  • 3
    \$\begingroup\$ I am sure you mean !(valToSet != preval) or valToSet == preval. Or what is !(valToSet <> preval)? \$\endgroup\$ – dfhwze Jun 14 at 4:42
  • \$\begingroup\$ Cheers I'll run some tests on both today :) That first part looks really weird to me too. I wrote the code over a year ago and to be honest I can't understand what I was doing there either, because it seems to me like there's an extra loop for (i = 0; i < dictionary.Dictionary.Count; i++) which seems like it's completely useless. \$\endgroup\$ – Mark Cilia Vincenti Jun 14 at 8:22
  • \$\begingroup\$ Also, I'm assuming that for the first part you meant var valToSet = dictionary.Dictionary.Max().Key.Item; \$\endgroup\$ – Mark Cilia Vincenti Jun 14 at 8:53
  • 2
    \$\begingroup\$ I can't understand what I was doing there either o_O so how can we? @MarkCiliaVincenti \$\endgroup\$ – t3chb0t Jun 14 at 12:19
  • 1
    \$\begingroup\$ @dfhwze, thank you for pointing that out, I put the not equals from SQL in there and see what you mean...oops. it is fixed now \$\endgroup\$ – Malachi Jun 14 at 14:25

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.