FIFO inventory allocation method

Question

I am an owner of a small industrial business startup and I am learning to write software in order to efficiently keep track of our inventory and finances, as our needs have outgrown the capabilities of Excel.

I was working on our perpetual FIFO inventory allocation method and I've inadvertently created a monster. This function serves to identify products listed on an invoice and determine what raw materials were included in each product at what quantity, at which point it sets up connecting records that hold data corresponding to the particular shipments that these raw materials were sourced from.

Furthermore, it must check to see if prior records have been created which would now be obsolete and remove them if necessary, so that reallocation can take place properly. It is set up to run whenever a new sale is entered into the system, so that it keeps track of everything all in real time.

It took me a long time to get it working correctly and now I am finding it overwhelming to look at and maintain, so I thought I ought to refactor it. However, my inexperience comes into play at this point as I am not sure how to go about this since it is a tangled mess of for, if and while loops.

Does anyone have any advice?

def remove_stale_SaleSource_records(list):
 for entry in list:
 shipment_record = entry.sale_tag
 entry.delete()
 shipment_record.sold_out = False
 shipment_record.save()
@receiver(post_save, sender=SalesRecord)
def spawn_SaleSource_record(sender, update_fields, created, instance, **kwargs):
 instance.sale_date = instance.invoice.sale_date
 # Check eligibility based on fields saved.
 if created or 'sale_item' in update_fields or 'sales_qty' in update_fields:
 # Remove existing SalesSource objects associated with this SalesRecord
 if not created and update_fields is 'sales_item' or 'sales_qty':
 remove_stale_SaleSource_records(SaleSource.objects.filter(sale=instance))
 # Set signaling SalesRecord as the first to be distributed.
 sales_series = []
 if instance not in sales_series:
 sales_series.append(instance)
 # Check for SalesRecord objects with an equal or more recent date than the object that was edited or created.
 for variety in instance.varieties_sold.items():
 print("")
 print(variety)
 possible_future_sales = SalesRecord.objects.filter(sale_date__gte=instance.sale_date).exclude(id=instance.id)
 if possible_future_sales:
 actual_future_sales = []
 # Single out items by timestamp
 for sale_item in possible_future_sales:
 print("--Sale DR: %s | Own DR %s" % (sale_item.date_rank, instance.date_rank))
 if variety[0] in sale_item.varieties_sold and sale_item.date_rank > instance.date_rank:
 actual_future_sales.append(sale_item)
 print("--Found %s future items" % (len(actual_future_sales)))
 # Add the Sales Record to the distribution list, but prevent it from being included twice.
 for obj in actual_future_sales:
 if obj not in sales_series:
 sales_series.append(obj)
 print("Adding %s to Sales Series" % (obj))
 remove_stale_SaleSource_records(SaleSource.objects.filter(sale=obj))
 print("Sales Series: %s" % str(sales_series))
 # For each record, identify each variety and allocate SalesSource records for each in order of item availability
 for sales_record in sales_series:
 for variety2 in sales_record.varieties_sold.items():
 print("bucket = %s" % (variety2[1]))
 bucket = variety2[1]
 while bucket > 0:
 x = Purchase.objects.filter(product_name=variety2[0], sold_out=False).first()
 if x:
 qty_available = x.current_turnout - x.qty_sold
 print("Tag %s has %s available" % (x.tag, qty_available))
 print("(Tag %s current turnout: %s)" % (x.tag, x.current_turnout))
 print("(Tag %s qty_sold: %s)" % (x.tag, x.qty_sold))
 qty_applied = 0
 if qty_available <= 0:
 x.sold_out = True
 x.save(update_fields=['sold_out'])
 else:
 if bucket > qty_available:
 qty_applied += qty_available
 x.sold_out = True
 x.save(update_fields=['sold_out'])
 else:
 qty_applied += Decimal(bucket)
 bucket -= Decimal(qty_applied)
 print("%s applied to %s, %s left in bucket" % (qty_applied,x.tag,bucket))
 qty_remaining = Decimal(qty_available) - Decimal(qty_applied)
 product_cost_inr = qty_applied * x.adjusted_cost
 SaleSource.objects.get_or_create(
 sale=sales_record,
 sale_tag=x,
 qty_applied=qty_applied,
 qty_remaining=qty_remaining,
 product_cost_inr=product_cost_inr,
 )
 else:
 print(" WARNING: BREAK DUE TO INSUFFICIENT PRODUCT IN STOCK")
 break

