Click here to Skip to main content
15,917,005 members
Please Sign up or sign in to vote.
1.00/5 (1 vote)
Hello, I have a poorly written product price change function that takes into account many different conditions. You need to refactor, since the function itself is poorly readable. Tell me where and what is better to read or what practices to use to start refactoring. Here is the function for clarity (sorry in advance):
JavaScript
function changePrice($this = null) {
  if ($this == null) {
      var rel_price =  $('.all_colors_preview.active#rel_colors').find('.item.active .price').attr('data-price'),
          option_price = $('.all_colors_preview.active#option_colors').find('.item.active .option-price').val(),
          prefix = $('.all_colors_preview.active#option_colors').find('.item.active .option-price-prefix').val(),
          price_current = $('.price_wrapper .price_current'),
          option_sizes = $('.option_size.active'),
          quantity = Number($('#inner .quantity_val').text());
  } else {
    var rel_price =  $this.find('#rel_colors.active .item.active .price').attr('data-price'),
        option_price = $this.find('#option_colors.active .item.active .option-price').val(),
        prefix = $this.find('#option_colors.active .item.active .option-price-prefix').val(),
        price_current = $('.price_wrapper .price_current'),
        option_sizes = $this.find('.option_size.active'),
        quantity = Number($this.find('.quantity_val').text());
  }

  if (!quantity) {
    quantity = Number($('#inner .quantity_val').text());
  }

  var current = Number(price_current.attr('data-original-price'));  

  if (rel_price != '' && rel_price != undefined) {
    price_current.attr('data-update-price', rel_price);
    updated = Number(price_current.attr('data-update-price')) * quantity;
    updated = String(updated).replace(/(\d)(?=(\d\d\d)+([^\d]|$))/g, '$1 ')
    price_current.text(updated + ' ₽');
  } else if (option_price != '' && option_price != undefined && prefix != '') {
    if (prefix == '+') {
      update_price = current + Number(option_price);
    } else {
      update_price = current - Number(option_price);
    }

    price_current.attr('data-update-price', update_price);
    update_price = quantity * price_current.attr('data-update-price');
    price_current.text(String(update_price).replace(/(\d)(?=(\d\d\d)+([^\d]|$))/g, '$1 ') + ' ₽');
  } else if (option_price == '') {
    price_current.attr('data-update-price', current);
    current *= quantity;
    price_current.text(String(current).replace(/(\d)(?=(\d\d\d)+([^\d]|$))/g, '$1 ') + ' ₽');
  } else {
    return false;
  }

  if (option_sizes && option_sizes.length) {
    if (price_current.attr('data-update-price')) {
      current = Number(price_current.attr('data-update-price')); 
    }
    var option_prefix = option_sizes.find('input[type="hidden"]').attr('data-prefix'),
        option_size_price = option_sizes.find('input[type="hidden"]').attr('data-price');
    if (option_prefix == '+') {
      update_price = current + Number(option_size_price);
    } else {
      update_price = current - Number(option_size_price);
    }

    price_current.attr('data-update-price', update_price);
    update_price = quantity * price_current.attr('data-update-price');
    price_current.text(String(update_price).replace(/(\d)(?=(\d\d\d)+([^\d]|$))/g, '$1 ') + ' ₽');
  }


What I have tried:

Haven't tried anything yet, don't know where to start
Posted
Updated 8-Dec-22 2:31am
v2
Comments
Member 15627495 8-Dec-22 5:23am    
if you have few code pattern doing same thing, you can create some functions ( ex : get_value(field) return the value;//

you are chaining a lot of if else if else ... maybe a 'switch' 'case...' could replace it for a code easy to read, 'light code' will appears. //
like a law , and with the aim to have 'clean code' , if you use twice or more some code, a function could be written instead.
Richard Deeming 8-Dec-22 8:59am    
The .all_colors_preview.active#rel_colors selector looks suspicious to me.

Are you trying to use the same ID on multiple elements in your document? That won't work. IDs within a document have to be unique.

If you're not, then the two selectors are identical: they both find the single element in the document with the ID rel_colors.

1 solution

A. You are using many Magic String Values. These can make readability and maintainability difficult. You could move them into constants.

B. Keep your code DRY - Don't Repeat yourself. If you have repetitive calls, write them as a method with parameters and a meaningful name.

C. Your function is performing 3 groups of tasks:
1. Getting values
2. Calculating values
3. updating display
Each of these could be broken into single responsibilities. ('S' in the SOLID principle)

Once you apply the above 3 methodologies, your code will become more readable.
 
Share this answer
 
Comments
HarrySto 12-Dec-22 9:43am    
Tell me, if you need to make the code unique, are there any automatic services for this? Apprefactoring was advised to me, have you heard something about it?
Graeme_Grant 12-Dec-22 10:39am    
I did a quick Google Search for you and there are plenty of tools to choose from: javascript refactoring tools - Google Search[^]

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900