Refactor Code

RefactorThis.Domain.Tests/InvoicePaymentProcessorTests.cs

@@ -43,7 +43,7 @@ public void ProcessPayment_Should_ReturnFailureMessage_When_NoPaymentNeeded( )
 				Payments = null
 			};
 
-			repo.Add( invoice );
+			repo.UpsertInvoice( invoice );
 
 			var paymentProcessor = new InvoiceService( repo );
 
@@ -71,7 +71,7 @@ public void ProcessPayment_Should_ReturnFailureMessage_When_InvoiceAlreadyFullyP
 					}
 				}
 			};
-			repo.Add( invoice );
+			repo.UpsertInvoice( invoice );
 
 			var paymentProcessor = new InvoiceService( repo );
 
@@ -98,7 +98,7 @@ public void ProcessPayment_Should_ReturnFailureMessage_When_PartialPaymentExists
 					}
 				}
 			};
-			repo.Add( invoice );
+			repo.UpsertInvoice( invoice );
 
 			var paymentProcessor = new InvoiceService( repo );
 
@@ -122,7 +122,7 @@ public void ProcessPayment_Should_ReturnFailureMessage_When_NoPartialPaymentExis
 				AmountPaid = 0,
 				Payments = new List<Payment>( )
 			};
-			repo.Add( invoice );
+			repo.UpsertInvoice( invoice );
 
 			var paymentProcessor = new InvoiceService( repo );
 
@@ -152,7 +152,7 @@ public void ProcessPayment_Should_ReturnFullyPaidMessage_When_PartialPaymentExis
 					}
 				}
 			};
-			repo.Add( invoice );
+			repo.UpsertInvoice( invoice );
 
 			var paymentProcessor = new InvoiceService( repo );
 
@@ -176,7 +176,7 @@ public void ProcessPayment_Should_ReturnFullyPaidMessage_When_NoPartialPaymentEx
 				AmountPaid = 0,
 				Payments = new List<Payment>( ) { new Payment( ) { Amount = 10 } }
 			};
-			repo.Add( invoice );
+			repo.UpsertInvoice( invoice );
 
 			var paymentProcessor = new InvoiceService( repo );
 
@@ -206,7 +206,7 @@ public void ProcessPayment_Should_ReturnPartiallyPaidMessage_When_PartialPayment
 					}
 				}
 			};
-			repo.Add( invoice );
+			repo.UpsertInvoice( invoice );
 
 			var paymentProcessor = new InvoiceService( repo );
 
@@ -230,7 +230,7 @@ public void ProcessPayment_Should_ReturnPartiallyPaidMessage_When_NoPartialPayme
 				AmountPaid = 0,
 				Payments = new List<Payment>( )
 			};
-			repo.Add( invoice );
+			repo.UpsertInvoice( invoice );
 
 			var paymentProcessor = new InvoiceService( repo );
 

RefactorThis.Domain/InvoiceService.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Linq;
+using System.Collections.Generic;
 using RefactorThis.Persistence;
 
 namespace RefactorThis.Domain
