5

Soy aprendiz y quisiera optimizar este código, en especial no se si sea correcto llamar tantas variables. ¿Hay alguna forma más rápida o eficaz de hacerlo?

<script>
    window.onload = function(){
        var envia = document.getElementById(continuar);
        onclick.envia = function (){
            var tarjeta = tarjeta1.value; 
            var tarjeta2 = tarjeta2.value; 
            var tarjeta3 = tarjeta3.value;
            var paYpal = paypal.value; 
            if (( tarjeta=="") || (tarjeta2=="") || (tarjeta3=="")){
                alert ( "Tarjetas vacías, ¿desea pagar con Paypal?")
            }else if (paYpal>=10){
                alert("Paypal erróneo, verifícalo")
            }else {
                alert("Para pagar en efectivo ponte en contácto con 0999888723")
                window.location.href = "verifica.html"
            }
        }
    }
</script>
8
  • 1
    Por que editaste el título??, estaba mejor de la manera anterior, yo identé el código para hacerlo un poco más legible, no era con respecto al titulo de la pregunta. el 13 sep. 2016 a las 17:40
  • 1
    ¿Identar el código es ordenarlo ?. Al momento aún no utilizo jquery estoy aprendiendo javascript primero el próximo capítulo empiezo con jquery.
    – Fabricio
    el 13 sep. 2016 a las 18:00
  • 1
    Entiendo que el "continuar" del getElementById debería ir entre comillas, a no ser que sea una variable global... Aparte de esto, hay poco que optimizar. el 13 sep. 2016 a las 18:21
  • 2
    como recién andas empezando con JavaScript, mira este enlace: dorey.github.io/JavaScript-Equality-Table habla sobre la diferencia de == y ===, != y !== saludos
    – fredyfx
    el 13 sep. 2016 a las 18:45
  • 1
    ¿Qué es onclick.envia? ¿No debería ser envia.onclick, o algo similar?
    – lilezek
    el 23 jul. 2017 a las 1:47

5 respuestas 5

8
+100

No es necesario definir variables, si no las vuelves a usar.

<script>
window.onload = function(){
    var envia = document.getElementById(continuar);
    onclick.envia = function (){
        if (( tarjeta1.value=="") || (tarjeta2.value=="") || (tarjeta3.value=="")){
            alert ( "Tarjetas vacías, ¿desea pagar con Paypal?")
        }else if (paypal.value>=10){
            alert("Paypal erróneo, verifícalo")
        }else {
            alert("Para pagar en efectivo ponte en contacto con 0999888723")
            window.location.href = "verifica.html"
        }
    }
}
</script>
2
  • Este código es sin duda el más limpio y quizá el más eficiente de todas las respuestas.
    – lilezek
    el 23 jul. 2017 a las 1:49
  • onclick.envia, es incorrecto (se que el OP también lo escribió mal) ¿podrías corregir tu código?
    – Marcos
    el 24 jul. 2017 a las 14:37
7

Bueno, cada uno lo hace a su manera, dejo lo que hice yo. Usé un for y return en el medio para detener todo. También intenté independizar las funciones.

function clickear()
{
    var variables=[tarjeta1,tarjeta2,tarjeta3,payPal]
    for(var i=0;i<variables.length;i++)
    {
        if(variables[i].value==""&i!=variables.length-1)
        {
            alert ( "Tarjetas vacías, ¿desea pagar con Paypal?");return;
        }
    }
    if(payPal>=10){alert("Paypal erróneo, verifícalo");return;}
    alert("Para pagar en efectivo ponte en contácto con 0999888723")
    window.location.href = "verifica.html"
}
function cargarVentana()
{
    var envia = document.getElementById(continuar)
    onclick.envia = clickear
}
window.onload = cargarVentana
6
  • Buena respuesta, pero no uses "alert" para desarrollo están bien porque te paran la ejecución del código y ves claramente si tu código funciona pero para producción quedan horribles. Puestos a establecer las notificaciones al usuario de dicha manera mejor sweet alert de boostrap. Le dan un aspecto mucho mas amigable
    – Korzan
    el 20 jul. 2017 a las 18:41
  • @Korzan Yo no uso alert, ya venía en la pregunta. PD: No me gusta usar Bootstrap.
    – user13558
    el 20 jul. 2017 a las 19:03
  • @Korzan Si tienes una alternativa, puedes dejar una nueva respuesta.
    – user13558
    el 20 jul. 2017 a las 19:06
  • En la condición dentro del bucle for falta un ampersand (&), y usar un bucle for con un array no es más rápido que poner tres ifs. Además, creo que el OP cuando escribe onclick.envia se refiere a envia.onclick o algo similar, ya que onclick no está definido en ningún sitio.
    – lilezek
    el 23 jul. 2017 a las 1:46
  • 1
    @ArtEze efectivamente, es lo mismo usar doble ampersand que uno solo en tu caso concreto. El OP está aprendiendo a programar, y en lugar de hacer un código más limpio, o más sencillo, utilizas un operador bitwise porque has añadido de forma totalmente artificial un vector con 3 tarjetas y un payPal, forzándote a comprobar en todo el bucle que no estás en la última iteración, cuando podrías haber puesto en el bucle que el iterador fuese de 0 a length-2 para evitar tener que comprobar que no estés en la última.
    – lilezek
    el 23 jul. 2017 a las 10:23
