3
\$\begingroup\$

I'm having a problem figuring out how I can make this particular part of my code DRY. Currently, the code is repeating itself but with minor changes in each case of a switch.

SearchParam is a string that the user has searched for in the application that this code is from. The function creates a new li element with searchParam as the text. This value is then added to localStorage as 'search1'. Dependent on how many li elements there are already, the function uses a switch to add the new li element to the beginning of the list. If there are already 3 items, then the last item in the list will be deleted and removed.

function searchHistory(searchParam) {
  var liElements = $('#history').children();
  var recentSearch = liElements.first();

  var newItem = `
     <li>
        <i class="fa fa-long-arrow-right icons"></i>
        <p class="list-title">${searchParam}</p>
     </li>
  `;

  switch($('#history').children().length) {
     case 0:
        $('#history').append(newItem);
        localStorage.setItem('search1', searchParam);
        break;

     case 1:
        var secondSearch = liElements.children()[1].innerHTML;

        recentSearch.before(newItem);
        localStorage.setItem('search1', searchParam);
        localStorage.setItem('search2', secondSearch);
        break;

     case 2:
        var secondSearch = liElements.children()[1].innerHTML;
        var thirdSearch = liElements.children()[3].innerHTML;

        recentSearch.before(newItem);
        localStorage.setItem('search1', searchParam);
        localStorage.setItem('search2', secondSearch);
        localStorage.setItem('search3', thirdSearch);
        break;

     default:
        var oldSearch = liElements.last();
        var secondSearch = liElements.children()[1].innerHTML;
        var thirdSearch = liElements.children()[3].innerHTML;

        localStorage.setItem('search1', searchParam);
        localStorage.setItem('search2', secondSearch);
        localStorage.setItem('search3', thirdSearch);

        recentSearch.before(newItem);
        oldSearch.remove();
  }
};
\$\endgroup\$
3
\$\begingroup\$

So if I understand the code correctly:

  • You're keeping track of search.
  • You're adding the search items to local storage.
  • You're keeping the history to a max of 3 items.
  • You're displaying them in the UI.

I would suggest to leave out the UI logic out of the storage logic. The storage logic should only deal with storage and retrieval from local storage. UI updates should be separate.

Next is to not depend on the UI for maintaining state. That's the job of your data. The UI should merely be a representation of the data. That means, you shouldn't be depending on innerHTML to keep that history.

The search history is simply an array which you push in stuff, and shift out stuff when it goes beyond 3. You do not need an item in the storage for each. Since localStorage only keeps strings, you'll need to use JSON.stringify and JSON.parse to serialize and deserialize the data, respectively.

I would split the functions, one for updating the stored data, and the other to update the UI based on the existing data.

function searchHistory(searchParam) {
  const maxHistoryLength = 3;
  const history = JSON.parse(localStorage.getItem('searchHistory') || '[]');
  const isHistoryMaxed = history.length === maxHistoryLength;
  const workingHistory = isHistoryMaxed ? history.slice(1) : history;
  const updatedHistory = workingHistory.concat(searchParam);

  localStorage.setItem('searchHistory', JSON.stringify(updatedHistory));
}

function updateSearchHistoryUi(){
  const history = JSON.parse(localStorage.getItem('searchHistory') || '[]');

  $('#history').empty().append(history.map(v => `
    <li>
      <i class="fa fa-long-arrow-right icons"></i>
      <p class="list-title">${v}</p>
    </li>
  `).join(''));
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This code is better. However, I suspect that it might break more easily if the browser has local storage disabled. \$\endgroup\$ – 200_success Apr 20 '17 at 14:13
  • \$\begingroup\$ @200_success could you please elaborate why this code might break more easily. The original code uses the localStorage too and there's no any kind of guarding there. So, IMO both code snippets are in the same boat. \$\endgroup\$ – Igor Soloydenko Apr 20 '17 at 15:24
  • \$\begingroup\$ @IgorSoloydenko The original code writes to localStorage, but uses the DOM as the main source of data. In contrast, Joseph's solution relies on localStorage for the model. \$\endgroup\$ – 200_success Apr 20 '17 at 16:43

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.