@@ -8,6 +9,8 @@ public class InvoiceService
 	{
 		private readonly InvoiceRepository _invoiceRepository;
 
+		private const decimal TAX_RATE = 0.14m;
+
 		public InvoiceService( InvoiceRepository invoiceRepository )
 		{
 			_invoiceRepository = invoiceRepository;
@@ -17,126 +20,68 @@ public string ProcessPayment( Payment payment )
 		{
 			var inv = _invoiceRepository.GetInvoice( payment.Reference );
 
-			var responseMessage = string.Empty;
-
-			if ( inv == null )
+			// Fail fast if invoice not found
+			if (inv == null)
 			{
 				throw new InvalidOperationException( "There is no invoice matching this payment" );
 			}
+
+			// Ensure payments collection is present so we can safely Add to it later.
+			inv.Payments = inv.Payments ?? new List<Payment>();
+
+			string responseMessage;
+
+			if ( inv.Amount == 0 )
+			{
+				if ( !inv.Payments.Any() )
+				{
+					responseMessage = "no payment needed";
+				}
+				else
+				{
+					throw new InvalidOperationException( "The invoice is in an invalid state, it has an amount of 0 and it has payments." );
+				}
+			}
 			else
 			{
-				if ( inv.Amount == 0 )
+				if ( inv.Payments.Any() )
 				{
-					if ( inv.Payments == null || !inv.Payments.Any( ) )
+					var paymentsSum = inv.Payments.Sum( x => x.Amount );
+					if ( paymentsSum != 0 && inv.Amount == paymentsSum )
+					{
+						responseMessage = "invoice was already fully paid";
+					}
+					else if ( paymentsSum != 0 && payment.Amount > ( inv.Amount - inv.AmountPaid ) )
 					{
-						responseMessage = "no payment needed";
+						responseMessage = "the payment is greater than the partial amount remaining";
 					}
 					else
 					{
-						throw new InvalidOperationException( "The invoice is in an invalid state, it has an amount of 0 and it has payments." );
+						// There are previous payments and this payment is valid to apply
+						bool isFinalPayment = ( inv.Amount - inv.AmountPaid ) == payment.Amount;
+						ApplyPayment(inv, payment, replaceAmountPaid: false, firstPayment: false);
+						responseMessage = isFinalPayment
+							? "final partial payment received, invoice is now fully paid"
+							: "another partial payment received, still not fully paid";
 					}
 				}
 				else
 				{
-					if ( inv.Payments != null && inv.Payments.Any( ) )
+					if ( payment.Amount > inv.Amount )
+					{
+						responseMessage = "the payment is greater than the invoice amount";
+					}
+					else if ( inv.Amount == payment.Amount )
 					{
-						if ( inv.Payments.Sum( x => x.Amount ) != 0 && inv.Amount == inv.Payments.Sum( x => x.Amount ) )
-						{
-							responseMessage = "invoice was already fully paid";
-						}
-						else if ( inv.Payments.Sum( x => x.Amount ) != 0 && payment.Amount > ( inv.Amount - inv.AmountPaid ) )
-						{
-							responseMessage = "the payment is greater than the partial amount remaining";
-						}
-						else
-						{
-							if ( ( inv.Amount - inv.AmountPaid ) == payment.Amount )
-							{
-								switch ( inv.Type )
-								{
-									case InvoiceType.Standard:
-										inv.AmountPaid += payment.Amount;
-										inv.Payments.Add( payment );
-										responseMessage = "final partial payment received, invoice is now fully paid";
-										break;
-									case InvoiceType.Commercial:
-										inv.AmountPaid += payment.Amount;
-										inv.TaxAmount += payment.Amount * 0.14m;
-										inv.Payments.Add( payment );
-										responseMessage = "final partial payment received, invoice is now fully paid";
-										break;
-									default:
-										throw new ArgumentOutOfRangeException( );
-								}
-
-							}
-							else
-							{
-								switch ( inv.Type )
-								{
-									case InvoiceType.Standard:
-										inv.AmountPaid += payment.Amount;
-										inv.Payments.Add( payment );
-										responseMessage = "another partial payment received, still not fully paid";
-										break;
-									case InvoiceType.Commercial:
-										inv.AmountPaid += payment.Amount;
-										inv.TaxAmount += payment.Amount * 0.14m;
-										inv.Payments.Add( payment );
-										responseMessage = "another partial payment received, still not fully paid";
-										break;
-									default:
-										throw new ArgumentOutOfRangeException( );
-								}
-							}
-						}
+						// First payment that exactly equals the invoice amount
+						ApplyPayment(inv, payment, replaceAmountPaid: true, firstPayment: true);
+						responseMessage = "invoice is now fully paid";
 					}
 					else
 					{
-						if ( payment.Amount > inv.Amount )
-						{
-							responseMessage = "the payment is greater than the invoice amount";
-						}
-						else if ( inv.Amount == payment.Amount )
-						{
-							switch ( inv.Type )
-							{
-								case InvoiceType.Standard:
-									inv.AmountPaid = payment.Amount;
-									inv.TaxAmount = payment.Amount * 0.14m;
-									inv.Payments.Add( payment );
-									responseMessage = "invoice is now fully paid";
-									break;
-								case InvoiceType.Commercial:
-									inv.AmountPaid = payment.Amount;
-									inv.TaxAmount = payment.Amount * 0.14m;
-									inv.Payments.Add( payment );
-									responseMessage = "invoice is now fully paid";
-									break;
-								default:
-									throw new ArgumentOutOfRangeException( );
-							}
-						}
-						else
-						{
-							switch ( inv.Type )
-							{
-								case InvoiceType.Standard:
-									inv.AmountPaid = payment.Amount;
-									inv.TaxAmount = payment.Amount * 0.14m;
-									inv.Payments.Add( payment );
-									responseMessage = "invoice is now partially paid";
-									break;
-								case InvoiceType.Commercial:
-									inv.AmountPaid = payment.Amount;
-									inv.TaxAmount = payment.Amount * 0.14m;
-									inv.Payments.Add( payment );
-									responseMessage = "invoice is now partially paid";
-									break;
-								default:
-									throw new ArgumentOutOfRangeException( );
-							}
-						}
+						// First payment and it's partial
+						ApplyPayment(inv, payment, replaceAmountPaid: true, firstPayment: true);
+						responseMessage = "invoice is now partially paid";
 					}
 				}
 			}
@@ -145,5 +90,33 @@ public string ProcessPayment( Payment payment )
 
 			return responseMessage;
 		}
+
+		private void ApplyPayment(Invoice inv, Payment payment, bool replaceAmountPaid, bool firstPayment)
+		{
+			if (replaceAmountPaid)
+			{
+				inv.AmountPaid = payment.Amount;
+			}
+			else
+			{
+				inv.AmountPaid += payment.Amount;
+			}
+
+			// Tax behavior mirrors original code: on the very first payment set TaxAmount = amount * rate;
+			// on subsequent payments only Commercial invoices accumulate additional tax.
+			if (firstPayment)
+			{
+				inv.TaxAmount = payment.Amount * TAX_RATE;
+			}
+			else
+			{
+				if (inv.Type == InvoiceType.Commercial)
+				{
+					inv.TaxAmount += payment.Amount * TAX_RATE;
+				}
+			}
+
+			inv.Payments.Add(payment);
+		}
 	}
 }

