I keep bouncing from “god function” to “tiny SRP functions” how do I break this cycle?

Question

This question is important for me in growing in my technical abilities. I find I swing from end-to-end, like a pendulum, in writing code that is simultaneously DRY yet readable & efficient. And I'm constantly doing this... which leads to too many re-factors and never a good, satisfied resolution.

The real example is an e-commerce cart, where you have two basic actions: add_item and remove_item. Both actions do the following things, adjust an item_counter used to calculate a total_price (which is shown in the view), add/remove a cart_item in the view.

Initially, I sought for the most streamlined code possible, so it ended up looking like:

$(".item").click(function() {
  type_of_operation = $(this).data("type") // can be add or remove
  item_id = $(this).data("id")
  doOperation(type_of_operation, item_id)
})

function doOperation(type_of_operation, item_id) {
  count = toggleItemCounter(type_of_operation)
  updatePrice(count)
  toggleCartItem(type_of_operation, item_id)
}

function toggleItemCounter(type_of_operation) {
  old_count = JSON.parse(sessionStorage.getItem("counter")
  if (type_of_operation == "add") {
    new_count = old_count + 1
  } else if (type_of_operation == "remove") {
    new_count = old_count - 1
  }
  return new_count
}

function updatePrice(count) {
  price = count * 10
  $("#total_price").text(price)
}

function toggleCartItem(type_of_operation, item_id) {
  if (type_of_operation == "add") {
    item = "<div class='item' data-type='remove' data-id='item_id'></div>"
    $("#item-list").append(item)
  } else if (type_of_operation == "remove") {
    $("#item-list").find("[data-id='"+item_id+"']").remove()
  }
}

As the code base grew, I realized that there are certain exceptional tasks for add/remove, and certain item_ids, so one function, for example, grew to something like this:

function doOperation(type_of_operation, item_id) {
  if (item_id = 999) {
    specialPriceUpdate()
  } else {
    if (type_of_operation == "modify") {
      adjustCartItem(item_id)
    } else {
      count = toggleItemCounter(type_of_operation)
      updatePrice(count)
      toggleCartItem(type_of_operation, item_id)
    }
  }
}

This growth continued like a tumor until doOperation became a bulky and very hard to read, with nests of conditionals and special returns and such. I realized my downfall was that I tried to make everything "generic" when I should have been more specific. So in a refactor, I rewrote the above to something like this:

$(".add_item").click(function() {
  item_id = $(this).data("id")
  count = addItemCounter()
  updatePrice(count)
  addCartItem(item_id)
}

$(".remove_item").click(function() {
  item_id = $(this).data("id")
  count = reduceItemCounter()
  updatePrice(count)
  removeCartItem(item_id)
}

$(".modify_item").click(function() {
  item_id = $(this).data("id")
  adjustCartItem(item_id)
}

$(".special_item") {
  specialPriceUpdate()
}

function addItemCounter() {
  old_count = JSON.parse(sessionStorage.getItem("counter")
  return old_count + 1
}

function reduceItemCounter() {
  old_count = JSON.parse(sessionStorage.getItem("counter")
  return old_count - 1
}

function updatePrice(count) {
  // same as previous
}

function addCartItem(item_id) {
  item = "<div class='item' data-type='remove' data-id='item_id'></div>"
  $("#item-list").append(item)
}

function removeCartItem(item_id) {
  $("#item-list").find("[data-id='"+item_id+"']").remove()
}

function adjustCartItem(item_id) {
  $("#item-list").find("[data-id='"+item_id+"']").append("some adjustment")
}

And then I thought, ok that looks more clear and readable. But then (because the real example has more functions), I was like... well wait a minute, you're generally always following the same pattern: adding or removing the counter, updating the price, then adding or removing the item, so maybe there should be a unified add and a unified remove function, so something like...

function addItem(item_id) {
  count = addItemCounter()
  updatePrice(count)
  addCartItem(item_id)
}

function removeItem(item_id) {
  count = reduceItemCounter()
  updatePrice(count)
  removeCartItem(item_id)
}

And knowing myself, once I'm at that point, I'll look at it and think, by golly addItem and removeItem are basically just the same, except for one's add and one's remove! Why don't I just collapse it into a single function with a bunch of toggles, and then some conditionals to allow for the exceptions? Yup, and then that will get me back to square 1...

See, unlike a pendulum, I don't ever stop swinging from side to side in reach some sort of good middle ground. Literally I could be going round and round in circles every time I sit to re-examine the code. My mind basically is like... "big god functions are more efficient... to... woah no way, need very distinct functions for everything... to... hmm it looks like these distinct functions could be a bit more dry, let's dry them... to... ok well I've dried them all the way back to a big god function again..."

I think part of it is that I'm not confident sticking to one solution, because there is so much context on what's "optimal". Part of it is that I'm self-taught, so I don't have guiding principles to really say, OK stop swinging, this is the appropriate rule of thumb/principle to follow.

Has this/does this happen to you? What do you do??!!

Answer 1

What you need is a new abstraction.

Simply pulling out common bits of functionality ends up creating a mess. You need to find a brand new way of looking at your problem that enables you to extract the common functionality without producing a mess.

That's easier said than done!

For this particular case, a better idea is to split the logic of updating the dom from the logic of updating the state. Something like this:

function updateApp(updater) {
    // fetch javascript object representing current state
    var currentState = JSON.parse(sessionStorage.getItem("appState"));
    // allow the passed in function to update that state
    updater(currentState);
    // save the state
    sessionStorage.setItem("appState", JSON.stringify(currentState));
    // update the user interface.
    // the vhtml function update the #item-list to correspond
    // to the html described by renderItems
    $('#item-list').vhtml(renderItems(state));
}

// this function returns a data structure that describes the html that should
// be rendered. 
function renderItems(state) {
    return h("div",
        state.items.map((item) => h("div.item", {data_id: item.id},
            "Price: " + item.price * item.qty)));
}

// when you want to modify the state, do it through the updateState
// function. Here we have the relatively straightforward task of
// just tweaking the data structure holding the state to include
// the extra item.
$(".add_item").click(function() {
  item_id = $(this).data("id")
  updateApp(function(state) {
      state.items.push({
          id: item_id
      });
  })
}

This new abstract changes the way you think about your code. Before, you had to worry about all the pieces of DOM that needed to be updated. Now, split the task into two:

1. For your event functions, just update the state object in response to the action
2. For the render function, just return the correct DOM nodes for the current state.

Now you simply don't need to write the code that figures out the correct changes to make to the DOM. That's handled for you. Hopefully, you can see that if you add more functions that change the dom you won't have to add a lot of complexity to handle all those cases. It will just work.

You may think it would be inefficient to simply rebuild the DOM each time for the render function. Yes, it would be. But there are a number of virtual dom libraries that make this efficient. It may or may not be worth adding to your project.

Whatever the merits to your particular case, this is the general solution to your problem: find a better way to divide your problem. The reason that it hasn't worked to build more generic functions for you is that you haven't found the ideal way to divide your task.

Answer 2

In what ways will the code vary in the future? Which code will be easiest to add to?

If you are going to have lots of per-item exceptions, then you don't want a full function for each case (priceUpdate, specialPriceUpdate, anotherUpdateFunction, ...). You'll want one function that can vary the computation according to parameters.

On the other hand, if you're going to be changing the way the items are counted, you might want an updateCount function that takes in a delta.

Alternatively, if you'll be adding lots of different adjustments (maybe color, size, style, etc.), then you might want to change the way those are represented.

I guess my answer is that it's okay to have bounded structural duplication. Adding and removing an item are meaningfully different, and I doubt you're going to be getting anything else that would use the same pattern. If you're making an abstraction and can't think of anywhere else it would be used, then stop.

Also, as a bit of concrete advice, having a getItemCount function to encapsulate JSON.parse(sessionStorage.getItem("counter")) would get rid of that duplication, and then addItem and removeItem can just use addition and subtraction. I can imagine lots of times you would want to get the item count (like in updatePrice).