Method Logic Structuring (.NET/C#/SQL)

[calebb]

This is somewhat of an academic question, but one that's been on my mind for a few months now. Actually, perhaps it's not academic and there really is a better way to do this - Anyway, I'll try to spare you as many boring details as I can, but feel free to ask for more background details.

I (and a database contractor, who also frequents the [H]) built a large e-commerce system over the summer. The items we sell are listed on many different marketplaces via whatever API the marketplace has provided. An early design decision was to have the order-import application take all the raw order report data and put it directly into a "raw" table in our database.

Then, a separate database stored procedure parses the order-raw data into the correct location based on a "mapping" table. For example, Amazon.com has a order report header named "order-id" - this maps to "MarketplaceOrderID" in our database which would go in the "ORDERS" table.

A quirky detail for most of the marketplaces we sell on: If a customer orders more than one item (different product id / ISBN), the marketplace actually returns all their order details (name, address, email address, MarketplaceOrderID) once for each unique item purchased. The only thing that makes each line unique is the ISBN (ASIN) of the product purchased and something that is generally named "Order-Item-ID." If a customer orders a qty > 1 for a specific item, that is returned on a single line.

Ok, we're getting closer to my question.

I try to follow good software design practices (n-tier in this case) and keep my code as reusable as possible. I also prefer to not have a single method that has many nested if/switch statements to maximize readability/maintainability. In most cases I can keep it to 1 or 2 levels max. But not in this case (so far).

So the two things that I'm specifically having issues with:
1. Structuring this method so that is is reusable between all marketplaces. Specifically, each marketplace has a few additional pieces of data manipulation that must happen.
.a. Some marketplaces do not use a standard (importable) datetime field. (in this case, I need to detect it via regex, pass it to the method to clean it up, and format it to a valid .NET datetime variable)
.b. Or, on a different marketplace, they don't include a shipping method ~10% of the time - so I need to set it to "standard" in the application. (I prefer not to put that logic in the database)
.c. On yet another marketplace, they sometimes accidentally duplicate a line in the order report file - so I need to check to make sure this order-item-id was not just processed.
.d. I have ~5 other marketplace-specific exceptions - so rather than drop all the data variation handling into a single common method (which really becomes unmaintainable), I have many different variations of this method - one for each marketplace. (new problem - spaghetti-ish code that still feels overly complicated)

2. Additionally, this is by far the ugliest section of code in the entire project (which includes 20+ different applications)... and remember, I omitted the code for handling the data variations I mentioned above - it's even messier than this in real life...

Semantically, this is all that the method needs to do:
1. Parse the order report headers by whatever delimiter necessary (tab, comma, pipe are the possibilities).
2. Iterate through the report line-by-line
.a. Make sure the number of fields matches the number of fields in the header
.b. Split by delimiter into an array
.c. Determine if this row represents is a new order in our database, a previously inserted order in our database or an additional row (order-item) for an order I'm currently importing.
.d. Check for (various marketplace specific details)
.e. Perform database insert.


Here's some code:

String rawOrderData = this.GetMarketplaceOrderReport(wr);  //the webrequest

if (strResult == string.Empty)
{
	m_validate.SendDebugEmail("[TBv2][SEV2][Miner] Error getting report id " + intReportID.ToString(), "See Subject");
    return false;
}

// This method has its own error handling - throws differnet exceptions when errors encountered
String rawOrderData = this.GetMarketplaceOrderReport(wr);
String stringDelimiter = "\t"; //This is actually passed into this method
String[] arrayOrderDataRows = strResult.Split('\n');
String[] arrayHeadersDataRow = arrayOrderDataRows[0].Split('\t');
String stringLastOrderID = String.Empty;  //To know when to request new orderid
Int64 intIdOrder = 0;
Int16 intArrayOffsetForMarketplaceOrderID = 1; //This is passed into this method - self descriptive
Int16 intArrayOffsetForOrderItemQuantity = 10;  //This is the offset in arrayOrderDataRow for the qty of this item
bool boolNewOrder = true;  //Determines if this is a new order, or another line in an existing order
int intIdRow = 0;  //Internal counter to assist the store procedure to know when we're on a new line

//start at 1 to skip first (headers) row
for (int i = 1; i < arrayOrderDataRows.Length; i++)
{
	String[] arrayOrderDataRow = arrayDataRows[i].Split(stringDelimiter);
	if (arrayOrderDataRow.Length != arrayHeadersDataRow.Length) 
	{
		//Occasionally a marketplace sends a malformed order report
		//sends a debug email and logs error to database
	}
	else
	{
		//Only consider getting a new Order ID if this is a new order
		if (strLastOrderID != arrayOrderDataRow[intArrayOffsetForMarketplaceOrderID])
		{
			//I say /consider/ because sometimes marketplaces send us a order a second time
			//I always run a check against the database to see if the marketplace-order-id is new or
			//...previously imported.
			boolNewOrder = CheckIfNewMarketplaceOrderId(arrayOrderDataRow[1].ToString());  //returns FALSE if this order was previously imported
			if (boolNewOrder)
			{
				//Set to true so that this row in the order report will be imported.
				boolNewOrder = true;
				//Also get a new internal orderid from the ID Broker.
				intIdOrder = this.GetIdOrder();
			}
		}
		
		//This is not the /LastOrderID/ yet... next time through the loop it will be
		strLastOrderID = arrayOrderDataRow[1].ToString();

		if (boolNewOrder)  //If true, we know we must process the current arrayOrderDataRow
		{
			for (int j = 0; j < Convert.ToInt16(arrayOrderDataRow[intArrayOffsetForOrderItemQuantity]); j++) //iterator for buying multiple copies of the same item
			{
				intIdRow++; // You can ignore this - this is an internal counter that is used by the stored procedure to know we are on a new line
				for (int k = 0; k < arrayOrderDataRow.Length; k++)
				{
					if (arrayOrderDataRow[k] != string.Empty)  //No need to insert NULLS into the database
					{
						// I snipped out all the marketplace specific code here
						// ...it's a lot of code...

						try
						{
							this.AddRawOrderField(intIdOrder, intIdRow, arrHeaders[k], arrayOrderDataRow[k]);
						}
						catch(Exception ex)
						{
							throw new ExErrorAddingRawOrderField(m_intIdMarketplace, intIdOrder, ex)
						}

					 }
				 }
			 }
		}
	}
}

Can anyone offer some advice / critique on this?

Thanks in advance!

 

[Wiseguy2001]

The main problem is trying to parse data from so many different formats/ sources, just using conditionals is going to make your head spin. The way around this is to a 'table driven method' the book code complete 2nd ed. covers this very (buy it, it's a good book anyway). What this is essentially ,is using a generic parse method along with meta data about each format in separate files. Setting this up is a little bit more difficult initially but when you have so many formats, it will be MUCH simpler to add/ modify.

Another thing I would do is to sanitise and check for any errors in one place (the further you allow them, the more they get to bite and play on your mind). e.g. straight after the data has been parsed and loading into an object and check the delivery settings, duplicate lines.

 

[Tytalus]

This sounds like you'd want to use an Interface to properly describe the generic process, then write classes that implement the new interface which contain the non-generic logic to perform. As a VERY quick example, consider credit card transactions... if you had a very simple table of credit card transactiosn and you had the following fields:

1) transactionId - some uniquely incrementing field on a per-transaction basis
2) cctype - could be 'AMEX', or 'DISC', or 'VISA'; identifies the credit card type
3) ccinfo - the credit card digits
4) expdate - the credit card expiration date
5) transactionamount - the amount of said transaction