RefactorThis.Persistence/Invoice.cs

@@ -8,19 +8,23 @@ public class Invoice
 		public Invoice( InvoiceRepository repository )
 		{
 			_repository = repository;
+			// Ensure Payments is initialized to safely add Payments
+			Payments = new List<Payment>();
+			// Default to Standard to provide a sensible default
+			Type = InvoiceType.Standard;
 		}
 
 		public void Save( )
 		{
-			_repository.SaveInvoice( this );
+			_repository.UpsertInvoice( this );
 		}
 
 		public decimal Amount { get; set; }
 		public decimal AmountPaid { get; set; }
 		public decimal TaxAmount { get; set; }
 		public List<Payment> Payments { get; set; }
-
 		public InvoiceType Type { get; set; }
+		public string Reference { get; set; }
 	}
 
 	public enum InvoiceType

RefactorThis.Persistence/InvoiceRepository.cs

@@ -1,21 +1,48 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+
 namespace RefactorThis.Persistence {
 	public class InvoiceRepository
 	{
-		private Invoice _invoice;
+		private readonly List<Invoice> _invoices = new List<Invoice>();
 
-		public Invoice GetInvoice( string reference )
+		public Invoice GetInvoice(string reference)
 		{
-			return _invoice;
-		}
+			if (string.IsNullOrWhiteSpace(reference))
+			{
+				return _invoices.FirstOrDefault();
+			}
 
-		public void SaveInvoice( Invoice invoice )
-		{
-			//saves the invoice to the database
+			return _invoices.FirstOrDefault(i =>
+				string.Equals(i.Reference, reference, StringComparison.OrdinalIgnoreCase));
 		}
 
-		public void Add( Invoice invoice )
+		public void UpsertInvoice(Invoice invoice)
 		{
-			_invoice = invoice;
+			if (invoice == null) throw new ArgumentNullException(nameof(invoice));
+
+			Invoice existing = null;
+			if (!string.IsNullOrWhiteSpace(invoice.Reference))
+			{
+				existing = _invoices.FirstOrDefault(i =>
+					string.Equals(i.Reference, invoice.Reference, StringComparison.OrdinalIgnoreCase));
+			}
+
+			if (existing == null)
+			{
+				existing = _invoices.FirstOrDefault(i => ReferenceEquals(i, invoice));
+			}
+
+			if (existing != null)
+			{
+				var index = _invoices.IndexOf(existing);
+				_invoices[index] = invoice;
+			}
+			else
+			{
+				_invoices.Add(invoice);
+			}
 		}
 	}
 }