3
\$\begingroup\$

I'm working on an application that exports and imports work items from Azure DevOps. To import work items into Azure DevOps, I have written the following code.

Would you be able to review and make suggestions? I'm unsure whether I've used the Async-Await pattern correctly.

namespace Export_Import_AzureDevOps.Factory
{
    public class ImportFactory : IImportFactory
    {
        private ConcurrentDictionary<int, int> idMapper = new ConcurrentDictionary<int, int>();
        private readonly ILogger<ImportFactory> _logger;
        private readonly Devops _devopsConfiguration;
        private readonly IImportService<WorkItemCore> _importWorkItemService;
        private readonly IImportService<SprintCore> _importSprintService;

        public ImportFactory(ILogger<ImportFactory> logger, IConfiguration configuration, IImportService<SprintCore> importSprintService, IImportService<WorkItemCore> importWorkItemService)
        {
            _logger = logger;
            _devopsConfiguration = configuration.GetSection(nameof(Devops)).Get<Devops>();
            _importSprintService = importSprintService;
            _importWorkItemService = importWorkItemService;
        }

        private void PublishSprints(string id)
        {
            var sprintPublishURL = $"{_devopsConfiguration.Target.ProjectId}/{_devopsConfiguration.Target.TargetTeamId}/_apis/work/teamsettings/iterations?api-version={_devopsConfiguration.APIVersion}";

            var jsonPublishSprintString = $@"{{""id"":""{id}""}}";

            HttpContent publishSprintContent = new StringContent(jsonPublishSprintString, Encoding.UTF8, "application/json");

            _importSprintService.Post(sprintPublishURL, publishSprintContent).ConfigureAwait(false);
        }

        private Task CreateSprints(Sprints sprints)
        {
            var sprintCreationURL = $"{_devopsConfiguration.Target.ProjectId}/_apis/wit/classificationNodes/Iterations?api-version={_devopsConfiguration.APIVersion}";

            foreach (Sprint sprint in sprints.value)
            {
                var jsonString = JsonConvert.SerializeObject(sprint);
                HttpContent content = new StringContent(jsonString, Encoding.UTF8, "application/json");

                var result = _importWorkItemService.Post(sprintCreationURL, content).Result;

                PublishSprints(result.identifier);
            }

            return Task.CompletedTask;
        }

        private int GetCurrentIdEquivalentId(int oldId)
        {
            return idMapper[oldId];
        }

        private void TryAddIdMapper(int oldId, int newId)
        {
            if (!idMapper.ContainsKey(oldId))
            {
                idMapper.TryAdd(oldId, newId);
            }
        }

        private int FindParentId(WorkItemDetails workItemDetails)
        {
            var parentId = 0;

            if (workItemDetails.relations != null)
            {
                var parentRelation = workItemDetails.relations.Where(relation => relation.attributes.name.Equals("Parent")).FirstOrDefault();

                if (parentRelation != null)
                {
                    parentId = Int32.Parse(parentRelation.url.Split("/")[parentRelation.url.Split("/").Length - 1]);
                }
            }

            return parentId;
        }

        private void CreateWorkItem(string url, WorkItem workItem)
        {
            List<WorkItemOperation> operations = new List<WorkItemOperation>();
            var parentId = FindParentId(workItem.details);

            operations.Add(
                    new WorkItemOperation()
                    {
                        path = "/fields/System.Title",
                        value = workItem.details.fields.Title ?? string.Empty
                    }
                );
            operations.Add(
                    new WorkItemOperation()
                    {
                        path = "/fields/System.Description",
                        value = workItem.details.fields.Description ?? string.Empty
                    }
                );
            operations.Add(
                    new WorkItemOperation()
                    {
                        path = "/fields/Microsoft.VSTS.Common.AcceptanceCriteria",
                        value = workItem.details.fields.AcceptanceCriteria ?? string.Empty
                    }
                );
            operations.Add(
                    new WorkItemOperation()
                    {
                        path = "/fields/System.IterationPath",
                        value = workItem.details.fields.IterationPath.Replace(_devopsConfiguration.Source.ProjectName, _devopsConfiguration.Target.ProjectName)
                    }
                );

            if (parentId != 0)
            {
                operations.Add(
                        new WorkItemOperation()
                        {
                            path = "/relations/-",
                            value = new Relationship()
                            {
                                url = $"{_devopsConfiguration.Target.DevOpsOrgURL}{_devopsConfiguration.Target.ProjectId}/_apis/wit/workitems/{GetCurrentIdEquivalentId(parentId)}",
                                attributes = new RelationshipAttribute()
                            }
                        }
                    );
            }

            var jsonString = JsonConvert.SerializeObject(operations);
            HttpContent content = new StringContent(jsonString, Encoding.UTF8, "application/json-patch+json");

            var result = _importWorkItemService.Post(url, content).Result;

            TryAddIdMapper(workItem.id, result.id);
        }

