Solved

Refactor C# code

Posted on 2016-07-21
13
86 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
  • 7
  • 5
13 Comments
 
LVL 32

Accepted Solution

by:
it_saige earned 500 total points
Comment Utility
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
Comment Utility
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
Comment Utility
File Attached
0
 
LVL 32

Expert Comment

by:it_saige
Comment Utility
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
Comment Utility
0
 
LVL 32

Expert Comment

by:it_saige
Comment Utility
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
Free Trending Threat Insights Every Day

Enhance your security with threat intelligence from the web. Get trending threat insights on hackers, exploits, and suspicious IP addresses delivered to your inbox with our free Cyber Daily.

 

Author Comment

by:Member_2_7967608
Comment Utility
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 32

Assisted Solution

by:it_saige
it_saige earned 500 total points
Comment Utility
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
Comment Utility
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 32

Expert Comment

by:it_saige
Comment Utility
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
Comment Utility
Excellent answer provided.
0
 
LVL 32

Expert Comment

by:Stefan Hoffmann
Comment Utility
Even when it is already solved: Have you took a look at AutoMapper?
0
 

Author Comment

by:Member_2_7967608
Comment Utility
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

IT, Stop Being Called Into Every Meeting

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

Join & Write a Comment

Recently while returning home from work my wife (another .NET developer) was murmuring something. On further poking she said that she has been assigned a task where she has to serialize and deserialize objects and she is afraid of serialization. Wha…
Calculating holidays and working days is a function that is often needed yet it is not one found within the Framework. This article presents one approach to building a working-day calculator for use in .NET.
Internet Business Fax to Email Made Easy - With eFax Corporate (http://www.enterprise.efax.com), you'll receive a dedicated online fax number, which is used the same way as a typical analog fax number. You'll receive secure faxes in your email, fr…
This video shows how to remove a single email address from the Outlook 2010 Auto Suggestion memory. NOTE: For Outlook 2016 and 2013 perform the exact same steps. Open a new email: Click the New email button in Outlook. Start typing the address: …

772 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

Need Help in Real-Time?

Connect with top rated Experts

10 Experts available now in Live!

Get 1:1 Help Now