1
\$\begingroup\$

I have a piece of code that takes a string array with 'key/value' pairs split with a ';' and finds the type of each variable and initializes the variable to the value given. The code given works as needed, but think that there must be a better method to handle this. All fields in the classes below Fields are of type string or int. 

 public static void CreateIssue(string project, params string[] fields )
 {
      Issue data = new Issue();
      //Blank Fields class with all fields set to null
      data.fields = new Fields();

      data.fields.project = new Project() { key = project };

      foreach(string a in fields)
      {
          string field = a.Split(';')[0];
          string value = a.Split(';')[1];
          string custom = field;

           //if a customfield_XXXXXX, this will pull only the customfield portion
          if (field.Contains('_'))
          {
              custom = field.Split('_')[0];
          }

          Type cType = data.fields.GetType(custom);
          if (cType != null && data.fields[field]?.GetType() != typeof(DateTime))
          {
              try
              {
                  var type = System.Activator.CreateInstance(cType);
                  data.fields[field] = type;
              }catch (ArgumentException esc)
              {
                  if (!esc.Message.Contains("System.String"))
                      MessageBox.Show("Failed for an argument other than to string.\r\n" + esc.Message);
                  //Expect this to mean we missed the string type
              }catch(Exception exc)
              {
                  MessageBox.Show("Unknown Exception type occurred \r\n" + exc.Message);
              }
          }

          if (data.fields[field] is Customfield)
          {
              //attempt to use as a CustomField
              Customfield lData = new Customfield();
              lData.id = Convert.ToInt32(value);//should be code number for id of required set
                                                //if more than one data, max should be two, add child data
              if (a.Split(';').Length > 2)
              {
                  Child lChild = new Child();
                  lChild.id = Convert.ToInt32(a.Split(':')[2]);
                  lData.child = lChild;
              }
              data.fields[field] = lData;
          }
          else if (data.fields[field] is DateTime)
          {
              data.fields[field] = Convert.ToDateTime(value);
          }
          else if (data.fields[field] is Issuetype)
          {
              data.fields[field] = new Issuetype() { id = value };
          }
          else if(field == Constants.Components)
          {
              List<Component> temp = new List<Component>();
              temp.Add(new Component() { name = field });
              data.fields[field] = temp;
          }else
          {
              data.fields[field] = value;
          }
      }
      req.AddJsonBody(data);
}

This is the class Fields

