I am looking for tips to improve the readability of my code (which is written in C#). My main concern is usage of consecutive if statements and how should I replace them.

My code:

private Dictionary<Guid, CommerceMediaFileAssociation<T>> GetMediaToContentsAssociations<T>(ref bool stopSignaled, CatalogContentBase catalog, CultureInfo culture, Action<string> onStatusChanged = null)
    where T : MediaData
{
    IEnumerable<ContentReference> descendentReferences = _contentLoader.GetDescendents(catalog.ContentLink);
    var associations = new Dictionary<Guid, CommerceMediaFileAssociation<T>>();

    if (descendentReferences.Any())
    {
        var descendentProducts = _contentLoader.GetItems<BaseProduct>(descendentReferences, culture);

        foreach (var product in descendentProducts)
        {
            if (stopSignaled)
            {
                onStatusChanged?.Invoke($"Reindexing canceled.");
                break;
            }

            if (product is IAssetContainer assetContainer && (assetContainer?.CommerceMediaCollection?.Any() ?? false))
            {
                foreach (CommerceMedia media in assetContainer.CommerceMediaCollection)
                {
                    PermanentLinkMap mediaLinkMap = _permanentLinkMapper.Find(media.AssetLink);

                    if ((mediaLinkMap?.Guid != null) && mediaLinkMap.Guid != Guid.Empty)
                    {
                        var productInformation = ProductUtilities.GetProductCategoriesAndPriority(product);

                        if (associations.TryGetValue(mediaLinkMap.Guid, out CommerceMediaFileAssociation<T> commerceMediaFileAssociations))
                        {
                            commerceMediaFileAssociations.Products.Add($"{product.ContentGuid.ToString()}||{product.MetaTitle}");

                            if (productInformation.Categories?.Any() ?? false)
                            {
                                foreach (string category in productInformation.Categories)
                                {
                                    commerceMediaFileAssociations.ProductCategories.Add(category);
                                }
                            }

                            associations[mediaLinkMap.Guid] = commerceMediaFileAssociations;
                        }
                        else
                        {
                            var commerceMediaFileAssociation = new CommerceMediaFileAssociation<T>();

                            commerceMediaFileAssociation.Products.Add($"{product.ContentGuid.ToString()}||{product.MetaTitle}");

                            foreach (string category in productInformation.Categories)
                            {
                                commerceMediaFileAssociation.ProductCategories.Add(category);
                            }

                            associations.Add(mediaLinkMap.Guid, commerceMediaFileAssociation);
                        }

                        associations[mediaLinkMap.Guid].Priority = productInformation.Priority;
                    }
                }
            }

            if (product is RockstarProduct rockstar)
            {
                var files = rockstar.Rockstar_Product_Product_Documents.FilteredItems.Select(x => x.GetContent() as IContentMedia) ?? new List<IContentMedia>();
                foreach (var file in files)
                {
                    PermanentLinkMap mediaLinkMap = _permanentLinkMapper.Find(file.ContentLink);
                    if ((mediaLinkMap?.Guid != null) && mediaLinkMap.Guid != Guid.Empty)
                    {
                        var productInformation = ProductUtilities.GetProductCategoriesAndPriority(product);

                        if (associations.TryGetValue(mediaLinkMap.Guid, out CommerceMediaFileAssociation<T> commerceMediaFileAssociations))
                        {
                            commerceMediaFileAssociations.Products.Add($"{product.ContentGuid.ToString()}||{product.MetaTitle}");
                            if (productInformation.Categories?.Any() ?? false)
                            {
                                foreach (string category in productInformation.Categories)
                                {
                                    commerceMediaFileAssociations.ProductCategories.Add(category);
                                }
                            }

                            associations[mediaLinkMap.Guid] = commerceMediaFileAssociations;
                        }
                        else
                        {
                            var commerceMediaFileAssociation = new CommerceMediaFileAssociation<T>();

                            commerceMediaFileAssociation.Products.Add($"{product.ContentGuid.ToString()}||{product.MetaTitle}");

                            foreach (string category in productInformation.Categories)
                            {
                                commerceMediaFileAssociation.ProductCategories.Add(category);
                            }

                            associations.Add(mediaLinkMap.Guid, commerceMediaFileAssociation);
                        }

                        associations[mediaLinkMap.Guid].Priority = productInformation.Priority;
                    }
                }
            }
        }
    }

    return associations;
}

How should I change it in order to make it clean, understandable and maintainable? Should I use guard clauses in this case?

CodePudding user response：

You have a lot of code duplications. Some code parts are almost equivalent. It is easy to make them equal. This allows you extract them to another method.

Also, you have if-else statements where the else-part has a lot in common with the if-part where basically only the first statement differs (after having made the almost equal statements equal). You can extract these statements and execute them after the if-else. Then if-part becomes empty. You can invert the if-else and then drop the else part.

There is also no point in testing a collection with Any() before looping through it. If the collection is empty, the loop body will not be executed anyway.

The extracted method:

private void AddCategoriesAndProducts<T>(Dictionary<Guid, CommerceMediaFileAssociation<T>> associations, BaseProduct product, string link) where T : MediaData
{
    PermanentLinkMap mediaLinkMap = _permanentLinkMapper.Find(link);

    if (mediaLinkMap?.Guid != null && mediaLinkMap.Guid != Guid.Empty) {
        var productInformation = ProductUtilities.GetProductCategoriesAndPriority(product);

        if (!associations.TryGetValue(mediaLinkMap.Guid, out CommerceMediaFileAssociation<T> commerceMediaFileAssociations)) {
            commerceMediaFileAssociations = new CommerceMediaFileAssociation<T>();
        }
        commerceMediaFileAssociations.Products.Add($"{product.ContentGuid}||{product.MetaTitle}");
        if (productInformation.Categories != null) {
            foreach (string category in productInformation.Categories) {
                commerceMediaFileAssociations.ProductCategories.Add(category);
            }
        }
        associations[mediaLinkMap.Guid] = commerceMediaFileAssociations;
        associations[mediaLinkMap.Guid].Priority = productInformation.Priority;
    }
}

The simplified method:

private Dictionary<Guid, CommerceMediaFileAssociation<T>> GetMediaToContentsAssociations<T>(ref bool stopSignaled, CatalogContentBase catalog, CultureInfo culture, Action<string> onStatusChanged = null)
    where T : MediaData
{
    IEnumerable<ContentReference> descendentReferences = _contentLoader.GetDescendents(catalog.ContentLink);
    var associations = new Dictionary<Guid, CommerceMediaFileAssociation<T>>();

    if (descendentReferences.Any()) {
        var descendentProducts = _contentLoader.GetItems<BaseProduct>(descendentReferences, culture);

        foreach (var product in descendentProducts) {
            if (stopSignaled) {
                onStatusChanged?.Invoke($"Reindexing canceled.");
                break;
            }

            if (product is IAssetContainer assetContainer && assetContainer.CommerceMediaCollection != null) {
                foreach (CommerceMedia media in assetContainer.CommerceMediaCollection) {
                    AddCategoriesAndProducts(associations, product, media.AssetLink);
                }
            }

            if (product is RockstarProduct rockstar) {
                var files = rockstar.Rockstar_Product_Product_Documents.FilteredItems.Select(x => x.GetContent() as IContentMedia) ?? new List<IContentMedia>();
                foreach (var file in files) {
                    AddCategoriesAndProducts(associations, product, file.ContentLink);
                }
            }
        }
    }

    return associations;
}

These methods are still relatively complex and could be split into even smaller methods. Methods like AddAssetProducts and AddRockStartProducts.