2
\$\begingroup\$

I have the following method that will return VariantID for given VariantName ("Name" in db).
To avoid calling my db few thousand times I have dumped all records (~30) to the simple dictionary.

I'm almost certain that this is the correct way of implementing this method but I would like to hear some feedback and any possible tips.

TestManagementEntities is an EntityFramework DbContext

Calling Method

private void UpdateVariants(WorkItem workItem, string variantValue, TestManagementEntities db)
    {           

        string[] variants = variantValue.Split(',').Where(x => !String.IsNullOrEmpty(x)).ToArray();

        int[] variantIds = new int[variants.Count()];

        Dictionary<string, int> variantDictionary =
            db.Variants.ToDictionary(x => x.Name, y => y.VariantID);
        Object lockObject = new Object();

        System.Threading.Tasks.Parallel.For(0, variants.Count(), i =>
        {
            variants[i] = variants[i].Trim();
            //Method in question call
            variantIds[i] = GetVariantId(variants[i], variantDictionary, db, lockObject);
        });

        //Some not relevant to question code here
    }

private int GetVariantId(string variantName, Dictionary<string, int> variantDictionary, TestManagementEntities db, Object lockObject)
    {
        if (variantDictionary.ContainsKey(variantName))
        {
            return variantDictionary[variantName];
        }
        else
        {
            lock (lockObject)
            {
                if (variantDictionary.ContainsKey(variantName))
                {
                    return variantDictionary[variantName];
                }
                else
                {
                    Variant newVariant = new Variant();
                    newVariant.Name = variantName;
                    db.Variants.Add(newVariant);
                    db.SaveChanges();
                    variantDictionary.Add(variantName, newVariant.VariantID);
                    return newVariant.VariantID;
                }
            }
        }
    }
\$\endgroup\$
4
\$\begingroup\$

Why is your parameter variantValue a string? Why not an IEnumerable<string> or an ICollection<string> or something like that?


Why is variants an array? If it had been an ICollection<string> it would have had a Count property that was far less costly than .Count(). Don't use the (extension) method .Count(), use the Length property. Also: why is it called variants when it really contains names?


Why do you do this inside that loop: variants[i] = variants[i].Trim();? Why not do this when you do variantValue.Split(',').Where(x => !String.IsNullOrEmpty(x)).ToArray();


The whole logic seems really contrived to me: storing the contents of a table in a dictionary, just to check whether you need to add a value to the DB and to retrieve the IDs? Why not make it much more simple:

  • Query the DB for the entries where the name corresponds with the ones you've received as a parameter (a WHERE IN query).
  • Remove the results from the list of names.
  • Loop through the filtered names, create a Variant for each and only execute db.SaveChanges(); after you've added ALL items.
  • After you've executed db.SaveChanges(); all those newly created variants will have an ID which you can retrieve.

By the way, don't do ContainsKey if you need the value associated with said key, instead use TryGetValue.


I suppose that you need variantIds for that //Some not relevant to question code here, and that bothers me a bit. GetVariantId for instance does far more than the method name says it does, and thus I suspect UpdateVariants contains more logic that really shouldn't be there according to the Single Responsibility Principle.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ @RobH D'oh, confused it with IEnumerable<T>. Fixed it. \$\endgroup\$ – BCdotWEB Jul 8 '15 at 12:19

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.