1
\$\begingroup\$

I have this old code and I want it to be easier to understand. Originally, it loaded using an onload on the opening body tag, so I managed to change that to a window.onload, but now redundant code is getting elements by ID, since it first finds the form and after that the actual select box.

var localidades = new Array(
  new Array("Baja California","Tijuana","Rosarito","Ensenada","Tecate","Mexicali","San Felipe"),
  new Array("Sonora","San Luis Río Colorado","Sonoita","Sásabe","Nogales","Naco","Agua Prieta"),
  new Array("Chihuahua","El Berrendo","Puerto Palomas","San Jerónimo","Ciudad Juárez","El Porvenir","Ojinaga"),
  new Array("Coahuila","Ciudad Acuña","Piedras Negras"),
  new Array("Nuevo León","Colombia"),
  new Array("Tamaulipas","Nuevo Laredo","Reynosa","Río Bravo","Nuevo Progreso","Matamoros")
);
function seleccionLocal(selection){
  var cajon = document.getElementById("forma1").city;
  for(var x=0;x<localidades.length;x++){
    if(selection==localidades[x][0]){
      var cities = localidades[x];
      cajon.length = cities.length;
      for(var n=1;n<cities.length;n++){
        cajon[n] = new Option(cities[n],cities[n]);
      }
      break;
    }
  }
}
window.onload = function StartIt(){
  var cajon = document.getElementById("forma1").country;
  cajon.length = localidades.length+1;
  for(var x=0;x<localidades.length;x++){
    cajon[x+1] = new Option(localidades[x][0],localidades[x][0]);
  }
}
<form id="forma1" action="" onsubmit="" style="margin:auto;">
    <select name="country" onchange="seleccionLocal(this.options[this.selectedIndex].value)">
    <option value="-1" >Estado</option>
  </select>
  <select name="city">
    <option value="-1" >Ciudad</option>
  </select>
</form>

It's working, but I guess it can be lighter. What say You?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. This question is incomplete. To help reviewers give you better answers, 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. Questions should include a description of what the code does. At the moment, the question only describes some circumstances of the code, but not the code itself. \$\endgroup\$ – Zeta Nov 10 '18 at 8:27
1
\$\begingroup\$

Mixing English and Spanish in code is always awkward. I don't recommend it. If you must do it, though, try to be consistent, and avoid mixing in English identifiers like var cities.

The localidades array, in which the first string of each member array is special and represents a state, is weird. An associative array, with the state names as keys, would be better.

In modern practice, the HTML should be free of all JavaScript, and the event hooks are added from the JavaScript. addEventListener(), which allows the code to add multiple handlers, is preferable to window.onload = …, which only allows one handler.

The "Estado" and "Ciudad" placeholder options should be disabled to prevent them from being selected or submitted.

Below, I've defined a function, selectorDeEstadoYCiudad, which acts as a controller for the two drop-downs.

var localidades = {
  "Baja California": ["Tijuana","Rosarito","Ensenada","Tecate","Mexicali","San Felipe"],
  "Sonora": ["San Luis Río Colorado","Sonoita","Sásabe","Nogales","Naco","Agua Prieta"],
  "Chihuahua": ["El Berrendo","Puerto Palomas","San Jerónimo","Ciudad Juárez","El Porvenir","Ojinaga"],
  "Coahuila": ["Ciudad Acuña","Piedras Negras"],
  "Nuevo León": ["Colombia"],
  "Tamaulipas": ["Nuevo Laredo","Reynosa","Río Bravo","Nuevo Progreso","Matamoros"],
};

function selectorDeEstadoYCiudad(localidades, forma) {
  forma.state.addEventListener('change', function (e) {
    var cajon = forma.city;
    var ciudades = localidades[e.target.value];
    for (var i = 0; i < ciudades.length; i++){
      cajon[i+1] = new Option(ciudades[i], ciudades[i]);
    }
    cajon.length = ciudades.length + 1;
    cajon.value = cajon[0].value;
  });
  
  var cajon = forma.state;
  for (var estado in localidades) {
    cajon.appendChild(new Option(estado, estado));
  }
}

window.addEventListener('DOMContentLoaded', function() {
  selectorDeEstadoYCiudad(localidades, document.getElementById('forma1'));
});
<form id="forma1" action="">
  <select name="state">
    <option disabled>Estado</option>
  </select>
  <select name="city">
    <option disabled>Ciudad</option>
  </select>
</form>

\$\endgroup\$
  • \$\begingroup\$ Thank You for your kind advice, I humbly accept it. And Thank You for helping me with the code. Beautiful code. \$\endgroup\$ – Samuel Ramzan Nov 10 '18 at 10:57

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.