2
\$\begingroup\$

I have a switch statement that references different configurations based on data pulled from a database. Each iteration of configuration has variations in what needs to be enabled/modified.

I've set up a method that takes the varying arguments for each configuration and does relevant logic, generally changing elements in a control. I've setup 4 overloaded methods varying in their control type.

        SchemeConfigurationItems scis = sm.GetSchemeConfiguration(sid, TriState.True);
        foreach (SchemeConfigurationItem sci in scis) {
            // One of solution, other configuration items should be single items rather than grouping such as this.
            switch (sci.ConfigurationTypeID)
            {
                case (int)ConfigurationType.GPDetails:
                     bool mandatory = sci.Mandatory;
                     pnlGP.Visible = true;

                     rfvGPName.Enabled = mandatory;
                     rfvGPPractice.Enabled = mandatory;
                     rfvGPPracticeAddress.Enabled = mandatory;
                     rfvGPPracticePostcode.Enabled = mandatory;

                     tbGPName.CssClass = MandatoryHandler(tbGPName.CssClass, mandatory);
                     tbGPPractice.CssClass = MandatoryHandler(tbGPPractice.CssClass, mandatory);
                     tbGPPracticeAddress.CssClass = MandatoryHandler(tbGPPracticeAddress.CssClass, mandatory);
                     tbGPPracticePostcode.CssClass = MandatoryHandler(tbGPPracticePostcode.CssClass, mandatory);
                     break;

                case (int)ConfigurationType.ReferralOwner:
                     configHandler(sci, ddlOwners, isReferralOwnerConfig, rfvOwners);
                     break;
                case (int)ConfigurationType.RequiresAssessmentsPassed:
                     break;
                case (int)ConfigurationType.ReferralDateReceived:
                     configHandler(sci, tbReferralReceivedDate, isReferralDateReceivedConfig, rfvReferralReceivedDate ,pnlReferralReceivedDate);
                     break;
                case (int)ConfigurationType.ReasonForReferral:
                     configHandler(sci, tbReason, isReasonForReferralConfig, rfvReason, pnlReason);
                     break;
                case (int)ConfigurationType.HealthCheck:
                     configHandler(sci, rblHealthCheck, isHealthCheckConfig, rfvHealthCheck, pnlHealthCheck);
                     break;
                case (int)ConfigurationType.PersonalSafety:
                     configHandler(sci, rblPersonalSafety, isPersonalSafetyConfig, rfvrblPersonalSafety, pnlPersonalSafety);
                     break;
                case (int)ConfigurationType.PersonalSafetyDetail:
                     configHandler(sci, tbPersonalSafety, isPersonalSafetyDetailConfig, rfvtbPersonalSafety, pnlPersonalSafetyDetail);
                     break;
                case (int)ConfigurationType.ServiceAwarenessMethod:
                     configHandler(sci, ddlServiceAwarenessMethod, isServiceAwarenessMethodConfig, pnl: pnlServiceAwarenessMethod);
                     break;
                case (int)ConfigurationType.ActivitySelection:
                     configHandler(sci, cbActivitySelection, isActivitySelectionConfig, pnl: pnlActivitySelection);
                     break;
                case (int)ConfigurationType.StatLog:
                     configHandler(sci: sci, pnl: pnlStatLog, def: "");
                     break;
            }
        }

