4
\$\begingroup\$

The task is to determine the region by comparing three sources of information taking into account the coefficients of their importance.

//GetRegionIdByKeywordAsync returns region id by string name, if the region could not be determined, 0 will return

//The received identifiers can completely or partially coincide, or be absolutely different
int regionIdByIp = await GetRegionIdByKeywordAsync(regionByIpStr);
int regionIdByPhone = await GetRegionIdByKeywordAsync(regionByPhoneStr);
int regionIdByInput = await GetRegionIdByKeywordAsync(regionByInputStr);

var weights = new List<KeyValuePair<int, int>>
{
  //Each source of information has its own importance coefficient, determined manually
  new KeyValuePair<int, int>(regionIdByIp, 5),
  new KeyValuePair<int, int>(regionIdByPhone, 3),
  new KeyValuePair<int, int>(regionIdByInput, 4)
};

//The final decision is determined by the search for a region that has the highest score in the total
int regionId = weights.Where(x => x.Key != default(int)).GroupBy(kvp => kvp.Key).OrderByDescending(x => x.Sum(y => y.Value)).Select(x => x.Key).FirstOrDefault();

Do you see any improvement / issue?

\$\endgroup\$
4
\$\begingroup\$

Since you are not actually using a dictionary I find anonymous tuples would be much more readable here. Consider this where you don't even need a List<T> but just a simple array:

var weights = new (int RegionId, int Importance)[]
{
  (regionIdByIp, 5),
  (regionIdByPhone, 3),
  (regionIdByInput, 4)
};

Where you define such a tuple as (int RegionId, int Importance). With it the next query (also written with line breaks) would be much easier to understand.

int mostImportantRegionId = 
    weights
        .Where(x => x.RegionId > 0)
        .GroupBy(kvp => kvp.RegionId)
        .OrderByDescending(x => x.Sum(y => y.Importance))
        .Select(x => x.RegionId)
        .FirstOrDefault();

I also think that x.Key != default(int) is a strage condition. If Key is an id then it probably should be greater than 0 so it's better to write x.RegionId > 0 instead.


What you also can do better is the await part. Currently you wait for each task to finish one-by-one. Let's them work in parallel with WaitAll:

var regionIdByIpTask = GetRegionIdByKeywordAsync(regionByIpStr);
var regionIdByPhoneTask = GetRegionIdByKeywordAsync(regionByPhoneStr);
var regionIdByInputTask = GetRegionIdByKeywordAsync(regionByInputStr);

Task.WaitAll(new [] 
{
    regionIdByIpTask,
    regionIdByPhoneTask,
    regionIdByInputTask
};

Now you can build your weights:

var weights = new (int RegionId, int Importance)[]
{
  (regionIdByIpTask.Result, 5),
  (regionIdByPhoneTask.Result, 3),
  (regionIdByInputTask.Result, 4)
};
\$\endgroup\$
  • \$\begingroup\$ No need to even Task.WaitAll - calling .Result blocks until the async action is complete. \$\endgroup\$ – Der Kommissar Aug 29 '17 at 12:50
  • \$\begingroup\$ @EBrown yes, but you don't want to wait for one task at a time but for all of them at the same time. With .Result they will run one after another and not in parallel, won't they? \$\endgroup\$ – t3chb0t Aug 29 '17 at 12:53
  • \$\begingroup\$ Not true: if the tasks have already started then calling .Result in sequence like that effectively has the same effect as calling Task.WaitAll because they need to finish, and if you call .Result on a complete task it returns immediately. If Input finishes first, great, but that doesn't help because we still need Ip to finish. Of course, now Phone is irrelevant (because it's a lower priority) so a more clever answer might utilize the priority to effectively cancel tasks that are now irrelevant. \$\endgroup\$ – Der Kommissar Aug 29 '17 at 12:55
3
\$\begingroup\$

As you're currently in an async method, you should also user Task.WhenAll to wait for these three sub tasks to finish asynchronously.

var regionIdByIpTask = GetRegionIdByKeywordAsync(regionByIpStr);
var regionIdByPhoneTask = GetRegionIdByKeywordAsync(regionByPhoneStr);
var regionIdByInputTask = GetRegionIdByKeywordAsync(regionByInputStr);

await Task.WhenAll(new [] 
{
    regionIdByIpTask,
    regionIdByPhoneTask,
    regionIdByInputTask
};

Task.WhenAll Method

Creates a task that will complete when all of the supplied tasks have completed.

Source: msdn.microsoft.com


Edit to clarify:

Task.WaitAll will actively wait for the completion of all sub tasks and thus block the current thread.

Task.WhenAll will return Task which can be awaited. Awaiting the task will "free" the current thread to process different tasks.

\$\endgroup\$
  • \$\begingroup\$ Haven't I already said that? :-] \$\endgroup\$ – t3chb0t Aug 30 '17 at 9:42
  • \$\begingroup\$ You used Task.WaitAll which will block the current thread until all tasks are completed. Task.WhenAll will await all tasks to complete, so the Thread won't be blocked. \$\endgroup\$ – Limeray Aug 30 '17 at 10:55
  • \$\begingroup\$ Oh, I see I cannot read, sorry ;-) \$\endgroup\$ – t3chb0t Aug 30 '17 at 10:56
  • \$\begingroup\$ Yeah the calls are very similar. I've added a clarification to my awnser. \$\endgroup\$ – Limeray Aug 30 '17 at 11:00
0
\$\begingroup\$

I have just one point to add to those already made:

.OrderByDescending(x => x.Sum(y => y.Value)).Select(x => x.Key).FirstOrDefault();

A full sort is overkill when the only element required is the largest. Finding largest elements by a projection is a relatively common operation. Both are good reasons for creating a MaxBy extension method (or using MoreLINQ's) - although here you really want

MaxByOrDefault(x => x.Sum(y => y.Value), x => x.Key)
\$\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.