Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am new in JS, I am writing some extra filters on the Interface. So extra filter needs to add or update the parameter in the URL.

Option 1. No parameter in existing URL:

url = https://custom.net/

We want to add new parameter in the URL where key issector and value is IndependentConsultingDoctors

Output will be: https://custom.net/?sector=IndependentConsultingDoctors

Option 2. Update value in parameter of existing URL

URL = https://custom.net/?filter=title&sector=1234&sortby=1&page=1

We want to update parameter in the URL where key issector and value is IndependentConsultingDoctors

Output will be: URL = https://custom.net/?filter=title&sortby=1&page=1&sector=IndependentConsultingDoctors

Option 3. Delete and add parameter:

URL = https://custom.net/?filter=title&reportype=1234&sortby=1&page=1

We want to add new parameter in the URL where key issector and value is IndependentConsultingDoctors and delete reportype and inspectiontype parameters.

Output will be: URL = https://custom.net/?filter=title&sortby=1&page=1&sector=IndependentConsultingDoctors

The following is my code:

$(this).find('.btn').click(function(){  
    //var pathname = window.location.href;
    var pathname = "https://abc.com/?filter_by=title&filter_condition=&sector=1234&reportype=3455&sortby=1&page=1&itemperpage=10&input_list=&current_selected_option=2&masterfilter=%26itemperpage%3D10%26filter_by%3DAll%26filter_condition%3D%26sortby%3D1%26from_date%3D%26to_date%3D&from_date=&to_date="
    // Key and Value which need to add in paramaetrs  
    var key = "sector"
    var value = "Acutes";
    // Delete key from the parameters
    var delete_key = ["reportype", "inspectiontype"];

    // Split URL BASE and Get Parameters
    split_href = pathname.split("?");

    //No parameter present   
    if (split_href.length==1) {
         window.location.href = split_href[0]+'?'+key+'='+value;
    }else{
        // Create dictionary of parameter
        var para_dict = {};
        var para_list = split_href[1].split("&");
        for (ii in para_list){
            var tmp = para_list[ii].split("=");
            para_dict[tmp[0]] = tmp[1];
        }
        // Add target kay and Value
        para_dict[key] = value;

        // Delete key from the dinctionary
        for (ii in delete_key){
            delete para_dict[delete_key[ii]];       
        }
        //Create New Parameter string
        var new_para = "?";
        for (ii in para_dict){
            new_para = new_para+ii+"="+para_dict[ii]+"&";   
        }
        console.log(split_href[0]+new_para);
    }

});

Can you review my code and give me a more optimized solution?

share|improve this question
    
Welcome to Code Review! To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question –  Quill Jun 17 at 12:34
    
@Quill: Updated example in the question. Plz review and if need more information then ask again. –  Vivek Sable Jun 17 at 13:09

2 Answers 2

up vote 1 down vote accepted

Your code looks good, but a few typographical things I can see are:


Whitespace:

if (split_href.length==1) {
window.location.href = split_href[0]+'?'+key+'='+value;
}else{
new_para = new_para+ii+"="+para_dict[ii]+"&";
console.log(split_href[0]+new_para);

You need to include whitespace around your variables when you join them.

//Create New Parameter string
//No parameter present
//var pathname = window.location.href;   

should have whitespace between the // and the content.


In for (ii in para_list) and for (ii in delete_key), you should use i instead of ii.


I don't know whether this is controllable by you, but "reportype" is misspelt.

// Add target kay and Value
// Delete key from the dinctionary

are misspelt also.


Using para everywhere is bad practice. Use parameter instead, it's much clearer.

share|improve this answer
    
Thank You. I will update and again pass for review. –  Vivek Sable Jun 17 at 16:46

It's good to follow the single responsibility principle. The current method does many things that would make sense to extract to smaller helper functions, for example:

  • convert query string to dictionary
  • concert dictionary to query string
  • delete list of keys from dictionary
  • ... and so on

What's the point? Next time you need something similar (and most certainly you will), you will be able to reuse the common elements easier.

And yeah, as @Quill said, the names are horrible. Some ideas:

  • params instead of para_dict
  • index instead of ii
  • key_value instead of tmp
  • url instead of pathname
share|improve this answer
    
yes, Correct. I will update code tomorrow and again pass for review.Thank You. –  Vivek Sable Jun 17 at 16:47

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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