This is a section of the switch statement, there's going to be around 50 more so this is how the general flow goes.

    public void configHandler(SchemeConfigurationItem sci = null, DropDownList ddl = null, bool Config = true, RequiredFieldValidator rfv = null, Panel pnl = null, string def = "")
    {
        if (pnl != null)
        {
            pnl.Visible = Convert.ToBoolean(sci.Active);
        }
        if (Convert.ToBoolean(sci.Active))
        {
            Config = true;

            if (ddl != null)
            {
                ddl.CssClass = sci.CssClass != String.Empty ? sci.CssClass : ddl.CssClass;
                ddl.ToolTip = sci.ToolTip != String.Empty ? sci.ToolTip : ddl.ToolTip;
                ddl.CssClass = MandatoryHandler(ddl.CssClass, rfv.Enabled);
            }

            if (rfv != null)
            {
                rfv.Enabled = Convert.ToBoolean(sci.Mandatory);
                rfv.ErrorMessage = sci.ErrorMessage != String.Empty ? sci.ErrorMessage : rfv.ErrorMessage;
            }
        }
    }

    public void configHandler(SchemeConfigurationItem sci = null, TextBox tb = null, bool Config = true, RequiredFieldValidator rfv = null, Panel pnl = null)
    {
        if (pnl != null)
        {
            pnl.Visible = Convert.ToBoolean(sci.Active);
        }
        if (Convert.ToBoolean(sci.Active))
        {
            Config = true;

            if (tb != null)
            {
                tb.CssClass = sci.CssClass != String.Empty ? sci.CssClass : tb.CssClass;
                tb.ToolTip = sci.ToolTip != String.Empty ? sci.ToolTip : tb.ToolTip;
                tb.CssClass = MandatoryHandler(tb.CssClass, rfv.Enabled);
            }

            if (rfv != null)
            {
                rfv.Enabled = Convert.ToBoolean(sci.Mandatory);
                rfv.ErrorMessage = sci.ErrorMessage != String.Empty ? sci.ErrorMessage : rfv.ErrorMessage;
            }
        }
    }

    public void configHandler(SchemeConfigurationItem sci = null, RadioButtonList rbl = null, bool Config = true, RequiredFieldValidator rfv = null, Panel pnl = null)
    {
        if (pnl != null)
        {
            pnl.Visible = Convert.ToBoolean(sci.Active);
        }
        if (Convert.ToBoolean(sci.Active))
        {
            Config = true;

            if (rbl != null)
            {
                rbl.CssClass = sci.CssClass != String.Empty ? sci.CssClass : rbl.CssClass;
                rbl.ToolTip = sci.ToolTip != String.Empty ? sci.ToolTip : rbl.ToolTip;
                rbl.CssClass = MandatoryHandler(rbl.CssClass, rfv.Enabled);
            }

            if (rfv != null)
            {
                rfv.Enabled = Convert.ToBoolean(sci.Mandatory);
                rfv.ErrorMessage = sci.ErrorMessage != String.Empty ? sci.ErrorMessage : rfv.ErrorMessage;
            }
        }
    }

    public void configHandler(SchemeConfigurationItem sci = null, CheckBoxList cbl = null, bool Config = true, RequiredFieldValidator rfv = null, Panel pnl = null)
    {
        if (pnl != null)
        {
            pnl.Visible = Convert.ToBoolean(sci.Active);
        }
        if (Convert.ToBoolean(sci.Active))
        {
            Config = true;

            if (cbl != null)
            {
                cbl.CssClass = sci.CssClass != String.Empty ? sci.CssClass : cbl.CssClass;
                cbl.ToolTip = sci.ToolTip != String.Empty ? sci.ToolTip : cbl.ToolTip;
                cbl.CssClass = MandatoryHandler(cbl.CssClass, rfv.Enabled);
            }

            if (rfv != null)
            {
                rfv.Enabled = Convert.ToBoolean(sci.Mandatory);
                rfv.ErrorMessage = sci.ErrorMessage != String.Empty ? sci.ErrorMessage : rfv.ErrorMessage;
            }
        }
    }

Here are the overloaded methods, as you can see all of the parameters are optional to allow for varied config. Some of you may notice the string def = "" in the first method, I implemented this to target a default method, if the key parameter wasn't included, in this example the Control type.

I'm looking for ways to make the solution neater, cleaner and smaller.

\$\endgroup\$
  • 1
    \$\begingroup\$ Can you tell us more about your configHandler? \$\endgroup\$ – Mast Sep 27 '16 at 13:45
  • \$\begingroup\$ @Mast The configHandler takes the optional parameters and applies values passed in the sci to the targeted Control and Validator, all of these are optional. \$\endgroup\$ – S.Wessels Sep 27 '16 at 13:59
3
\$\begingroup\$

It looks like you've fallen pray to the Copy+Paste monster.

There will be a point where you'll "need" an overload of configHandler that the compiler won't be able to resolve; instead of adding bogus parameters and change the order of the parameters (i.e. making the API a nightmare), you need to step back and think of another way.

But first things first. Close your eyes, empty your mind. Breathe.