Answer 1

Picking the right tool

First of all, it's not immediately clear what the spawn_SaleSource_record() function does. There's no docstring, nor is there a description of what the parameters mean. (In particular, kwargs is always mysterious. Fortunately, in this case, kwargs isn't used at all.) It's also not obvious what the output of the function is. It appears that it has the side-effect of modifying and creating some Purchase and SaleSource records, and also prints out some debugging messages.

I'm not sure exactly what all this code does, but I can say that I strongly suspect that all of it could be done in a couple of SQL queries, and it would probably be faster and easier to understand. This is the kind of data manipulation that relational databases excel at.

The code

I'll make a few observations about the code anyway.

---

# Check eligibility based on fields saved.
if created or 'sale_item' in update_fields or 'sales_qty' in update_fields:
 ...

If you inverted the condition, then you could reduce the indentation level of the rest of the function by one.

# Check eligibility based on fields saved.
if not created and 'sale_item' not in update_fields and 'sales_qty' not in update_fields:
 return
...

---

if not created and update_fields is 'sales_item' or 'sales_qty':

That condition is almost certainly not what you meant. It reads nicely in English, but means something else altogether in Python. Based on operator precedence, it actually means...

if ((not created) and (update_fields is 'sales_item')) or 'sales_qty':

Since 'sales_qty' is a non-empty string, it's always True. Also, I get the impression that update_fields is supposed to be a list, so update_fields is 'sales_item' will probably never be the case. The net effect is that the whole condition effectively means...

if not created:

---

# Set signaling SalesRecord as the first to be distributed.
sales_series = []
if instance not in sales_series:
 sales_series.append(instance)

Since sales_series is a newly created empty list, I'm sure that instance is not going to be in the list. You could just write

sales_series = [instance]

---

# Check for SalesRecord objects with an equal or more recent date than the object that was edited or created.
for variety in instance.varieties_sold.items():
 print("")
 print(variety)
 possible_future_sales = SalesRecord.objects.filter(sale_date__gte=instance.sale_date).exclude(id=instance.id)
 if possible_future_sales:
 actual_future_sales = []
 # Single out items by timestamp
 for sale_item in possible_future_sales:
 print("--Sale DR: %s | Own DR %s" % (sale_item.date_rank, instance.date_rank))
 if variety[0] in sale_item.varieties_sold and sale_item.date_rank > instance.date_rank:
 actual_future_sales.append(sale_item)
 print("--Found %s future items" % (len(actual_future_sales)))
 # Add the Sales Record to the distribution list, but prevent it from being included twice.
 for obj in actual_future_sales:
 if obj not in sales_series:
 sales_series.append(obj)
 print("Adding %s to Sales Series" % (obj))
 remove_stale_SaleSource_records(SaleSource.objects.filter(sale=obj))

The if possible_future_sales condition can be eliminated. If it's an empty list, then actual_future_sales would also be empty. Eliminating branches makes your code easier to understand.

I'm skeptical that this block is good code, because possible_future_sales = SalesRecord.objects.filter(...) is a query that runs in a loop, but it doesn't depend on the variety loop variable, so I would expect it to produce the same result every time.

---

So, that's about half of the code in the function, and I'm going to end my review here. I don't think that I've understood any of the code up to this point. However, as you can see, I've applied a series of logical deductions to simplify the code and revealed some issues. I hope that you can continue applying similar techniques to the rest of your code.

• Adam Starrh: possible_future_sales is retrieving all potential matches, which are then compared to thevariety to determine if it is an actual match or not. It indeed does produce some repeat data, when varieties repeat, but the function is designed to only allow a match to be included once in the final series that will be evaluated.
• Adam Starrh: Also, thanks for catching the poorly written if statement. I had updated the one above, but forgot to update/test the functionality of that inner one. This would have made for some serious headaches. I will study your first point as well. This turned out to be really helpful, thank you!