Solved

Refactor C# code

Posted on 2016-07-21
13
127 Views
Last Modified: 2016-09-08
I want to refactor the below C# code.  (Using .NET 4.5)  Any suggestions or directions for refactoring the entire code


I am copying data from GetTransactionResponse "resp" to Request "req" object.
a>resp has some properties and some arrays coming from service call
b>req is class I want to convert ultimately to Json for communication

I am using 4.6

  public Request ConvertToRequest(GetTransactionResponse resp)
        {
            Request req = new Request();
            List<SalesOrder> salesOrders = new List<SalesOrder> { };

            if (resp.transaction != null & resp.status.success)
            {
                 foreach (AttributeType quoteAttr in resp.transaction.quote_data.attributes)
                {
                    if(!String.IsNullOrEmpty(quoteAttr.value))
                    {
                        if(quoteAttr.field == "processing_quote")
                        {
                            req.regionCode = quoteAttr.value;
                        }
                        else if (quoteAttr.field == "salesOffice_quote")
                        {
                            req.regionalSalesOffice = quoteAttr.value;
                        }
                        else if(quoteAttr.field == "s_quote")
                        {
                            req.regionalSalesOffice = quoteAttr.value;
                        }
                        else if (quoteAttr.field == "_quote_company_name")
                        {
                            req.companyName = quoteAttr.value;
                        }
                        else if (quoteAttr.field == "_email")
                        {
                            req.contactEmailAddress = quoteAttr.value;
                        }
                        else if (quoteAttr.field == "preparedBy_quote")
                        {
                            req.dmName = quoteAttr.value;
                        }
                    }
                }
                salesOrders = new List<SalesOrder>();
                List<ScnCode> scnListCode = new List<ScnCode>();
                     
                foreach (SubDocType subDoc in resp.transaction.quote_data.sub_documents)
                {
                    SalesOrder salesOrder = new SalesOrder();
                    AttributeType prodAttrs = subDoc.attributes[0];
                    if (prodAttrs != null && !string.IsNullOrEmpty(prodAttrs.value))
                    {
                        string[] values = prodAttrs.value.Split(new string[] { "**" }, StringSplitOptions.None);
                        salesOrder.productCode = values[1];
                        salesOrder.salesOrderType = values[2];
                        salesOrder.startDate = values[3];
                        salesOrder.numberOfPays = (!string.IsNullOrEmpty(values[4])?int.Parse(values[4]):0);
                       
                        foreach (string scn in values[5].Split(new string[] { "$$" }, StringSplitOptions.RemoveEmptyEntries))
                        {
                            scnListCode.Add(new acsModels.ScnCode { scnCode =scn });
                        }
                        salesOrder.scnCodes = scnListCode;
                        salesOrders.Add(salesOrder);
                    }
                }

                req.salesOrders = salesOrders;
            }
            return req;

        }
0
Comment
Question by:Member_2_7967608
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
  • 7
  • 5
13 Comments
 
LVL 34

Accepted Solution

by:
it_saige earned 500 total points
ID: 41723432
Probably the first thing I would do is create a FieldMapping attribute that can be applied to the Request properties; e.g.:
[AttributeUsage(AttributeTargets.Property, AllowMultiple = true)]
class FieldMappingAttribute : Attribute
{
	public string MapTo { get; set; }
}

class Request
{
	[FieldMapping(MapTo="processing_quote")]
	public string RegionCode { get; set; }
	[FieldMapping(MapTo = "salesoffice_quote"), FieldMapping(MapTo="s_quote")]
	public string RegionalSalesOffice { get; set; }
	[FieldMapping(MapTo = "_quote_company_name")]
	public string CompanyName { get; set; }
	[FieldMapping(MapTo = "_email")]
	public string ContactEmailAddress { get; set; }
	[FieldMapping(MapTo = "preparedby_quote")]
	public string DMName { get; set; }
	public List<SalesOrder> SalesOrders { get; set; }
}

Open in new window

