11
\$\begingroup\$

My current condition is written as follows:

.Where(m => Math.Round((m.DateAndTime - DateTime.Now).TotalMinutes, 0) <= 30 && Math.Round((m.DateAndTime - DateTime.Now).TotalMinutes, 0) > 0)

which basically says where the rounded value is less than or equal to 30 and the rounded value is greater than 0.

Is there a neater way to write this than having

Math.Round((m.DateAndTime - DateTime.Now).TotalMinutes, 0)

twice in the same statement?

The full expression is:

List<Meeting> finalList = initialList.Where(m => Math.Round((m.DateAndTime - DateTime.Now).TotalMinutes, 0) <= 30 && Math.Round((m.DateAndTime - DateTime.Now).TotalMinutes, 0) > 0).ToList();
\$\endgroup\$
  • 4
    \$\begingroup\$ I really wanted to comment on all the answers, but I have always found it easier to read 0 < value && value <= 30 in those languages that don't allow you to write 0 < value <= 30 It makes it much clearer that you're testing for something to be in a specified range. \$\endgroup\$ – Tom Tanner Jul 22 '15 at 12:46
  • \$\begingroup\$ @TomTanner, a valid point and I'd normally do the same. I kept my order the same as the OP's though to avoid causing possible confusion. \$\endgroup\$ – David Arno Jul 22 '15 at 12:55
22
\$\begingroup\$

If you need to store a result of a method you should just create a method for it.

private bool IsInRange(DateTime dateTime, decimal min, decimal max)
{
    decimal totalMinutes = Math.Round((dateTime - DateTime.Now).TotalMinutes, 0);
    return totalMinutes > min && totalMinutes <= max;
}  

now your expression looks like so

.Where(m => IsInRange(m.DateAndTime, 0, 30));  

but the values 0 and 30 are still magic numbers which should be extracted to meaningful constants.

The choosen method name is not the best like @Falco has correctly pointed out in his comment

UhhOhhh Horribly wrong named Function.... AreMinutesInRangeFromNow or something like this would be better - with your Method-Name I would expect no Magic happening with DateTime.Now - or pass the already substracted value, but don't hide this!

so you should change it to a better name. I don't like the name @Falco suggested either but couldn't come up with a different one. So it is up to you to choose one.

\$\endgroup\$
  • \$\begingroup\$ Oh, weird, I always get errors like Method 'Boolean functionName(System.String, System.String)' has no supported translation to SQL. I figured you just couldn't call functions in linq queries. \$\endgroup\$ – Charles Clayton Jul 22 '15 at 21:50
  • 2
    \$\begingroup\$ I would expect min to come before max in the parameter list. \$\endgroup\$ – BlueRaja - Danny Pflughoeft Jul 22 '15 at 22:09
  • \$\begingroup\$ @BlueRaja-DannyPflughoeft you are right. Updated answer. \$\endgroup\$ – Heslacher Jul 23 '15 at 4:58
  • 4
    \$\begingroup\$ UhhOhhh Horribly wrong named Function.... AreMinutesInRangeFromNow or something like this would be better - with your Method-Name I would expect no Magic happening with DateTime.Now - or pass the already substracted value, but don't hide this! \$\endgroup\$ – Falco Jul 23 '15 at 8:32
  • \$\begingroup\$ @crclayton You can call functions in linq queries as long as you're not querying the database itself in your query, otherwise it thinks you're trying to translate things to SQL, so what I do is pull the list from the db, then run linq queries on that list object. \$\endgroup\$ – barnacle.m Jul 23 '15 at 10:18
15
\$\begingroup\$

One way is to use query-style LINQ and the let keyword:

var finalList = (from m in initialList
                 let value = Math.Round((m.DateAndTime - DateTime.Now).TotalMinutes
                 where value <= 30 && value > 0
                 select m).ToList();
\$\endgroup\$
  • \$\begingroup\$ Why would you rewrite it to query-style, when the asker clearly prefers method-style? \$\endgroup\$ – ANeves Jul 22 '15 at 12:46
  • 3
    \$\begingroup\$ @ANeves, why do you assume the OP prefers the fluent style? I use both styles, depending on the requirement. Further, these answers aren't just for the benefit of the OP, but for others who come searching with similar questions. They may prefer the query syntax; or they may not. Having said all that, if the OP doesn't like using query syntax, then Johnbot has a viable alternative solution that sticks to the fluent style. \$\endgroup\$ – David Arno Jul 22 '15 at 12:53
  • \$\begingroup\$ I agree that I should not assume preference. But it is what he is using in the question; I still think that using query-style distracts from the actual solution. \$\endgroup\$ – ANeves Jul 22 '15 at 12:56
  • 1
    \$\begingroup\$ @ANeves there's not really an equivalent to this using fluent syntax and the closest way is a bit clunky. As far as function goes, this is the most elegant. Form-wise, it's up to the individual's aesthetic preference. \$\endgroup\$ – moarboilerplate Jul 22 '15 at 17:47
  • 1
    \$\begingroup\$ @moarboilerplate If you look at the IL from this answer and mine you'll see that they're identical except for the anonymous type and delegate names. \$\endgroup\$ – Johnbot Jul 23 '15 at 9:17
10
\$\begingroup\$

Write an IsBetween extension method:

public static bool IsBetween<T>(this T item, T start, T end)
{
    return Comparer<T>.Default.Compare(item, start) >= 0
        && Comparer<T>.Default.Compare(item, end) <= 0;
}

And use it like this:

.Where(m => Math.Round((m.DateAndTime - DateTime.Now).TotalMinutes, 0).IsBetween(0, 30))

(And also take into account the remarks WRT magic numbers and UtcNow etc.)

\$\endgroup\$
8
\$\begingroup\$

Use a block-lambda and extract a variable

You can use a block-lambda instead of a statement lambda, and extract a variable:

var now = DateTime.Now;
List<Meeting> finalList = initialList.Where(m => {
    var minutesToNow = Math.Round((m.DateAndTime - now).TotalMinutes, 0);
    return minutesToNow <= 30 && minutesToNow > 0
}).ToList();

Alternatively, use a method like Heslacher suggested.

Use UtcNow

You should avoid using local times, and use Utc times. Prefer DateTime.UtcNow to DateTime.Now.

\$\endgroup\$
  • \$\begingroup\$ Good idea on avoiding local times if I ever want my software to scale internationally! \$\endgroup\$ – barnacle.m Jul 23 '15 at 10:16
5
\$\begingroup\$

You can introduce an additional Select where you calculate the offset from Now:

var finalList =
    initialList.Select(m =>
                        new
                            {
                                Meeting = m,
                                MinutesOffsetFromNow = Math.Round((m.DateAndTime - DateTime.Now).TotalMinutes, 0)
                            })
                .Where(x => x.MinutesOffsetFromNow <= 30 && x.MinutesOffsetFromNow > 0)
                .Select(x => x.Meeting)
                .ToList();

And then transform it back to only a Meeting just before the ToList.


Doing calculations correctly with time is hard using just DateTime, especially if you have to consider time zone changing as with daylight saving time or users in different time zones.

You should consider if Noda Time can help you express you intents more clearly.

\$\endgroup\$
2
\$\begingroup\$

You can simplify your condition, and not use Math.Round (which is really not needed here). It will be more concise and readable:

dt > DateTime.Now && dt <= DateTime.Now.AddMinutes(30)
\$\endgroup\$
  • \$\begingroup\$ Oh there's a very important reason I use Math.Round, but that has nothing to do with the question \$\endgroup\$ – barnacle.m Jul 23 '15 at 10:17

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.