        private Task CreateWorkItems(Dictionary<string, WorkItemQueryResult> workItemCollection)
        {
            foreach (var workItemCategory in workItemCollection.Keys)
            {
                var url = $"{_devopsConfiguration.Target.ProjectId}/_apis/wit/workitems/%24{workItemCategory}?api-version={_devopsConfiguration.APIVersion}";

                foreach (var workItem in workItemCollection[workItemCategory].workItems)
                {
                    CreateWorkItem(url, workItem);
                }
            }

            return Task.CompletedTask;
        }

        public async Task<Board> Import(IFormFile file)
        {
            string fileContent = null;

            using (var reader = new StreamReader(file.OpenReadStream()))
            {
                fileContent = reader.ReadToEnd();
            }
            var board = JsonConvert.DeserializeObject<Board>(fileContent);

            await CreateSprints(board.sprints);
            await CreateWorkItems(board.workItemCollection);

            return board;
        }
    }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Why do you think that you haven't used the Async-Await pattern correctly? \$\endgroup\$ Commented Sep 25, 2022 at 19:49
  • \$\begingroup\$ @PeterCsala - are you saying this code is fine? I want to improve code if I have to.. \$\endgroup\$ Commented Sep 25, 2022 at 23:59
  • 1
    \$\begingroup\$ There are a handful of places where your code could be improved. I'm planning to post a review but it takes some time. :) \$\endgroup\$ Commented Sep 26, 2022 at 5:12
  • \$\begingroup\$ After reviewing your code in depth I have to say that you haven't used the async-await pattern everywhere where you should. \$\endgroup\$ Commented Sep 26, 2022 at 15:04

1 Answer 1

3
\$\begingroup\$

Url handling

Rather than recreating some URLs over and over again you can define them only once and use them multiple times:

...

private readonly string _versionQueryString;
private readonly string _sprintCreationURL;
private readonly string _sprintPublishURL;
private const string WorkItemPathPrefix = "/fields/";

public ImportFactory(...)
{
    ...

    _versionQueryString = $"?api-version={_devopsConfiguration.APIVersion}";
    _sprintCreationURL = $"{_devopsConfiguration.Target.ProjectId}/_apis/wit/classificationNodes/Iterations{_versionQueryString}";
    _sprintPublishURL = $"{_devopsConfiguration.Target.ProjectId}/{_devopsConfiguration.Target.TargetTeamId}/_apis/work/teamsettings/iterations{_versionQueryString}";
}

public methods

It is a good practice to

  • start with your public methods after the constructors
  • continue with private/protected methods that are used inside the public methods
  • finish with the helper methods that are used inside the non public facing methods
//Ctor
public ImportFactory(...)
//Public
public async Task<Board> Import(IFormFile file)
//Private/Protected
private async Task CreateSprints(Sprints sprints)
private async Task CreateWorkItems(Dictionary<string, WorkItemQueryResult> workItems)
private async Task CreateWorkItem(string categoryURL, WorkItem workItem)
//Helpers
private int FindParentId(WorkItemDetails details)
private HttpContent GetJsonContent(object data, string mimeType = "application/json")

Import method

  • You can use the using declaration since C# 8 to simplify your code
  • You can also use the ReadToEndAsync method of the StreamReader to take advantage of non-blocking I/O
public async Task<Board> Import(IFormFile file)
{
    using var reader = new StreamReader(file.OpenReadStream());
    string fileContent = await reader.ReadToEndAsync();

    var board = JsonConvert.DeserializeObject<Board>(fileContent);

    await CreateSprints(board.sprints);
    await CreateWorkItems(board.workItemCollection);

    return board;
}

CreateSprints method