Then to process these you could write a series of classes that operate differently based on the type of credit card provided, and ensure they all operate using the same interface:

public interface ICreditCardTransaction
{
    Boolean PerformTransaction(String ccinfo, String expDate, String transactionAmount);
}

Then for each class, implement them using the interface:

public class AmExTransaction : ICreditCardTransaction
{
    public Boolean PerformTransaction(String ccinfo, String expDate, String transactionAmount)
    {
        //do stuff for AmEx
    }
}

Then in your main loop, access the table, strip out the relevant data then switch based on the cctype and instantiate the appropriate type. I won't provide an example of that here since I have stuff to do at work right now, but hopefully this gives you an idea of how to compartmentalize your data a bit more so it doesn't feel so unwieldly.

I hope this makes sense and applies!

 

[calebb]

Wiseguy2001:
Thanks for the advice! We happen to have a copy of that book you mentioned in stock so I'm taking a look at it now!

Tytalus:
I'm a huge fan of interfaces, abstract classes, inheritance, etc! However, I don't see how implementing an interface would help with organizing/maintaining the large number of conditionals that need to be evaluated in this particular problem -

 

[Tytalus]

Tytalus:
I'm a huge fan of interfaces, abstract classes, inheritance, etc! However, I don't see how implementing an interface would help with organizing/maintaining the large number of conditionals that need to be evaluated in this particular problem -

