4
\$\begingroup\$

I wrote a selector for several countries. This selector allows the user to choose a country, then a region and then a city. After the user chooses a city, it displays the selected object's country code, region code and area code (in terms of the VK API social network).

The selectors use the jquery ui autocomplete plugin.

HTML:

<div class="outer" id="countryOuter">
    <input id="country" placeholder="Страна">
</div>  

<div class="outer" id="regionOuter">
    <input id="region" placeholder="Регион">
</div>

<div class="outer" id="cityOuter">
    <input id="city" placeholder="Город">
</div>

JS:

$( document ).ready(function() {
    getCountries();

    var countriesArr = [],
            countriesObj = {};
    var regionsArr = [],
            regionsObj = {};    
    var citiesArr = [],
            citiesObj = {};                 

    $('#region').attr('disabled', 'disabled');
    $('#city').attr('disabled', 'disabled');

    function getCountries() {
        $.ajax({
        url: "http://api.vk.com/method/database.getCountries?v=5.69",
        dataType: "jsonp",
        success: function( data ) {
                var countriesRaw = data['response']['items'];
              console.log( countriesRaw );

              countriesArr = countriesRaw.map(function(country) {
                return country.title;
              });
              console.log(countriesArr);

              countriesRaw.forEach(function(country) {
                countriesObj[country.title] = country.id;
              });
              console.log(countriesObj);

                document.getElementById('countryOuter').style.display = 'block';

                $( "#country" ).autocomplete({
                    delay: 0.5,
                    source: countriesArr,
                    select: function(event, ui) {
                        $('#country').attr('disabled', 'disabled');
                        $('#region').removeAttr('disabled');                    
              console.log(event, countriesObj[ui.item.label]);
              getRegions(countriesObj[ui.item.label]);
              }
                }); 
        }           
        });
    };

    function getRegions(countryId) {
        $.ajax({
        url: "http://api.vk.com/method/database.getRegions?v=5.69&country_id=" + countryId,
        dataType: "jsonp",
        success: function( data ) {
                var regionsRaw = data['response']['items'];
              console.log( regionsRaw );

              regionsArr = regionsRaw.map(function(region) {
                return region.title;
              });
              console.log(regionsArr);

              regionsRaw.forEach(function(region) {
                regionsObj[region.title] = region.id;
              });
              console.log(regionsObj);

              console.log('____countryId', countryId);

                $( "#region" ).autocomplete({
                    delay: 0.5,
                    source: regionsArr,
                    select: function(event, ui) {
                        $('#region').attr('disabled', 'disabled');
                        $('#city').removeAttr('disabled');                  
              console.log(event, ui);
              getCitiesOut(countryId, regionsObj[ui.item.label]);
              }
                });     
            }               
      })    
    };

    function getCitiesOut(countryId, regionId) {
        console.log('countryId, regionId', countryId, regionId);
        getCities();

        function getCities( offset, limit ) {
        if( ! limit || limit > 1000 ) limit = 1000; //число запрашиваемых городов, максимально 1000, по умолчанию 1000
        if( ! offset ) offset = 0; //по умолчанию 0

        $.ajax({
            url: "http://api.vk.com/method/database.getCities?v=5.69&need_all=0&count="+limit+"&country_id=" + countryId +"&region_id=" + regionId +"&offset="+(offset*limit),
            dataType: "jsonp",
            success: function( data ) {
            console.log('data', data);
            var allCities = data.response.count;
            console.log('allCities', allCities);
            var citiesRaw = data['response']['items'];
            //с помощью concat можно объеденить 2 массива.
            citiesArr = citiesArr.concat(citiesRaw.map(function(city) {
              return city.title;
            }));
            //если в текущем ответе не все города то делаем ещё запрос с новым offset
            if( allCities > offset*limit + limit)
               getCities(offset+1, limit);   

            console.log('citiesArr', citiesArr);   

                  citiesRaw.forEach(function(cities) {
                    citiesObj[cities.title] = cities.id;
                  });           

                    $( "#city" ).autocomplete({
                        delay: 0.5,
                        source: citiesArr,
                        select: function(event, ui) {
                            $('#city').attr('disabled', 'disabled');
                  console.log(event, ui);
                  alert('Код страны' + countryId + ', Код региона' + regionId + ', Код города' + citiesObj[ui.item.label]);
                  }
                    });                  
            }           
          });
        };  
    }
});

Unfortunately jsfiddle and similar services do not support JSONP-requests which are used in my code, but this Plunker example demonstrates the code.

\$\endgroup\$
3
\$\begingroup\$
  1. There is too much nesting, which, for me, makes the logic hard to read. A good rule of thumb is to aim for two levels of nesting, by using the Extract Method refactoring.

  2. When the user clicks on the input, nothing happens. When the user starts typing a name that isn't in the list, sometimes the list pops up (if the initial matches) and sometimes they just type (if there is no matching string). I expect many users will be confused, and they might feel more comfortable if you re-used an existing paradigm.

  3. Your three functions contain a lot of identical code. This means that whenever you want to change the user experience in all three fields, you will need to change it in three places. Yet if you want the experience to be consistent, you probably want to change it in only one place. I suggest extracting the common functionality into a shared function.

\$\endgroup\$
2
\$\begingroup\$

dcorking has good points in his/her answer - especially about the redundant code (i.e. point #3). Below is my suggestion to improve the code, in addition to his/her advice.

Cache DOM Lookups

Like I mentioned in this other answer to one of your recent questions, it would be wise to store the DOM element references in variables when the DOM is ready, then use those whenever using the DOM elements instead of querying the DOM each time. While this code is quite small and only looks up DOM elements by ID 7 times, it is a good practice to get in the habit of - especially for working on pages where many DOM references will be needed by the Javascript code.

For example, the code could be updated like below:

$( document ).ready(function() {
    //could use const instead of var for these, since they shouldn't be re-assigned
    var regionInput = $('#region');
    var cityInput = $('#city');

    //later on, reference those variables instead of querying each time they are needed
    regionInput.attr('disabled', 'disabled');
    cityInput.attr('disabled', 'disabled');

Note the comment about using const- it could be used in place of var unless Browser compatibility is an issue.

\$\endgroup\$
  • 1
    \$\begingroup\$ IIUC the ready callback isn't needed on modern browsers. You can look up DOM elements at the top level, and the browser does the right thing. \$\endgroup\$ – dcorking Nov 17 '17 at 8:46
  • 1
    \$\begingroup\$ Good point -I've been wondering about that, and after reading posts like this and this believe that when we put our script tags near the end of the <body> tag it likely won't be an issue... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Nov 17 '17 at 17:02

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.