Still reading? I said close your eyes!

Okay. Now that your mind is completely cleansed, what does "config handler" evoke?

  • CSS classes
  • UI controls
  • ~> Configuration settings <~
  • Tooltips

Did you say configuration settings? There's absolutely no subliminal mind-trickery here: "config handler" invariably rings "configuration settings" in the reader's mind - and this will be you in 6 months, or someone else inheriting the code base to maintain it. But "configuration settings" in any .net dev's mind, points to web.config, not CSS styles.

A "handler" is usually what we call a method that handles an event; it has a signature that matches that of some EventHandler<TEventArgs> delegate. Using the term "handler" for something that's not a "handler" is also confusing.

So "config handler" isn't about config, and isn't about handling either.

You need a wildly different approach: you simply can't keep going like this (50 overloads? no thanks!). Assuming your controls are all inherited from the System.Web.UI.WebControls.WebControl class, I'd start by combining all overloads and leveraging the controls' inheritance hierarchy:

private void SetControlProperties(SchemeConfigurationItem item, WebControl control, RequiredFieldValidator validator, WebControl panel = null)
{
    if (control == null) { return; }
    if (validator == null) { throw new ArgumentNullException("validator"); } // little assumption here...

    if (panel != null)
    {
        panel.Visible = item.IsActive;
    }

    validator.IsEnabled = item.IsMandatory; // why isn't that a bool in the first place?

    validator.ErrorMessage = string.IsNullOrEmpty(item.ErrorMessage)
        ? validator.ErrorMessage
        : item.ErrorMessage;

    var css = string.IsNullOrEmpty(item.CssClass)
        ? control.CssClass
        : item.CssClass;

    control.CssClass = ApplyMandatoryFieldStyle(css, validator.IsEnabled); // method could very likely be inlined here

    control.ToolTip = string.IsNullOrEmpty(item.ToolTip)
        ? control.ToolTip
        : item.ToolTip;
}

Note, your methods can throw a possible NullReferenceException if rfv is null, because you're reading its Enabled property and then checking if it's null or not. You're also setting its Enabled property after you've read it, which makes your workflow extremely confusing - it's not clear what happens to the ErrorMessage either, and the Config parameter seems utterly useless; it's assigned a value, and then that value gets lost in limbo.

Another thing - why cast every single case to int, if you can work with the enum type and cast the ConfigurationtypeID you're switching on (once!) instead? That would reduce visual clutter a bit:

switch ((ConfigurationType)sci.ConfigurationTypeID)
{
    case ConfigurationType.ReferralOwner:
        SetControlProperties(sci, ddlOwners, rfvOwners);
        break;
    case ConfigurationType.ReferralDateReceived:
        SetControlProperties(sci, tbReferralReceivedDate, rfvReferralReceivedDate, pnlReferralReceivedDate);
        break;
    case ConfigurationType.ReasonForReferral:
        SetControlProperties(sci, tbReason, rfvReason, pnlReason);
        break;
    case ConfigurationType.HealthCheck:
        SetControlProperties(sci, rblHealthCheck, rfvHealthCheck, pnlHealthCheck);
        break;
    //...
}

Now, that's a little better... but I'd warmly recommend you consider implementing validation using the tools the framework is giving you; validation is a solved problem, no need to implement your own framework for it - that switch block smells like you're working at a much lower level of abstraction than that of the tools the framework is putting in your hands. You probably don't need that switch block at all.

\$\endgroup\$
  • \$\begingroup\$ Great answer, I'll be marking this as the accepted choice due to its simplicity, and neat style. I've taken on board the suggestions you've made and it looks great! With regards to the switch block, it's not for applying validation, we have a large page that varies based on data extracted from a database, each client essentially has their own configuration and these individual items are a list of possible variations that need to be toggled on a per client basis. The variations in validation are simply to allow us to implement custom values without changing the hard code. \$\endgroup\$ – S.Wessels Sep 28 '16 at 8:49