Well, in the end you'll still need a long case structure (or lots of if-elses) in order to process it as defined. Unless you can change your process to "pre-filter" the selections (provide tags parsed by some delimiter, perhaps?) the end result is that you have a single field with String data that needs to be analyzed in order to generate the proper workflow for the data provided.

Where interfacing comes in for this particular example is the ability to minimize the size of the case structure, and to reduce or eliminate the bulk of the processing in this case structure. If possible, you could even write a static method that would take the data read from a single row and take as your return value the interfaced object that was created. I do this fairly often for certain data types that we use in our current product--a String of well-formed XML data is passed around as a command, and I have a parser which efficiently processes the data in to a format I can handle, with a minimum of overall effort. From this object I can tell it to create certain other (interfaced) objects which I can then use to provide finer detail to the data provided. Everyone in the office is now used to using my "all-encompassing data morphing object" to perform a variety of tasks using just a few method calls to "drill down" to the data required to execute the command.

The problem you described can't be abstracted down or reduced in size any further. A really bad idea that my former boss used to do (which I have since removed from our process entirely) was to create classes named exactly the same as our delimiting field (in this case, it was the command type.) So if our command was "userlogon" we'd have a UserLogon class which was interfaced by our common data interface. We'd then use reflection to dynamically load the "UserLogon" data type and return the object as the interfaced type for additional processing. This process was wrong for at least two reasons, but I'm sure you can identify a LOT more reasons for why you don't want to dynamically instantiate based on a VARCHAR field in a database.

 

[Wiseguy2001]

So how are you getting on?

 

[calebb]

Cool thanks for the further explanation Tytalus!

I haven't had time to work on refactoring this yet. I think I'll have time this weekend or early next week.

Also, the copies of Code Complete we had were the 1994 edition (Something like 50 copies in stock)... but no v2. So I ordered a copy from one of our competitors via amazon.com (lol)

And... I just added our listings to another newer marketplace; Guess what? More conditionals. For this particular marketplace, the order results file does not include what the customer paid for shipping. (or the gross sale total) (or and accurate book cost for that matter).
1. We can list books for whatever price we want, but customers will never pay less than 1.99 for a book - so they pass us back whatever we had the book priced at. (new conditional - if using this marketplace and sale price is less than 1.99 and product type is book, set to 1.99. If product type is VHS/CD/DVD, set to 2.49.
2. Shipping - this one is a nightmare.
.a. Outer Conditional: 1st item or additional item (cheaper for 2+ items)
.b. Next conditional in: Product type (book/vhs/cd/dvd)
.c. Next conditional in: Stardard or Expedited
.d. Next conditional in: USA, Canada or Rest of World
Yes, that's right, 24 new conditionals.

It works just fine, but if another developer needed to update this, their eyes would glaze over for sure... and it makes me cringe looking at it. For this case, I considered storing each conditional as a field in a table - pass in the required conditions and out pops a shipping price.

The table driven method sounds very promising for situations like this - can't wait to dig in and try it.

 

[Wiseguy2001]

Yeah another 24 won't hurt, maybe throw in a few ternaries to make the code just a little more interesting, breaks within 'if' statements are nice also. Oh and goto's, goto's are cool!

You may have a job for life!

You gotta start breaking the routine down, still hat's off to you hanging in there.