  • There is no need for the PublishSprints method, since the whole can be simplified to a single line
await _importSprintService.Post(_sprintPublishURL, GetJsonContent(new { id = result.identifier }));
  • Since the Post is an async method you should await it
  • The GetJsonContent helper method looks like this
private HttpContent GetJsonContent(object data, string mediaType = "application/json")
{
    var jsonString = JsonConvert.SerializeObject(data);
    return new StringContent(jsonString, Encoding.UTF8, mediaType);
}
  • After all of these modifications the CreateSprints looks like this
private async Task CreateSprints(Sprints sprints)
{
    foreach (Sprint sprint in sprints.value)
    {
        var result = await _importWorkItemService.Post(_sprintCreationURL, GetJsonContent(sprint));
        await _importSprintService.Post(_sprintPublishURL, GetJsonContent(new { id = result.identifier }));
    }
}
  • I'm not sure whether this _importWorkItemService.Post(_sprintCreationURL, GetJsonContent(sprint) should be called on _importWorkItemService or on _importSprintService
    • Without knowing the actual implementation I can't tell but it is likely a bug in your code

CreateWorkItems method

  • Since CreateWorkItem should be an async method (see next section) that's why this should be async as well
  • I've renamed your workItemCollection to workItems since you had a sprints parameter in CreateSprints
private async Task CreateWorkItems(Dictionary<string, WorkItemQueryResult> workItems)
{
    foreach (var workItemCategory in workItems.Keys)
    {
        var categoryURL = $"{_devopsConfiguration.Target.ProjectId}/_apis/wit/workitems/%24{workItemCategory}{_versionQueryString}";

        foreach (var workItem in workItems[workItemCategory].workItems)
        {
            await CreateWorkItem(categoryURL, workItem);
        }
    }
}

CreateWorkItem method

  • The GetCurrentIdEquivalentId and TryAddIdMapper can be inlined since they are not complex and they are used only inside the CreateWorkItem method once
  • You can use the collection initializer to populate your operations List
var operations = new List<WorkItemOperation>
{
    new WorkItemOperation()
    {
        path = $"{WorkItemPathPrefix}System.Title",
        value = workItem.details.fields.Title ?? ""
    },
    new WorkItemOperation()
    {
        path = $"{WorkItemPathPrefix}System.Description",
        value = workItem.details.fields.Description ?? ""
    },
    new WorkItemOperation()
    {
        path = $"{WorkItemPathPrefix}Microsoft.VSTS.Common.AcceptanceCriteria",
        value = workItem.details.fields.AcceptanceCriteria ?? ""
    },
    new WorkItemOperation()
    {
        path = $"{WorkItemPathPrefix}System.IterationPath",
        value = workItem.details.fields.IterationPath.Replace(_devopsConfiguration.Source.ProjectName, _devopsConfiguration.Target.ProjectName)
    }
};
  • Please prefer await over .Result
await _importWorkItemService.Post(categoryURL, GetJsonContent(operations, "application/json-patch+json"));

FindParentId method

  • This can be simplified by using the ?. and ?: operators
private int FindParentId(WorkItemDetails details)
{
    var parentRelation = details.relations?.Where(relation => relation.attributes.name.Equals("Parent")).FirstOrDefault();
    return parentRelation == null ? 0 : int.Parse(parentRelation.url.Split("/")[parentRelation.url.Split("/").Length - 1]);
}
  • Please note that the int.Parse is error-prone, please prefer int.TryParse instead

For the sake of completeness here is the full code after refactoring

class ImportFactory : IImportFactory
{
    private ConcurrentDictionary<int, int> idMapper = new ConcurrentDictionary<int, int>();
    private readonly ILogger<ImportFactory> _logger;
    private readonly Devops _devopsConfiguration;
    private readonly IImportService<WorkItemCore> _importWorkItemService;
    private readonly IImportService<SprintCore> _importSprintService;

    private readonly string _versionQueryString;
    private readonly string _sprintCreationURL;
    private readonly string _sprintPublishURL;
    private readonly string _projectId;
    private const string WorkItemPathPrefix = "/fields/";

