1
\$\begingroup\$

I have found myself referencing controls on a form (think Windows forms but more proprietary) by hard-typing their names in over and over again. Often I'll have a number of similar controls which could be named coolControl1department, coolControl2department, coolControl1jobTitle, coolControl2jobTitle, etc. This becomes tedious and a nightmare to maintain. Renaming the controls is definitely an option but I would still need some way of generating their names to assign values and whatnot.

To make this less reliant on me typing in the names I've created a dictionary method to concatenate the various parts of the control names, including unique reference numbers, and then call the final values elsewhere.

While it's more centralised than it was previously, there is still room for improvement and I'd appreciate any suggestions to streamline this.

Note that I'm only able to use C# version 4.0 in this application. Code below.

    public void DoStuffWithControls()
    {
        Dictionary<string, string> myControls1 = GeneratedControlNames(1);
        Dictionary<string, string> myControls2 = GeneratedControlNames(2);
        string genericControl1;
        string genericControl2;
        string departmentControl1;
        string departmentControl2;
        myControls1.TryGetValue("generic", out genericControl1);
        myControls1.TryGetValue("department", out departmentControl1);
        myControls2.TryGetValue("generic", out genericControl2);
        myControls2.TryGetValue("department", out departmentControl2);            
        EnquiryForm.GetEnquiryControl(genericControl1, EnquiryControlMissing.Exception).Value = "some value";
        EnquiryForm.GetEnquiryControl(departmentControl1, EnquiryControlMissing.Exception).Value = "some value";
        EnquiryForm.GetEnquiryControl(genericControl1, EnquiryControlMissing.Exception).AnotherProperty = "some other value";
        EnquiryForm.GetEnquiryControl(genericControl2, EnquiryControlMissing.Exception).Value = "some value";
        EnquiryForm.GetEnquiryControl(genericControl2, EnquiryControlMissing.Exception).AnotherProperty = "some other value";
        EnquiryForm.GetEnquiryControl(departmentControl2, EnquiryControlMissing.Exception).Value = "some value";
    }

    private static Dictionary<string, string> GeneratedControlNames(int refNumber)
    {
        var genericPrefix = "coolControl";
        var departmentControlSuffix = "Department";
        var jobTitleControlSuffix = "Title";
        var directDialControlSuffix = "DDI";
        var emailDialControlSuffix = "Email";

        var dict = new Dictionary<string, string>();
        dict.Add("generic", string.Concat(genericPrefix, refNumber));
        dict.Add("department", string.Concat(genericPrefix, refNumber, departmentControlSuffix));
        dict.Add("jobTitle", string.Concat(genericPrefix, refNumber, jobTitleControlSuffix));
        dict.Add("directDial", string.Concat(genericPrefix, refNumber, directDialControlSuffix));
        dict.Add("email", string.Concat(genericPrefix, refNumber, emailDialControlSuffix));
        return dict;
    }
\$\endgroup\$
1
  • 1
    \$\begingroup\$ How much work has this really saved? You still need to do what amounts to typing out the identifier by typing out which dictionary it's in as well as the generalized label while also needing an out variable declaration and TryGetValue call. There are various ways to fix the issue, of course (such as what CharlesNRice mentioned), but I can't help but wonder if the most streamlined version of this would be to just have one dictionary or static class with all the labels that you can reference. This strikes me as a "don't invent a fancy tool when all you need is a hammer" kind of situation. \$\endgroup\$
    – Abion47
    Aug 31, 2018 at 16:49

1 Answer 1

1
\$\begingroup\$

I think the dictionary is overkill. Plus adds a lot more code with the TryGet. I would make a class to help and hide the magic strings.

As a side note magic strings should be made into constants so they are maintained in one place and if a typo you just need to fix it in one place. Also having a 1 or 2 being passed in also seems magic to me. I would make that into an enum. If we left it as an int then when coming back I have no idea what values I can pass in. Is negative numbers allowed? What about 9999?

public enum RefNumber
{
    One = 1,
    Two = 2,
}

I'd create a class to hold the magic strings

