Does this expression require optimization or not?

Question

I don't like the look of this function. Is there a way to make it look less ugly without "magic strings".

private static bool Inconsistent(AdStats adStat)   {
  return 
    adStat.Daily.Impressions != adStat.Hourly.Sum(h => h.Value.Impressions) ||
    adStat.Daily.Clicks != adStat.Hourly.Sum(h => h.Value.Clicks) ||
    adStat.Daily.Spent != adStat.Hourly.Sum(h => h.Value.Spent) ||
    adStat.Daily.SocialImpressions != adStat.Hourly.Sum(h => h.Value.SocialImpressions) ||
    adStat.Daily.SocialClicks != adStat.Hourly.Sum(h => h.Value.SocialClicks) ||
    adStat.Daily.SocialSpent != adStat.Hourly.Sum(h => h.Value.SocialSpent) ||
    adStat.Daily.UniqueImpressions != adStat.Hourly.Sum(h => h.Value.UniqueImpressions) ||
    adStat.Daily.UniqueClicks != adStat.Hourly.Sum(h => h.Value.UniqueClicks) ||
    adStat.Daily.SocialUniqueImpressions != adStat.Hourly.Sum(h => h.Value.SocialUniqueImpressions) ||
    adStat.Daily.SocialUniqueClicks != adStat.Hourly.Sum(h => h.Value.SocialUniqueClicks);
}

When people say "optimize", they mean making the code run faster, or making it use less memory. Sometimes making the code itself take up less space. Making it more readable is just called making it more readable. — 15ee8f99-57ff-4f92-890c-b56153, Jun 05 '13 at 13:39
You could insert spaces to make the `!=` (and on) parts line up. That might make it less ugly visually, though still just as "ugly" in the sense of needing the properties to match for the method to be correct. That can be ensured by unit tests that use "magic strings". — Tim S., Jun 05 '13 at 13:43
@GrantWinney, i meant, that i could've used reflection and pass a list of properties to update. EdPlunkett, what i really want is DRY basically. — Causarium, Jun 05 '13 at 13:45

Ani · Accepted Answer · 2013-06-05T13:54:21.470

I think by "optimization", you mean "redundancy reduction" aka Don't Repeat Yourself?

Essentially, you have a bunch of metrics. You want to check if, for any of those metrics, the value of that metric for an ad on a daily basis deviates from the sum of that metric on an hourly basis.

Once you think of it that way, you can do:

Func<Stat, int>[] metricGetters = 
{
   stat => stat.Impressions,
   stat => stat.Clicks,
   // .. etc. etc.
};

return metricGetters.Any(getter => getter(adStat.Daily) 
                                != adStat.Hourly.Sum(h => getter(h.Value)));

score 1 · Answer 2 · answered Jun 05 '13 at 13:45

you could extract adStat.Hourly.Select(h => h.Value) to variable, it would reduce code amount:

 private static bool Inconsistent(AdStats adStat)
    {
        var hourlyValue = adStat.Hourly.Select(x => x.Value);
        return
          adStat.Daily.Impressions != hourlyValue.Sum(h => h.Impressions) ||
          adStat.Daily.Clicks != hourlyValue.Sum(h => h.Clicks) ||
          adStat.Daily.Spent != hourlyValue.Sum(h => h.Spent) ||
          adStat.Daily.SocialImpressions != hourlyValue.Sum(h => h.SocialImpressions) ||
          adStat.Daily.SocialClicks != hourlyValue.Sum(h => h.SocialClicks) ||
          adStat.Daily.SocialSpent != hourlyValue.Sum(h => h.SocialSpent) ||
          adStat.Daily.UniqueImpressions != hourlyValue.Sum(h => h.UniqueImpressions) ||
          adStat.Daily.UniqueClicks != hourlyValue.Sum(h => h.UniqueClicks) ||
          adStat.Daily.SocialUniqueImpressions != hourlyValue.Sum(h => h.SocialUniqueImpressions) ||
          adStat.Daily.SocialUniqueClicks != hourlyValue.Sum(h => h.SocialUniqueClicks);
    }

I also like Ani idea, it allows even inject additional conditions from outside.

Does this expression require optimization or not?

2 Answers2