public class Fields
{
    public object this[string propertyName]
    {
        get
        {
            // probably faster without reflection:
            // like:  return Properties.Settings.Default.PropertyValues[propertyName] 
            // instead of the following
            Type myType = typeof(Fields);
            PropertyInfo myPropInfo = myType.GetProperty(propertyName);
            return myPropInfo.GetValue(this, null);
        }
        set
        {
            Type myType = typeof(Fields);
            PropertyInfo myPropInfo = myType.GetProperty(propertyName);
            myPropInfo.SetValue(this, value, null);
        }

    }
    public Type GetType(string field )
    {
        string type = field;
        if (!char.IsUpper(field[0]))
        {
            char[] b = field.ToCharArray();
            b[0] = char.ToUpper(b[0]);
            type = new string(b);
        }

        return Type.GetType("Central_Processing." + type);
    }
    public Fields()
    {
        this.project = new Project();
    }
    public Fields(params string[] accessors )
    {
        foreach(string a in accessors)
        {
            string type = a;
            if (!char.IsUpper(a[0]))
            {
                char[] b = a.ToCharArray();
                b[0] = char.ToUpper(b[0]);
                type = new string(b);
            }

            if((Type.GetType("Central_Processing." + type) ?? typeof(string)) == typeof(string))
            {
                break;
            }
            try
            {
                this.GetType().GetProperty(a)?.SetValue(this, System.Activator.CreateInstance(Type.GetType("Central_Processing." + type)));
            }catch(Exception esc)
            {
                System.Windows.Forms.MessageBox.Show(esc.Message);
            }
        }
    }
    public string customfield_10073 { get; set; }
    public string customfield_14311 { get; set; }
    public string customfield_14312 { get; set; }
    public string customfield_12010 { get; set; }
    public string customfield_14310 { get; set; }
    public string customfield_14315 { get; set; }
    public string customfield_14316 { get; set; }
    public string customfield_14313 { get; set; }
    public string customfield_14314 { get; set; }
    public string customfield_10510 { get; set; }
    public string customfield_11711 { get; set; }
    public string customfield_11710 { get; set; }
    public string lastViewed { get; set; }
    public string customfield_10060 { get; set; }
    public string customfield_10061 { get; set; }
    public string customfield_10062 { get; set; }
    public Customfield customfield_13210 { get; set; }
    public List<string> labels { get; set; }
    public Customfield customfield_10610 { get; set; }
    public string customfield_12910 { get; set; }
    public string customfield_12912 { get; set; }
    public string customfield_12911 { get; set; }
    public string customfield_12914 { get; set; }
    public string aggregatetimeoriginalestimate { get; set; }
    public string customfield_12913 { get; set; }
    public string customfield_12915 { get; set; }
    public string assignee { get; set; }
    public List<Component> components { get; set; }
    public string customfield_14410 { get; set; }
    public Customfield customfield_13320 { get; set; }
    public string customfield_15500 { get; set; }
    public string customfield_14411 { get; set; }
    public string customfield_12111 { get; set; }
    public string customfield_14412 { get; set; }
    public string customfield_10730 { get; set; }
    public string customfield_13317 { get; set; }
    public string customfield_11810 { get; set; }
    public Customfield customfield_13316 { get; set; }
    public Customfield customfield_13319 { get; set; }
    public Customfield customfield_11812 { get; set; }
    public string customfield_13318 { get; set; }
    public string customfield_11811 { get; set; }
    public string customfield_10728 { get; set; }
    public List<string> subtasks { get; set; }
    public string customfield_10041 { get; set; }
    public string customfield_13311 { get; set; }
    public string customfield_13310 { get; set; }
    public string customfield_13313 { get; set; }
    public string customfield_13312 { get; set; }
    public string customfield_13315 { get; set; }
    public string customfield_13314 { get; set; }
    public Customfield customfield_10710 { get; set; }
    public string customfield_10712 { get; set; }
    public Issuetype issuetype { get; set; }
    public string customfield_14510 { get; set; }
    public Project project { get; set; }
    public string customfield_12210 { get; set; }
    public string customfield_15600 { get; set; }
    public string customfield_11911 { get; set; }
    public string customfield_11910 { get; set; }
    public string resolutiondate { get; set; }
    public string customfield_16000 { get; set; }
    public string customfield_13410 { get; set; }
    public string customfield_10815 { get; set; }
    public DateTime updated { get; set; }
    public string timeoriginalestimate { get; set; }
    public string description { get; set; }
    public string customfield_10010 { get; set; }
    public string customfield_10011 { get; set; }
    public string customfield_10013 { get; set; }
    public string customfield_14610 { get; set; }
    public string customfield_15700 { get; set; }
    public string customfield_10015 { get; set; }
    public string summary { get; set; }
    public string customfield_16100 { get; set; }
    public string customfield_13510 { get; set; }
    public string environment { get; set; }
    public string customfield_10912 { get; set; }
    public string duedate { get; set; }
    public string customfield_10230 { get; set; }
    public string customfield_10231 { get; set; }
    public List<string> fixVersions { get; set; }
    public string customfield_10111 { get; set; }
    public string customfield_10232 { get; set; }
    public string customfield_15800 { get; set; }
    public string customfield_12410 { get; set; }
    public string customfield_14710 { get; set; }
    public string customfield_10226 { get; set; }
    public Customfield customfield_10227 { get; set; }
    public string customfield_10228 { get; set; }
    public string customfield_10229 { get; set; }
    public string customfield_16200 { get; set; }
    public string customfield_15114 { get; set; }
    public string customfield_15115 { get; set; }
    public string customfield_10220 { get; set; }
    public string customfield_13610 { get; set; }
    public string customfield_10221 { get; set; }
    public string customfield_10222 { get; set; }
    public string customfield_15116 { get; set; }
    public string customfield_10223 { get; set; }
    public string customfield_15117 { get; set; }
    public string customfield_10224 { get; set; }
    public string customfield_12514 { get; set; }
    public DateTime customfield_10214
    {
        get;
        set;
    }
    public string customfield_12513 { get; set; }
    public string customfield_10215 { get; set; }
    public string customfield_12516 { get; set; }
    public string customfield_10216 { get; set; }
    public string customfield_12515 { get; set; }
    public string customfield_10217 { get; set; }
    public string customfield_10218 { get; set; }
    public string timeestimate { get; set; }
    public List<string> versions { get; set; }
    public Customfield customfield_14013 { get; set; }
    public string customfield_14810 { get; set; }
    public Customfield customfield_12510 { get; set; }
    public Customfield customfield_10210 { get; set; }
    public string customfield_15900 { get; set; }
    public string customfield_12512 { get; set; }
    public Customfield customfield_12511 { get; set; }
    public DateTime customfield_10213
    {
        get;
        set;
    }
    public string aggregatetimeestimate { get; set; }
    public string customfield_15210 { get; set; }
    public Customfield customfield_11410 { get; set; }
    public string customfield_12620 { get; set; }
    public string customfield_13710 { get; set; }
    public string customfield_11524 { get; set; }
    public string customfield_12613 { get; set; }
    public string customfield_10314 { get; set; }
    public string customfield_11523 { get; set; }
    public string customfield_12612 { get; set; }
    public List<Customfield> customfield_10315 { get; set; }
    public string customfield_12615 { get; set; }
    public string customfield_12614 { get; set; }
    public string customfield_12617 { get; set; }
    public string customfield_12616 { get; set; }
    public string customfield_12619 { get; set; }
    public string customfield_12618 { get; set; }
    public string customfield_14110 { get; set; }
    public string timespent { get; set; }
    public string aggregatetimespent { get; set; }
    public string customfield_10310 { get; set; }
    public Customfield customfield_11522 { get; set; }
    public string customfield_12611 { get; set; }
    public Customfield customfield_11513 { get; set; }
    public Customfield customfield_11512 { get; set; }
    public Customfield customfield_10426 { get; set; }
    public Customfield customfield_11514 { get; set; }
    public string customfield_10427 { get; set; }
    public List<Customfield> customfield_10428 { get; set; }
    public string workratio { get; set; }
    public string customfield_13010 { get; set; }
    public DateTime created { get; set; }
    public string customfield_15310 { get; set; }
    public Customfield customfield_10420 { get; set; }
    public string customfield_13810 { get; set; }
    public string customfield_10421 { get; set; }
    public Customfield customfield_11511 { get; set; }
    public string customfield_10422 { get; set; }
    public Customfield customfield_11510 { get; set; }
    public Customfield customfield_10413 { get; set; }
    public string customfield_10416 { get; set; }
    public string customfield_10417 { get; set; }
    public string customfield_10419 { get; set; }
    public string customfield_10092 { get; set; }
    public string customfield_10093 { get; set; }
    public string customfield_10094 { get; set; }
    public string customfield_13120 { get; set; }
    public string customfield_15422 { get; set; }
    public string customfield_13122 { get; set; }
    public string customfield_15423 { get; set; }
    public string customfield_13121 { get; set; }
    public string customfield_14210 { get; set; }
    public string customfield_15420 { get; set; }
    public string customfield_10098 { get; set; }
    public string customfield_15421 { get; set; }
    public string customfield_15424 { get; set; }
    public string customfield_12710 { get; set; }
    public string customfield_15419 { get; set; }
    public string customfield_13119 { get; set; }
    public string customfield_13118 { get; set; }
    public string customfield_15417 { get; set; }
    public string customfield_15418 { get; set; }
    public List<string> attachment { get; set; }
    public string customfield_13111 { get; set; }
    public string customfield_13113 { get; set; }
    public string customfield_15415 { get; set; }
    public string customfield_13115 { get; set; }
    public string customfield_15416 { get; set; }
    public string customfield_13114 { get; set; }
    public string customfield_11610 { get; set; }
    public string customfield_13117 { get; set; }
    public string customfield_15414 { get; set; }
    public string customfield_13116 { get; set; }
    public string customfield_12810 { get; set; }
}
\$\endgroup\$
3
  • \$\begingroup\$ You should only split a once \$\endgroup\$ Commented Jan 5, 2018 at 16:07
  • 1
    \$\begingroup\$ Why do you have so many similar properites in theFields class like the customfield_10098? What are they about? I cannot imagine anyone actually using them. What is your goal? \$\endgroup\$ Commented Jan 6, 2018 at 17:51
  • \$\begingroup\$ @t3chb0t Each of the items in Fields is a settable field in JIRA. I need to have them set to null or the value expected when uploading the JSON data, or it will clear any data present when sending the data up. All are necessary to be present for usage elsewhere. \$\endgroup\$ Commented Jan 6, 2018 at 18:04