3
\$\begingroup\$
  • The SchemeConfigurationItem sci = null parameter doesn't need to be optional because you are always passing a parameter which can't be null.

  • Based on the naming guidelines for .NET methods should be named using PascalCase casing.

  • You have the following in each of the overloaded methods

    if (rfv != null)
    {
        rfv.Enabled = Convert.ToBoolean(sci.Mandatory);
        rfv.ErrorMessage = sci.ErrorMessage != String.Empty ? sci.ErrorMessage : rfv.ErrorMessage;
    }  
    

    This should be extracted to a separate method to remove code duplication. It is easier to change something in one place instead of changing it in multiple places and maybe forget one.

  • You should really work on the naming of things. Its a hard task to find good and descriptive names but it will save a lot of time afterwards if you or Sam the maintainer will have to fix a bug. Names like sci or pnlGP won't tell you what it is about.

  • If you invert the condition

    if (Convert.ToBoolean(sci.Active))  
    

    you can return early and save one level of indentation which makes the code easier to read like so

    if (!Convert.ToBoolean(sci.Active))  
    {
        return;
    }
    
\$\endgroup\$
  • \$\begingroup\$ Why does indentation make it harder to read? I agree with your point, I actually prefer your example, I'm just curious as to why? \$\endgroup\$ – S.Wessels Sep 27 '16 at 14:54
  • \$\begingroup\$ @S.Wessels the more visual noise, the heavier the cognitive load. It reduces the signal-to-noise ratio. \$\endgroup\$ – Mathieu Guindon Sep 27 '16 at 14:55
  • \$\begingroup\$ What @Mat'sMug said and because you need to scroll if the indentation is going to be very deep. Its just a pointer to use early returns if possible. \$\endgroup\$ – Heslacher Sep 27 '16 at 14:56
  • \$\begingroup\$ That makes sense. Do you have any reference to more I can read up on? or is this personal experience? \$\endgroup\$ – S.Wessels Sep 27 '16 at 14:58
  • 1
    \$\begingroup\$ One way would be to read just the c# tagged questions here ;-) \$\endgroup\$ – Heslacher Sep 27 '16 at 15:01
2
\$\begingroup\$

All your configHandler methods differ only by one parameter and they are all amost identical. You also use the same properties for each control (second parameter).

You should create an interface for it

public interface IStyleable
{
    string CssClass { get; set; }
    string ToolTip { get; set; }
}

that you will pass to a single configHandler via a ConfigProperties class holding all parameters:

public void configHandler(ConfigParameters configParams = null)
{
    configParams = configParams ?? new ConfigParameters();

    if (configParams.Panel != null)
    {
        configParams.Panel.Visible = Convert.ToBoolean(configParams.SchemeConfigurationItem.Active);
    }


    if (Convert.ToBoolean(configParams.SchemeConfigurationItem.Active))
    {
        Config = true;

        if (configParams.Control != null)
        {
            configParams.Control.CssClass = string.IsNullOrEmpty(configParams.SchemeConfigurationItem.CssClass) ? configParams.Control.CssClass : configParams.SchemeConfigurationItem.CssClass;
            configParams.Control.ToolTip = string.IsNullOrEmpty(configParams.SchemeConfigurationItem.ToolTip) ? configParams.Control.ToolTip : configParams.SchemeConfigurationItem.ToolTip;
            configParams.Control.CssClass = MandatoryHandler(configParams.Control.CssClass, configParams.RequiredFieldValidator.Enabled);
        }

        if (configParams.RequiredFieldValidator != null)
        {
            configParams.RequiredFieldValidator.Enabled = Convert.ToBoolean(configParams.SchemeConfigurationItem.Mandatory);
            configParams.RequiredFieldValidator.ErrorMessage = string.IsNullOrEmpty(configParams.SchemeConfigurationItem.ErrorMessage) ? configParams.RequiredFieldValidator.ErrorMessage : configParams.SchemeConfigurationItem.ErrorMessage;
        }
    }
}

This way you can configure each parameter separately without having to set other parameters explicitly to null:

configHandler(new ConfigParameters 
{
    SchemeConfigurationItem = ..., 
    Control = someCheckBoxList 
});

and the parameters are:

public class ConfigParameters
{
    public SchemeConfigurationItem SchemeConfigurationItem { get; set; }
    public IStyleable Control { get; set; }
    public bool Config { get; set; }
    public RequiredFieldValidator RequiredFieldValidator { get; set; }
    public Panel Panel { get; set; }
}
\$\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.