1

Tengo una propuesta compacta que se puede condensar en una sola línea. Podría ser más eficiente si dejamos el trabajo del reload/redirect al que (supongo) sería el FORM.

Considera lo siguiente:

(function(){
   document.getElementById(continuar).addEventListener('click', function(){
      [tarjeta1.value, tarjeta2.value, tarjeta3.value].indexOf("") >= 0 ? alert("Tarjetas vacías, ¿desea pagar con Paypal?") : (paypal.value >= 10 ? alert("Paypal erróneo, verifícalo") : (alert("Para pagar en efectivo ponte en contácto con 0999888723"), window.location.href = "verifica.html") );
   });
}());

Si no se ejecuta el window.location.href podríamos rodear la operación ternaria con un solo alert() y haríamos que el resultado de las operaciones ternarias fuera únicamente texto. Como este no es el caso, hacemos que el resultado de cada operción sea la función alert y en el tercer caso un compuesto.

Nota: en realción con la mayoría de las propuestas encontradas, me parece que se reduce el uso de variables, funciones y loops; resulta en una menor cantidad de ciclos usados para completar esta funcionalidad.

Edit 2:

Olvidé mencionar que el código de arriba requiere estar hasta abajo de tu página HTML para que sea ejecutado cuando los elementos YA existen. Para colocarlo al inicio del documento o incluirlo en un archivo al inicio se puede usar window.onload con el mismo código:

window.onload = function(){
   document.getElementById(continuar).addEventListener('click', function(){
      [tarjeta1.value, tarjeta2.value, tarjeta3.value].indexOf("") >= 0 ? alert("Tarjetas vacías, ¿desea pagar con Paypal?") : (paypal.value >= 10 ? alert("Paypal erróneo, verifícalo") : (alert("Para pagar en efectivo ponte en contácto con 0999888723"), window.location.href = "verifica.html") );
   });
};
2
  • 1
    Que sea más corto no significa que sea más eficiente. Creas un array e inicias una llamada a indexOf, que podría introducir carga en la ejecución.
    – lilezek
    el 23 jul. 2017 a las 1:10
  • Yo usaria operadoresternarios function clickear() { for (var a = [tarjeta1, tarjeta2, tarjeta3, payPal], e = 0; e < a.length; e++) if ("" == a[e].value & e != a.length - 1) return void alert("Tarjetas vacías, ¿desea pagar con Paypal?"); payPal >= 10 ? alert("Paypal erróneo, verifícalo") : (alert("Para pagar en efectivo ponte en contácto con 0999888723"), window.location.href = "verifica.html") } function cargarVentana() { document.getElementById(continuar); onclick.envia = clickear } window.onload = cargarVentana; el 25 jul. 2017 a las 0:57
1

Yo a tu codigo le haria unos cuantos ajustes.

Lo primero que me di cuenta es que nadie le agrego comillas a continuar, asumo que son comillas como no hay nada definido con el nombre de continuar

esto te generara error var envia = document.getElementById(continuar);

para limpiar un poco el codigo eliminaria los parentesis en la condicion, cambiaria esto:

if (( tarjeta=="") || (tarjeta2=="") || (tarjeta3==""))

Por esto:

if ( tarjeta=="" || tarjeta2=="" || tarjeta3=="")

Los parentesis son utiles para agrupar 2 o mas condiciones pero usarla en uno solo no tendria mucho sentido.

Tambien puedes reducir los var de esta manera.

        var tarjeta = tarjeta1.value,
        tarjeta2 = tarjeta2.value,
        tarjeta3 = tarjeta3.value,
        paYpal = paypal.value;

Aunque como ya mencionaron en otros comentarios, si no los vas a usar no es necesario que los declares.

Por otro lado tu condicion no esta bien, deberias usar la misma variable en todo la condicion fijate como leo tu condicion.

supongamos que el valor de tarjeta es igual a 1. diria,

si el valor de tarjeta es igual a vacio muestra el alert si no paYpal mayor o igual a 10