2 Answers 2

Reset to default
1
\$\begingroup\$

Major problem

Your code contains a big flaw: it's never actually checking the type of the target property!

  1. First, you're extracting the field name (or just "Customfield" if it contains an underscore).
  2. Then you're passing that on to Fields.GetType(string field), but that method treats field as a type name! The only reason that this appears to work is that some properties have the same name as their type (Issuetype issuetype), but it's essentially broken. The Fields(params string[] accessors) constructor is making the same mistake.
  3. Then you're trying to create an instance of that type, and you assign it to that property. In many cases, that will fail or produce an object of an incorrect type (string customfield_10073 produces a Customfield object, not a string).
  4. Then, you fetch that instance again, check its type, and that's where the type-specific value handling comes in. Note that not all property types are taken into account.

In short: do not confuse the name of a property with its type. The type of string Foobar { get; set; } is string, not Foobar.

You can fix this by letting Fields.GetType(string field) return GetProperty(field)?.PropertyType. You'll also need to remove that special customfield name handling, and there's no need for uppercasing names anymore. You can also skip step 3, because step 2 already tells you what target type you're dealing with.

Other improvements

  • There are several property types that you're not taking into account: List<string>, Project and List<Customfield>.
  • lChild.id = Convert.ToInt32(a.Split(':')[2]); contains a typo: it should split on ;. But there's no need to call Split multiple times: just call it once and store the results in an array, then work with that.
  • You may want to take non-existing field names into account ("foobar;foo" causes a rather generic NullReferenceException). And should calling code be allowed to set any property, including project?
  • Do you want any user of the Fields class to be able to access fields by name, or is that only a convenience for CreateIssue? If so, you may want to move all that reflection code into CreateIssue.
  • Type myType = typeof(Fields) can be simplified to GetType(), unless you want to hide fields from derived classes.
  • I don't know in what context the CreateIssue method will be used, but if it's a 'low-level' utility method it shouldn't display message boxes. Instead, it should throw exceptions or return an error result, so that (higher-level) UI code can display an appropriate error message. That allows your method to be reused in different contexts.
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for identifying that for me, I was sure there was something causing the level of work being written in that meant I was missing something. For the other improvements: Added in missing property types (was holding back until I found a better way to handle them), Split was fixed from previous answer suggestion, exception handling added in, All users should be capable of calling Fields not just CreateIssue, All message boxes were switched over as needed. \$\endgroup\$ Commented Jan 8, 2018 at 3:54
1
\$\begingroup\$

