2
\$\begingroup\$

Could you help me to speed up/optimize this code:

// Get API Keys
var keys = xRoot.Elements("key").Select(key => new { ID = int.Parse(key.Attribute("id").Value), VerificationCode = key.Attribute("verificationCode").Value });

// Get status of account using API Key
var tasks = keys.Select(key => EveOnlineClient.GetAccountStatusAsync(key.ID, key.VerificationCode));

// Get info about API Key
// Each API key may contain one or more game characters
var tasks2 = keys.Select(key => EveOnlineClient.GetAPIKeyInfoAsync(key.ID, key.VerificationCode).ContinueWith(x =>
    {
        var characters = x.Result.Characters;

        // Get the sheet for each character in each API key
        var tasks3 = characters.Select(character => EveOnlineClient.GetCharacterSheetAsync(key.ID, key.VerificationCode, character.ID));

        // Get skill in training for each character in each API key
        var tasks4 = characters.Select(character => EveOnlineClient.GetSkillInTrainingAsync(key.ID, key.VerificationCode, character.ID));

        Task.WaitAll(tasks3.ToArray());
        Task.WaitAll(tasks4.ToArray());
    }));

await Task.WhenAll(tasks.Concat(tasks2));

Every method of EveOnlineClient downloads XML to local cache if needed (if local cache has expired), parses it and returns as an object.

\$\endgroup\$
3
\$\begingroup\$
int.Parse(key.Attribute("id").Value)

XAttribute (and XElement) supports implicit cast to int (and other types), this means you can write just:

(int)key.Attribute("id")

One more advantage of this approach is that it uses the invariant culture, unlike the Parse() methods, which use the current culture by default. This is especially important for decimal values, since many cultures use , as the decimal separator, not ..

And if you're going to use casts for numeric types, you might want to use them for string too for consistency, instead of accessing Value.


var tasks
var tasks2
var tasks3
var tasks4

Those are not very good variable names, try to make names more descriptive, like accountStatusTasks.

Also, it's hard to advise more about the structure of the code when you don't use all those tasks in any way, except to wait for them. I assume that's not all they're good for.


There are not many cases where ContinueWith() is useful, when you can use await. Here, I would make the Select() lambda async, which also means you can use await Task.WhenAll() instead of Task.WaitAll(), which blocks a thread unnecessarily:

var tasks2 = keys.Select(async key =>
{
    var keyInfo = await EveOnlineClient.GetAPIKeyInfoAsync(key.ID, key.VerificationCode);

    var characters = keyInfo.Characters;

    // Get the sheet for each character in each API key
    var tasks3 = characters.Select(character => EveOnlineClient.GetCharacterSheetAsync(key.ID, key.VerificationCode, character.ID));

    // Get skill in training for each character in each API key
    var tasks4 = characters.Select(character => EveOnlineClient.GetSkillInTrainingAsync(key.ID, key.VerificationCode, character.ID));

    await Task.WhenAll(tasks3.Concat(tasks4));
});

Quite often, code that makes many requests to a single server in parallel is limited by ServicePointManager.DefaultConnectionLimit, which by default is set to just 2. But if you decide to change this, make sure to follow any guidelines about rate limiting of the API you're using; otherwise, you might easily end up blocked.

\$\endgroup\$
  • \$\begingroup\$ Using Parse() is not a wrong approach. The connected client might have a culture set which is valid in that context. With your approach, "123,456" could mean 123456 or 123.456. Very different values. \$\endgroup\$ – Vince Panuccio Aug 7 '14 at 1:44
  • 1
    \$\begingroup\$ @VincePanuccio No, with my approach, "123,456" will be always invalid, because the cast uses . as decimal separator and no thousand separator. If the format of the XML was culture-dependent, that would be a terrible design, since you couldn't safely transfer it from one computer to another. \$\endgroup\$ – svick Aug 7 '14 at 8:31

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.