    public ImportFactory(ILogger<ImportFactory> logger,
        IConfiguration configuration,
        IImportService<SprintCore> importSprintService,
        IImportService<WorkItemCore> importWorkItemService)
    {
        _logger = logger;
        _devopsConfiguration = configuration.GetSection(nameof(Devops)).Get<Devops>();
        _importSprintService = importSprintService;
        _importWorkItemService = importWorkItemService;

        _versionQueryString = $"?api-version={_devopsConfiguration.APIVersion}";
        _projectId = _devopsConfiguration.Target.ProjectId;
        _sprintCreationURL = $"{_projectId}/_apis/wit/classificationNodes/Iterations{_versionQueryString}";
        _sprintPublishURL = $"{_projectId}/{_devopsConfiguration.Target.TargetTeamId}/_apis/work/teamsettings/iterations{_versionQueryString}";
    }

    public async Task<Board> Import(IFormFile file)
    {
        using var reader = new StreamReader(file.OpenReadStream());
        string fileContent = await reader.ReadToEndAsync();

        var board = JsonConvert.DeserializeObject<Board>(fileContent);

        await CreateSprints(board.sprints);
        await CreateWorkItems(board.workItemCollection);

        return board;
    }

    private async Task CreateSprints(Sprints sprints)
    {
        foreach (Sprint sprint in sprints.value)
        {
            var result = await _importWorkItemService.Post(_sprintCreationURL, GetJsonContent(sprint));
            await _importSprintService.Post(_sprintPublishURL, GetJsonContent(new { id = result.identifier }));
        }
    }

    private async Task CreateWorkItems(Dictionary<string, WorkItemQueryResult> workItems)
    {
        foreach (var workItemCategory in workItems.Keys)
        {
            var categoryURL = $"{_projectId}/_apis/wit/workitems/%24{workItemCategory}{_versionQueryString}";

            foreach (var workItem in workItems[workItemCategory].workItems)
            {
                await CreateWorkItem(categoryURL, workItem);
            }
        }
    }

    private async Task CreateWorkItem(string categoryURL, WorkItem workItem)
    {
        var operations = new List<WorkItemOperation>
        {
            new WorkItemOperation()
            {
                path = $"{WorkItemPathPrefix}System.Title",
                value = workItem.details.fields.Title ?? ""
            },
            new WorkItemOperation()
            {
                path = $"{WorkItemPathPrefix}System.Description",
                value = workItem.details.fields.Description ?? ""
            },
            new WorkItemOperation()
            {
                path = $"{WorkItemPathPrefix}Microsoft.VSTS.Common.AcceptanceCriteria",
                value = workItem.details.fields.AcceptanceCriteria ?? ""
            },
            new WorkItemOperation()
            {
                path = $"{WorkItemPathPrefix}System.IterationPath",
                value = workItem.details.fields.IterationPath.Replace(_devopsConfiguration.Source.ProjectName, _devopsConfiguration.Target.ProjectName)
            }
        };

        var parentId = FindParentId(workItem.details);
        if (parentId != 0)
        {
            operations.Add(new WorkItemOperation()
            {
                path = "/relations/-",
                value = new Relationship()
                {
                    url = $"{_devopsConfiguration.Target.DevOpsOrgURL}{_projectId}/_apis/wit/workitems/{idMapper[parentId]}",
                    attributes = new RelationshipAttribute()
                }
            });
        }

        var result = await _importWorkItemService.Post(categoryURL, GetJsonContent(operations, "application/json-patch+json"));

        if (!idMapper.ContainsKey(workItem.id))
        {
            idMapper.TryAdd(workItem.id, result.id);
        }
    }

    private int FindParentId(WorkItemDetails details)
    {
        var parentRelation = details.relations?.Where(relation => relation.attributes.name.Equals("Parent")).FirstOrDefault();
        return parentRelation == null ? 0 : int.Parse(parentRelation.url.Split("/")[parentRelation.url.Split("/").Length - 1]);
    }

    private HttpContent GetJsonContent(object data, string mediaType = "application/json")
    {
        var jsonString = JsonConvert.SerializeObject(data);
        return new StringContent(jsonString, Encoding.UTF8, mediaType);
    }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thanks for taking the time to explain so thoroughly. \$\endgroup\$ Commented Sep 26, 2022 at 18:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.