I recommend reducing every method until it's 15 lines or less. When methods are easier to read it becomes easier to see how to improve.

You could replace this: 

      string field = a.Split(';')[0];
      string value = a.Split(';')[1];
      string custom = field;

      //if a customfield_XXXXXX, this will pull only the customfield portion
      if (field.Contains('_'))
      {
          custom = field.Split('_')[0];
      }

with a function that returns an object.

public class FieldInfo
{
    public string Field { get; }
    public string Value { get; }
    public string Custom { get; }

    public FieldInfo(string field, string value, string custom)
    {
        Field = field;
        Value = value;
        Custom = value;
    }

    public static FieldInfo Parse(string input)
    {
        string field = null;
        string value = null;
        string custom = null;
        var split = input.Split(';');
        if (split.Length == 2)
        {
            field = split[0];
            value = split[1];
            var fieldSplit = field.Split('_');
            custom = fieldSplit.Length > 1 ? fieldSplit[0] : field;
            return new FieldInfo(field, value, custom);
        }
        throw new ArgumentException("Unable to parse input.");
    }
}

It's more code, but now you can handle malformed strings or whatnot without having to make the main method even longer with validation. And the main method loses replaces six lines with one.

Now you can use

var fieldInfo = FieldInfo.Parse(a);

Whatever it is that you're doing with these fields in the loop, I'd make that its own method, even separate methods for handling the different cases. It's the same code just separated out into methods so that each is smaller and easier to read. Every time you do that and give the method a descriptive name, the code becomes self-documenting. You can read it and tell what it's doing from the names of the methods that it calls.

It's good to write classes so that they can be unit tested. If your class attempts to display message boxes, there's no way to check for that in a unit test. It's better to for the class to either return values indicating success or failure or to throw exceptions. You can write unit tests to check for those. Then, if you use this class in a Windows Form, that form can use the output to determine whether to show a message.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.