3

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??!!

  • 2
    I'm long overdue for a follow-up post on this, but, you need to apply the POAP. – svidgen Jul 27 '17 at 0:22
  • fair, in this case i suppose the question is as much what is/should be more optimal... – james Jul 27 '17 at 0:41
  • please don't cross-post: stackoverflow.com/questions/45339080/… "Cross-posting is frowned upon as it leads to fragmented answers splattered all over the network..." – gnat Jul 27 '17 at 5:01
  • Whilst not a PHP expert, is there anything stopping you re-factoring towards object oriented code? A Cart class would naturally have add and remove methods and the state necessary to track its contents. Better composed code would help here. – Stewart Ritchie Jul 27 '17 at 7:39
  • Refactoring to improve one's code is a normal part of software engineering, and your refactorings seem quite reasonable, so I'm sort of wondering what the problem is here, if any. – Robert Harvey Jul 27 '17 at 18:20
5

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.

  • 1
    +1 I agree completely. His "SRP functions" are not single responsibility. He needs proper separation between application logic and UI state. – Samuel Jul 27 '17 at 2:13
  • Look up flow oriented programming – l--''''''---------'''''''''''' Jul 27 '17 at 4:55
  • this is very helpful! to be clear, in your suggested solution, it should be $(".add_item").click(function() { ... updateApp, not updateState right? Sorry I'm not trying to be nitpicky, it's just late and I want to be sure I'm reading this correctly. I like that first part of the solution very much! The second part in the suggested solution would require the DOM to be completely rebuilt each time updateApp is called, correct? If so that probably wouldn't work. Actually I have that now, and on iPads it's monstrously slow (the real code has more changes involved) – james Jul 27 '17 at 5:50
  • @james, yes it was supposed to be updateApp. So, yes, rebuilding the whole DOM would be unworkably slow. But google "virtual dom" for a number of libraries that provide an optimization to make it fast enough for practical use. – Winston Ewert Jul 27 '17 at 6:54
1

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).

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.