public static class ControlsNames
{
    private const string GenericPrefix = "coolControl";
    private const string DepartmentSufix = "Department";
    private const string JobTitleControlSuffix = "JobTitle";
    private const string DirectDialControlSuffix = "DDI";
    private const string EmailDialControlSuffix = "Email";

Looking at your example you always format with the Prefix then refnumber then optional suffix. I'll make a method in the class to handle that for us

private static string FormatName(string suffix, RefNumber refNumber)
{
     return string.Concat(GenericPrefix, (int)refNumber, suffix);
}

Now we just need to make an easy way to know what suffix we need. I'm going to make some static methods that will do that for us and return back the string. The only exception is Generic we don't want a suffix but we can just pass in string.empty

public static string Generic(RefNumber refNumber)
{
    return FormatName(string.Empty, refNumber);
}

public static string Department(RefNumber refNumber)
{
    return FormatName(DepartmentSufix, refNumber);
}

public static string JobTitle(RefNumber refNumber)
{
    return FormatName(JobTitleControlSuffix, refNumber);
}

public static string DirectDial(RefNumber refNumber)
{
    return FormatName(DirectDialControlSuffix, refNumber);
}

public static string Email(RefNumber refNumber)
{
    return FormatName(EmailDialControlSuffix, refNumber);
}

Now we don't have any dictionary or TryGets and the code looks like this now

public void DoStuffWithControls()
{
    string genericControl1 = ControlsNames.Generic(RefNumber.One);
    string genericControl2 = ControlsNames.Generic(RefNumber.Two);
    string departmentControl1 = ControlsNames.Department(RefNumber.One);;
    string departmentControl2 = ControlsNames.Department(RefNumber.Two); ;

    EnquiryForm.GetEnquiryControl(genericControl1, EnquiryControlMissing.Exception).Value = "some value";
    EnquiryForm.GetEnquiryControl(departmentControl1, EnquiryControlMissing.Exception).Value = "some value";
    EnquiryForm.GetEnquiryControl(genericControl1, EnquiryControlMissing.Exception).AnotherProperty = "some other value";
    EnquiryForm.GetEnquiryControl(genericControl2, EnquiryControlMissing.Exception).Value = "some value";
    EnquiryForm.GetEnquiryControl(genericControl2, EnquiryControlMissing.Exception).AnotherProperty = "some other value";
    EnquiryForm.GetEnquiryControl(departmentControl2, EnquiryControlMissing.Exception).Value = "some value";
}
\$\endgroup\$
5
  • \$\begingroup\$ Having an enum where One = 1, Two = 2, etc. whose only purpose is to replace integer literals with english-word equivalents strikes me as being incredibly redundant, nor I think does it make the values any less magic number-y. \$\endgroup\$
    – Abion47
    Aug 31, 2018 at 16:33
  • \$\begingroup\$ @Abion47 honestly not a lot but does constrain what values can be passed in that are valid. Otherwise you have to either build an incorrect string or throw an ArgumentOutOfRangeException at runtime. I would prefer to know valid values at design time. \$\endgroup\$ Aug 31, 2018 at 16:35
  • \$\begingroup\$ But I would argue that the whole point of this approach would be to help with an arbitrary number of controls with similar names. With the enum, if OP were to add a third control under a particular identifier, they now have to edit the enum and the place the enum is implemented. This method to eliminate the magic number has backfired in that it's creating more work to maintain instead of reducing it. \$\endgroup\$
    – Abion47
    Aug 31, 2018 at 16:38
  • \$\begingroup\$ @Abion47 its one small part. If OP wants to make it uint then it's his decision. Only he knows right now if it's worth handing design time control with maintenance or not. Same argument could be made if he added another field like zip or last name. It's a small piece of the code to me it's not worth arguing over. \$\endgroup\$ Aug 31, 2018 at 16:44
  • \$\begingroup\$ It's unlikely to be more than 1-6, currently it's 1-3. I essentially have a bunch of repeating controls each time someone 'adds' an item. Then I need to reference the controls over and over again to do various stuff, validate, etc. I like the idea here though and it's definitley given me some ideas for going forward. Thanks :) \$\endgroup\$ Sep 3, 2018 at 8:49

Your Answer

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

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