if (( tarjeta=="") || (tarjeta2=="") || (tarjeta3=="")){
                alert ( "Tarjetas vacías, ¿desea pagar con Paypal?")
            }else if (paYpal>=10){
                alert("Paypal erróneo, verifícalo")
            }else {
                alert("Para pagar en efectivo ponte en contácto con 0999888723")
                window.location.href = "verifica.html"
            }

Seria mejor dejarlo asi ya que no necesitas hacer otra condicion con la variable tarjeta.

if (tarjeta=="" || tarjeta2=="" || tarjeta3==""){
    alert ( "Tarjetas vacías, ¿desea pagar con Paypal?")
}

Lo mismo para paYpal.

if (paYpal>=10){
    alert("Paypal erróneo, verifícalo")
}

Ahora la ultima condicion fijate que no estoy seguro... ya que deberia haber una variable llamada efectivo por ejemplo que si es verdadero mostraria el mensaje.

alert("Para pagar en efectivo ponte en contácto con 0999888723")
window.location.href = "verifica.html"

Pienso que eso seria lo mejor. si el valor es verdadero muestro el mensaje y por cierto tiene un error le falta el ; a cada linea que codigo.

El resultado final quedaria asi.

<script>
    window.onload = function(){
        var envia = document.getElementById('continuar');
        onclick.envia = function (){

            // verificar si las tarjetas tienen algun valor
            if (tarjeta.value=="" || tarjeta2.value=="" || tarjeta3.value==""){
               alert ( "Tarjetas vacías, ¿desea pagar con Paypal?");
            }

            //verificar el valor de paypal 
            if (paYpal.value >= 10){
                alert("Paypal erróneo, verifícalo");
            }

            //verificar el valor de efectivo
            if( efectivo.value ) {
                alert("Para pagar en efectivo ponte en contácto con 0999888723");
                window.location.href = "verifica.html";
            }
        }
    }
</script>

Si! agregue comentarios, es una buena practica y es altamente recomendable comentar el codigo.

Otra cosa seria la estructura fijate el nombre del evento se llama envia onclick.envia = function() pero no estas enviando lo que hace el metodo es verificar o validar los datos, se prodria estructurar mejor el codigo

como por ejemplo;

function init(){
 //definimos las variables
 
 var el = document.getElementById('continuar');
 
 el.addEventListener("click", enviar );
}

function validar(){

  //aqui irian todas las condiciones
  //devolveremos true o false
  return true;
}
function enviar(){
  
  //llamamos al metodo validar()

  if ( validar() ){
    //si validar devuelve true
    // mostramos los errores en la pagina
    alert('hola soy un error');
  }else{
    //si no errores proseguimos a enviar los datos
  }
}

window.onload = init;
<button id="continuar"> continuar </button>

3
  • +1 por ser el único que ha corregido el onclick.envia. De todos modos, tu último ejemplo yo creo que es innecesario.
    – lilezek
    el 23 jul. 2017 a las 10:32
  • yo diria que es un habito que tengo, en lo personal siempre trabajo los projectos en gran escala por muy pequenos que sean por que nunca se que tanto pueden crecer y si no tienes estructurado tu proyecto en lo que empiece a crecer crecera codigo innecesario, puedo ver mi projecto despues de unos 3 anos y entenderlo sin mucho esfuerzo por que todo esta estructurado y si alguien usara my codigo le sera mas facil de entender.... el 23 jul. 2017 a las 10:55
  • La gente que se dedica a hacer grandes proyectos intenta evitar JavaScript. En cualquier caso, hay que distinguir entre un proyecto grande y un usuario que está aprendiendo a programar.
    – lilezek
    el 23 jul. 2017 a las 10:56
0

Aqui un intento con expressiones regulares:

var tarjeta1 = "3323434", tarjeta2 = "333", tarjeta3 = "", payPal = "32324232";
function clickear()
{
    var tarjetas = [tarjeta1,tarjeta2,tarjeta3,payPal].join(",").match(/([\w]+\,?)/g);
    if(tarjetas.length < 4)
    {
      alert ( "Tarjetas vacías, ¿desea pagar con Paypal?");return;
    }
    if(payPal>=10){alert("Paypal erróneo, verifícalo");return;}
    alert("Para pagar en efectivo ponte en contácto con 0999888723")
    window.location.href = "verifica.html"
}

clickear();

2
  • El usuario está aprendiendo a programar, ¿por qué introduces expresiones regulares? Son lentas (ya que hay que compilarlas) y son menos claras que poner un if detrás de otro.
    – lilezek
    el 23 jul. 2017 a las 1:48
  • Ok. Gracias por la observación.
    – Einer
    el 23 jul. 2017 a las 1:55

Tu Respuesta

By clicking “Publica tu respuesta”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

¿No es la respuesta que buscas? Examina otras preguntas con la etiqueta o formula tu propia pregunta.