Using this we could then create an extension method that takes a request as a the target type parameter (in other words - this) and then a collection of attributes (probably a dictionary as the linq to build this is pretty straight forward based upon the information provided; e.g. -
static class Extensions
{
	private static void ApplyRequestAttributes(this Request request, Dictionary<string, object> attributes)
	{
		foreach (PropertyInfo property in request.GetType().GetProperties(BindingFlags.Instance | BindingFlags.Public))
		{
			FieldMappingAttribute[] mappings = (FieldMappingAttribute[])property.GetCustomAttributes(typeof(FieldMappingAttribute), false);
			if (mappings.Length > 0)
			{
				object value = null;
				foreach (var mapping in mappings)
				{
					if (attributes.TryGetValue(mapping.MapTo, out value))
					{
						property.SetValue(request, value, null);
						break;
					}
				}
			}
		}
	}
}

Open in new window

The above would be called like this:
var attributes = (from attribute in response.Transaction.QuoteData.Attributes 
			   group attribute by attribute.Field into grp 
			   let pair = grp.FirstOrDefault() 
			   where pair != null && !string.IsNullOrWhiteSpace(pair.Value.ToString())
			   select new KeyValuePair<string, object>(pair.Field, pair.Value)).ToDictionary(k => k.Key, v => v.Value, StringComparer.OrdinalIgnoreCase);
result.ApplyRequestAttributes(attributes);

Open in new window

The remainder of the method could then be easily refactored into a linq statement; e.g. -
result.SalesOrders = (from document in response.Transaction.QuoteData.SubDocuments
				  let values = document.Attributes[0].Value.ToString().Split(new[] { "**" }, StringSplitOptions.None)
				  where (document.Attributes != null && document.Attributes.Count > 0) && !string.IsNullOrWhiteSpace(document.Attributes[0].Value.ToString())
				  select new SalesOrder()
				  {
					  ProductCode = values[1],
					  SalesOrderType = values[2],
					  StartDate = values[3],
					  NumberOfPays = !string.IsNullOrWhiteSpace(values[4]) ? int.Parse(values[4]) : 0,
					  ScnCodes = (from code in values[5].Split(new[] { "$$" }, StringSplitOptions.None) select new ScnCode() { Code = code }).ToList()
				  }).ToList();

Open in new window

Putting it all together we get a fairly straight-forward pair of extension methods -
static class Extensions
{
	public static Request ConvertToRequest(this GetTransactionResponse response)
	{
		Request result = new Request();
		// Assuming that response is not null and successful
		if (response != null && response.Success)
		{
			var attributes = (from attribute in response.Transaction.QuoteData.Attributes 
						   group attribute by attribute.Field into grp 
						   let pair = grp.FirstOrDefault() 
						   where pair != null && !string.IsNullOrWhiteSpace(pair.Value.ToString())
						   select new KeyValuePair<string, object>(pair.Field, pair.Value)).ToDictionary(k => k.Key, v => v.Value, StringComparer.OrdinalIgnoreCase);
			result.ApplyRequestAttributes(attributes);
			result.SalesOrders = (from document in response.Transaction.QuoteData.SubDocuments
							  let values = document.Attributes[0].Value.ToString().Split(new[] { "**" }, StringSplitOptions.None)
							  where (document.Attributes != null && document.Attributes.Count > 0) && !string.IsNullOrWhiteSpace(document.Attributes[0].Value.ToString())
							  select new SalesOrder()
							  {
								  ProductCode = values[1],
								  SalesOrderType = values[2],
								  StartDate = values[3],
								  NumberOfPays = !string.IsNullOrWhiteSpace(values[4]) ? int.Parse(values[4]) : 0,
								  ScnCodes = (from code in values[5].Split(new[] { "$$" }, StringSplitOptions.None) select new ScnCode() { Code = code }).ToList()
							  }).ToList();
		}
		return result;
	}

	private static void ApplyRequestAttributes(this Request request, Dictionary<string, object> attributes)
	{
		foreach (PropertyInfo property in request.GetType().GetProperties(BindingFlags.Instance | BindingFlags.Public))
		{
			FieldMappingAttribute[] mappings = (FieldMappingAttribute[])property.GetCustomAttributes(typeof(FieldMappingAttribute), false);
			if (mappings.Length > 0)
			{
				object value = null;
				foreach (var mapping in mappings)
				{
					if (attributes.TryGetValue(mapping.MapTo, out value))
					{
						property.SetValue(request, value, null);
						break;
					}
				}
			}
		}
	}
}

Open in new window

Proof of overall concept based upon provided structure -
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;

namespace EE_Q28958730
{
	class Program
	{
		static void Main(string[] args)
		{
			GetTransactionResponse response = new GetTransactionResponse()
			{
				Success = true,
				Transaction = new Transaction()
				{
					QuoteData = new QuoteData()
					{
						Attributes = new List<AttributeType>()
						{
							new AttributeType() { Field = "ProCesSinG_qUotE", Value = "1" },
							new AttributeType() { Field = "s_qUote", Value = "REGION5" },
							new AttributeType() { Field = "_qUotE_cOmpAny_nAme", Value = "ACME" },
							new AttributeType() { Field = "_emAIl", Value = "paul@acme.com" },
							new AttributeType() { Field = "prEparEdBy_quotE", Value = "Nancy" }
						},
						SubDocuments = new List<SubDocType>()
						{
							new SubDocType() { Attributes = new List<AttributeType>() { new AttributeType() { Field = "products", Value = "0**9**REG**03/29/2016**4**1$$2$$3$$4" }} },
							new SubDocType() { Attributes = new List<AttributeType>() { new AttributeType() { Field = "products", Value = "1**8**NOM**04/29/2016**8**1$$2$$3$$4$$5$$6$$7$$8" }} },
							new SubDocType() { Attributes = new List<AttributeType>() { new AttributeType() { Field = "products", Value = "2**7**REG**05/29/2016**16**1$$2$$3$$4$$5$$6$$7$$8$$9$$10$$11$$12$$13$$14$$15$$16" }} },
							new SubDocType() { Attributes = new List<AttributeType>() { new AttributeType() { Field = "products", Value = "3**6**NOM**06/29/2016**8**1$$2$$3$$4$$5$$6$$7$$8" }} },
							new SubDocType() { Attributes = new List<AttributeType>() { new AttributeType() { Field = "products", Value = "4**5**REG**07/29/2016**4**1$$2$$3$$4" }} }
						}
					}
				}
			}; 
			
			var request = response.ConvertToRequest();
			Console.WriteLine("Request for {0} at {1}", request.ContactEmailAddress, request.CompanyName);
			Console.WriteLine("From {0} in {1} by {2}", request.RegionalSalesOffice, request.RegionCode, request.DMName);
			foreach (var order in request.SalesOrders)
			{
				Console.WriteLine("Product Code: {0}; Sales Order Type: {1}; Start Date: {2}; Number Of Pays: {3}", order.ProductCode, order.SalesOrderType, order.StartDate, order.NumberOfPays);
				Console.WriteLine("ScnCodes: {0}", string.Join(" | ", (from code in order.ScnCodes select code.Code).ToArray()));
			}
			Console.ReadLine();
		}
	}

	static class Extensions
	{
		public static Request ConvertToRequest(this GetTransactionResponse response)
		{
			Request result = new Request();
			// Assuming that response is not null and successful
			if (response != null && response.Success)
			{
				var attributes = (from attribute in response.Transaction.QuoteData.Attributes 
							   group attribute by attribute.Field into grp 
							   let pair = grp.FirstOrDefault() 
							   where pair != null && !string.IsNullOrWhiteSpace(pair.Value.ToString())
							   select new KeyValuePair<string, object>(pair.Field, pair.Value)).ToDictionary(k => k.Key, v => v.Value, StringComparer.OrdinalIgnoreCase);
				result.ApplyRequestAttributes(attributes);
				result.SalesOrders = (from document in response.Transaction.QuoteData.SubDocuments
								  let values = document.Attributes[0].Value.ToString().Split(new[] { "**" }, StringSplitOptions.None)
								  where (document.Attributes != null && document.Attributes.Count > 0) && !string.IsNullOrWhiteSpace(document.Attributes[0].Value.ToString())
								  select new SalesOrder()
								  {
									  ProductCode = values[1],
									  SalesOrderType = values[2],
									  StartDate = values[3],
									  NumberOfPays = !string.IsNullOrWhiteSpace(values[4]) ? int.Parse(values[4]) : 0,
									  ScnCodes = (from code in values[5].Split(new[] { "$$" }, StringSplitOptions.None) select new ScnCode() { Code = code }).ToList()
								  }).ToList();
			}
			return result;
		}

		private static void ApplyRequestAttributes(this Request request, Dictionary<string, object> attributes)
		{
			foreach (PropertyInfo property in request.GetType().GetProperties(BindingFlags.Instance | BindingFlags.Public))
			{
				FieldMappingAttribute[] mappings = (FieldMappingAttribute[])property.GetCustomAttributes(typeof(FieldMappingAttribute), false);
				if (mappings.Length > 0)
				{
					object value = null;
					foreach (var mapping in mappings)
					{
						if (attributes.TryGetValue(mapping.MapTo, out value))
						{
							property.SetValue(request, value, null);
							break;
						}
					}
				}
			}
		}
	}

	[AttributeUsage(AttributeTargets.Property, AllowMultiple = true)]
	class FieldMappingAttribute : Attribute
	{
		public string MapTo { get; set; }
	}

	class GetTransactionResponse
	{
		public Transaction Transaction { get; set; }
		public bool Success { get; set; }
	}

	class Transaction
	{
		public QuoteData QuoteData { get; set; }
	}

	class QuoteData
	{
		public List<AttributeType> Attributes { get; set; }
		public List<SubDocType> SubDocuments { get; set; }
	}

	class SubDocType
	{
		public List<AttributeType> Attributes { get; set; }
	}

	class AttributeType
	{
		public string Field { get; set; }
		public object Value { get; set; }
	}

	class Request
	{
		[FieldMapping(MapTo="processing_quote")]
		public string RegionCode { get; set; }
		[FieldMapping(MapTo = "salesoffice_quote"), FieldMapping(MapTo="s_quote")]
		public string RegionalSalesOffice { get; set; }
		[FieldMapping(MapTo = "_quote_company_name")]
		public string CompanyName { get; set; }
		[FieldMapping(MapTo = "_email")]
		public string ContactEmailAddress { get; set; }
		[FieldMapping(MapTo = "preparedby_quote")]
		public string DMName { get; set; }
		public List<SalesOrder> SalesOrders { get; set; }
	}

	class SalesOrder
	{
		public string ProductCode { get; set; }
		public string SalesOrderType { get; set; }
		public string StartDate { get; set; }
		public int NumberOfPays { get; set; }
		public List<ScnCode> ScnCodes { get; set; }
	}

	class ScnCode
	{
		public string Code { get; set; }
	}
}

Open in new window

Which produces the following output -Capture.JPGNOTE:  You will probably notice the case discrepancies in the fields.  This was so that I could test the case-insensitivity aspect of the dictionary key's, in this way, no matter the case as long as field mapping term matches the dictionary key term, we should be good.  Could probably also do with some exception checking and parsing, but I think you get the overall gist.

-saige-
0
 

Author Comment

by:Member_2_7967608
ID: 41728069
The code works well. Thank you very much. I see an exception as shown in attached, even though the error doesn't propagate to UI.
0
 

Author Comment

by:Member_2_7967608
ID: 41728070
File Attached
0
Increase Agility with Enabled Toolchains

Connect your existing build, deployment, management, monitoring, and collaboration platforms. From Puppet to Chef, HipChat to Slack, ServiceNow to JIRA, Splunk to New Relic and beyond, hand off data between systems to engage the right people.

Connect with xMatters.

 
LVL 34

Expert Comment

by:it_saige
ID: 41728133
There is no attachment.  Try taking a screen shot or use the snipping tool to cut out a screenshot of the exception.  You can try to copy and paste the exception.

-saige-
0
 

Author Comment

by:Member_2_7967608
ID: 41728550
0
 
LVL 34

Expert Comment

by:it_saige
ID: 41729563
That error is perfectly normal (and expected).  The exception itself is being set because the DeclaringMethod is not a generic (and it isn't); refer to - https://msdn.microsoft.com/en-us/library/system.type.declaringmethod(v=vs.110).aspx for more information.

-saige-
0
 

Author Comment

by:Member_2_7967608
ID: 41729582
Thanks saige.  Do you see any performance issue. This is encapsulated part of service .
But total # of Fields would be not more than 10.
0
 
LVL 34

Assisted Solution

by:it_saige
it_saige earned 500 total points
ID: 41729693
On the whole I would say no, however, if we really wanted to split hairs, we could move the variable declarations for mappings and value outside the loop since they are explicitly set inside the loop; e.g. -
private static void ApplyRequestAttributes(this Request request, Dictionary<string, object> attributes)
{
	FieldMappingAttribute[] mappings = default(FieldMappingAttribute[]);
	object value = default(object);

	foreach (PropertyInfo property in request.GetType().GetProperties(BindingFlags.Instance | BindingFlags.Public))
	{
		mappings = (FieldMappingAttribute[])property.GetCustomAttributes(typeof(FieldMappingAttribute), false);
		if (mappings.Length > 0)
		{
			value = null;
			foreach (var mapping in mappings)
			{
				if (attributes.TryGetValue(mapping.MapTo, out value))
				{
					property.SetValue(request, value, null);
					break;
				}
			}
		}
	}
}

Open in new window


Depending on the total number of properties in the target class, you could make an argument for using the attributes dictionary as the loop mechanism and matching to the property in the target class from the attribute instead of the current method; e.g. -
private static void ApplyRequestAttributes(this Request request, Dictionary<string, object> attributes)
{
	PropertyInfo[] properties = request.GetType().GetProperties(BindingFlags.Instance | BindingFlags.Public);
	PropertyInfo property = default(PropertyInfo);

	foreach (var attribute in attributes)
	{
		property = (from p in properties
				  from map in (FieldMappingAttribute[])p.GetCustomAttributes(typeof(FieldMappingAttribute), false)
				  where map.MapTo.Equals(attribute.Key, StringComparison.OrdinalIgnoreCase)
				  select p).FirstOrDefault();

		if (property != null)
			property.SetValue(request, attribute.Value, null);
	}
}

Open in new window


In all honesty though, this way seems a little counter-intuitive and less efficient (as it would loop through all of the target class properties for each resolved attribute).  I would also point out that the attribute dictionary loop mechanism based implementation of the ApplyRequestAttributes uses four loops as opposed to two in the original ApplyRequestAttributes.

Also without knowing the exact make-up of the rest of your code base with regards to the provided hints to it's structure, I can't say that the refactored code will be any more (or less) efficient than that of the already existing code base.

-saige-
0
 

Author Comment

by:Member_2_7967608
ID: 41733057
Thanks
We have the following structure.
1.  We have Service A which gives Response A
2.  From Response A we create a new request B. The Code for refactor in this Question
    This is where I have to do the conversion from one Service response to Another request object and they have different structure.
3.  This Request B will be used to make another service call.

All proxies included in Wrapper.dll .
0
 
LVL 34

Expert Comment

by:it_saige
ID: 41733059
The process you have described is fairly straightforward.  There should not be any performance concerns with the refactored code.  If you are concerned about performance, then you could always perform some unit tests and analysis between both methods to see if there are any potential bottlenecks.

-saige-
0
 

Author Closing Comment

by:Member_2_7967608
ID: 41733115
Excellent answer provided.
0
 
LVL 34

Expert Comment

by:ste5an
ID: 41789314
Even when it is already solved: Have you took a look at AutoMapper?
0
 

Author Comment

by:Member_2_7967608
ID: 41789767
No I haven't used Automapper. Just quickly browsed it. It looks cool and seems to reduce code lot. I think field names should map to each other.

But my DTO has different names than the webservice object.
And some fields are field values like Value = "1**8**NOM**04/29/2016**8**1$$2$$3$$4$$5$$6$$7$$8" .

Can I map this to class object using Automapper.
0

Featured Post

Free Tool: Subnet Calculator

The subnet calculator helps you design networks by taking an IP address and network mask and returning information such as network, broadcast address, and host range.

One of a set of tools we're offering as a way of saying thank you for being a part of the community.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

This article is for Object-Oriented Programming (OOP) beginners. An Interface contains declarations of events, indexers, methods and/or properties. Any class which implements the Interface should provide the concrete implementation for each Inter…
It was really hard time for me to get the understanding of Delegates in C#. I went through many websites and articles but I found them very clumsy. After going through those sites, I noted down the points in a easy way so here I am sharing that unde…
In this brief tutorial Pawel from AdRem Software explains how you can quickly find out which services are running on your network, or what are the IP addresses of servers responsible for each service. Software used is freeware NetCrunch Tools (https…
This is my first video review of Microsoft Bookings, I will be doing a part two with a bit more information, but wanted to get this out to you folks.

690 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question