trjhtr

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.rn" + esc.Message);
 //Expect this to mean we missed the string type
 catch(Exception exc)
 
 MessageBox.Show("Unknown Exception type occurred rn" + 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; 

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.

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.