2
\$\begingroup\$

I have a switch statement that works just fine, however, it has a lot of duplicate logic within it. I have an enum that my controllers use to say which is making the publish request, so ProductInfo controller passes in ContentFields.ProductInformation into PublishTable

public static void PublishTable(string user, ContentFields tableToPublish)
        {
            var jsonData = DataAccess.GetEnvironmentJson((int)Environments.Production);
            var model = JsonConvert.DeserializeObject<Models.FullDataSetModel>(jsonData);
            using (var db = new LoginPageContentEntities())
            {
                var deployEntry = db.Deployment.Find((int)Environments.Production);
                deployEntry.DateUpdated = DateTime.Now;
                switch (tableToPublish)
                {
                    case ContentFields.ProductInformation:
                        deployEntry.ProductInformation = false;
                        model.ProductInformation.Clear();
                        model.ProductInformation = db.ProductInformation.ToList();
                        break;
                    case ContentFields.DidYouKnow:
                        deployEntry.DidYouKnow = false;
                        model.DidYouKnow.Clear();
                        model.DidYouKnow = db.DidYouKnow.ToList();
                        break;
                    case ContentFields.MaintenanceMessage:
                        deployEntry.MaintenanceMessage = false;
                        model.MaintenanceMessage.Clear();
                        model.MaintenanceMessage = db.MaintenanceMessage.ToList();
                        break;
                    case ContentFields.TrainingEvent:
                        deployEntry.TrainingEvent = false;
                        model.TrainingEvents.Clear();
                        model.TrainingEvents = db.TrainingEvents.ToList();
                        break;
                    case ContentFields.VideoContent:
                        deployEntry.VideoContent = false;
                        model.HtmlSnippets.Clear();
                        model.HtmlSnippets = db.HtmlSnippets.ToList();
                        break;
                    case ContentFields.ProductMarketingUrl:
                        deployEntry.ProductMarketingUrl = false;
                        model.ProductMarketingUrl.Clear();
                        model.ProductMarketingUrl = db.ProductMarketingUrl.ToList();
                        break;
                }
                var json = JsonConvert.SerializeObject(model);
                deployEntry.JsonCache = json;
                db.SaveChanges();
            }
        }

Model:

public class FullDataSetModel
    {
        public List<DidYouKnow> DidYouKnow                   { get; set; }
        public List<HtmlSnippets> HtmlSnippets               { get; set; }
        public List<MaintenanceMessage> MaintenanceMessage   { get; set; }
        public List<ProductInformation> ProductInformation   { get; set; }
        public List<TrainingEvents> TrainingEvents           { get; set; }
        public List<ProductMarketingUrl> ProductMarketingUrl { get; set; }
    }

My question is how can i remove the need for the switch here while being able to specify which part of the model to update. Or, whether that is even needed and a switch is ok here even though there is a lot of duplicate logic. More or less this comes down to design, I feel this is pretty human readable to understand what is happening but it may not be the most efficient way to handle this code.

As an aside if this was to be a database context rather than a model such as:

public static void RevertChanges(Environments targetToRestoreFrom, ContentFields tableToRevert)
        {
            var jsonData = DataAccess.GetEnvironmentJson((int)targetToRestoreFrom);
            var model = JsonConvert.DeserializeObject<Models.FullDataSetModel>(jsonData);            
            using (var db = new LoginPageContentEntities())
            {
                switch (tableToRevert)
                {
                    case ContentFields.ProductInformation:
                        db.Database.ExecuteSqlCommand("TRUNCATE TABLE [ProductInformation]");
                        db.ProductInformation.AddRange(model.ProductInformation);
                        break;
                    case ContentFields.DidYouKnow:
                        db.Database.ExecuteSqlCommand("TRUNCATE TABLE [DidYouKnow]");
                        db.DidYouKnow.AddRange(model.DidYouKnow);
                        break;
                    case ContentFields.MaintenanceMessage:
                        db.Database.ExecuteSqlCommand("TRUNCATE TABLE [MaintenanceMessage]");
                        db.MaintenanceMessage.AddRange(model.MaintenanceMessage);
                        break;
                    case ContentFields.TrainingEvent:
                        db.Database.ExecuteSqlCommand("TRUNCATE TABLE [TrainingEvents]");
                        db.TrainingEvents.AddRange(model.TrainingEvents);
                        break;
                    case ContentFields.VideoContent:
                        db.Database.ExecuteSqlCommand("TRUNCATE TABLE [HtmlSnippets]");
                        db.HtmlSnippets.AddRange(model.HtmlSnippets);
                        break;
                    case ContentFields.ProductMarketingUrl:
                        db.Database.ExecuteSqlCommand("TRUNCATE TABLE [ProductMarketingUrl]");
                        db.ProductMarketingUrl.AddRange(model.ProductMarketingUrl);
                        break;
                }                
                db.SaveChanges();
            }
        }

How could one remove the switch here and still specify which entity the context should be referencing?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Since this question was cross-posted from Stack Overflow, I recommend that you add the link there and here. I went ahead and fetched the link: stackoverflow.com/questions/32012909/… \$\endgroup\$ – Ismael Miguel Aug 14 '15 at 15:11
  • \$\begingroup\$ Ah great, I assumed my question would be closed there as off topic so I didn't bother getting it (as the link would be dead). \$\endgroup\$ – Zach M. Aug 14 '15 at 15:12
  • \$\begingroup\$ It would be closed, but not deleted. Everybody can still see it. Also, the link there, would avoid people from spamming you to move the question to Code Review, even if it already was (like in your case) \$\endgroup\$ – Ismael Miguel Aug 14 '15 at 15:13
  • \$\begingroup\$ Can you use reflection? It may be a bit slower, but more readable and maintainable. Other option would be to have array of object lists and do some casting, but have universal access to internal array holding data for the properties. I would use reflection. \$\endgroup\$ – firda Aug 14 '15 at 15:20
  • \$\begingroup\$ I will look into that, I am not very familiar with reflection. \$\endgroup\$ – Zach M. Aug 14 '15 at 15:21
1
\$\begingroup\$

There is one big thing that makes your code hard to refactor to something prettier namely the ContentFields enum. It contains two values that are incompatible with the model:

  • ContentFields.TrainingEvent vs. model.TrainingEvents
  • ContentFields.VideoContent vs. model.HtmlSnippets

This makes it practically impossible to build the SqlCommand dynamicly or to get the properties by reflection.

If you cannot rename the values you might consider adding custom attributes and get the right name from there (refer to Getting attributes of Enum's value and Creating Custom Attributes:

ContentField
{
    [ModelPropertyName("TrainingEvents")]
    TrainingEvent
}

The last thing is the name of the ContentFields enum. If it's not marked with the [Flags] attribute (you're not using it like that) then it shouldn't be in plural.

\$\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.