3
\$\begingroup\$

The goal is:

  • I can add many services as I need.
  • I can add many taxes as I need (the idea is that services has all of this taxes)
  • Calculate per services each tax value
  • Sum all the taxes of each service and throw me the total value per service.

This is the first attempt at making a JavaScript program and I would love to hear any advice, what did I do wrong, what is right, what could be better or improved.

CodePen

// Services
var servicios = [
    //  { nombre_servicio: 'Fotografia', valor: 40000 }, { nombre_servicio: 'Tarjetas', valor: 60000 }
];
var tablaServicios = 0;
var nS = document.getElementById('nombre-servicio');
var vS = document.getElementById('valor-servicio');
var botonAgregarServicio = document.getElementById('agrega-valor');

//Taxes
var impuestos = [
    //{ nombre_impuesto: 'Iva', valor: 6 }, { nombre_impuesto: 'ICA', valor: 2 }
];
var tablaImpuestos = 0;
var nI = document.getElementById('nombre-imp');
var vI = document.getElementById('num-imp');
var botonAgregarImpuesto = document.getElementById('agrega-impuesto');

//Calculate button
var botonCalcular = document.getElementById('calcular');

// Calculate Event on click
botonCalcular.addEventListener("click", calcularImpuestos);

//Add service
botonAgregarServicio.addEventListener("click", agregaServicio);

//Get input values and add to services array
function agregaServicio() {
    servicios.push({
        nombre_servicio: nS.value,
        valor: vS.value
    });

    //Close modal    
    $('#modalServ').modal('hide');
    //Clear inputs 
    $('.agrega-servicio input').val('');

    //Get last item of services array
    var s = servicios[servicios.length - 1].nombre_servicio;
    var v = servicios[servicios.length - 1].valor;

    //Add table if not exist
    if (tablaServicios == 0) {
        genera_tabla('servicios');

        var tabla = document.querySelectorAll('.servicios thead tr th');
        tabla[0].innerHTML = 'Nombre servicio';
        tabla[1].innerHTML = 'Valor servicio'

        agregaFila('.servicios', s, '$ ' + v);

        //Delete "add new service" overlay
        var el = document.querySelectorAll('#servicios .no-agrego');
        el[0].parentNode.removeChild(el[0]);

        tablaServicios = 1;

    } else {
        agregaFila('.servicios', s, '$ ' + v);
    }

}

//Add tax
botonAgregarImpuesto.addEventListener("click", agregaImpuesto);

//Get input values and add to taxes array
function agregaImpuesto() {
    impuestos.push({
        nombre_impuesto: nI.value,
        valor: vI.value
    });

    //Close modal     
    $('#modalImp').modal('hide');
    //Clear inputs
    $('.agrega-impuesto input').val('');

    //Get last item of taxes array
    var s = impuestos[impuestos.length - 1].nombre_impuesto;
    var v = impuestos[impuestos.length - 1].valor;

    //Add table if not exist
    if (tablaImpuestos == 0) {
        genera_tabla('impuestos');

        var tabla = document.querySelectorAll('.impuestos thead tr th');
        tabla[0].innerHTML = 'Nombre impuesto';
        tabla[1].innerHTML = 'Valor impuesto (%)'

        agregaFila('.impuestos', s, v + ' %');

        //Delete "add new tax" overlay
        var el = document.querySelectorAll('#impuestos .no-agrego');
        el[0].parentNode.removeChild(el[0]);

        tablaImpuestos = 1;

    } else {
        agregaFila('.impuestos', s, v + ' %');
    }

}

function calcularImpuestos() {

    if (servicios.length > 0 && impuestos.length > 0) {

        //Loop services array items
        for (var i = 0; i < servicios.length; i++) {

            //Results container
            var resultados = document.getElementById('genResultados');

            //Generate elements in DOM
            var contenedorTabla = document.createElement('div');
            var modulo = document.createElement('div');
            var h1 = document.createElement('h1');
            var tituloServicio = document.createTextNode(servicios[i].nombre_servicio);
            var panel = document.createElement('div');
            var panelBody = document.createElement('div');
            var panelFooter = document.createElement('div');
            var row = document.createElement('div');
            var col1 = document.createElement('div');
            var titR1 = document.createTextNode('Total');
            var col2 = document.createElement('div');

            //Add created elements into container
            col1.appendChild(titR1);
            row.appendChild(col1);
            row.appendChild(col2);
            h1.appendChild(tituloServicio);
            modulo.appendChild(h1);
            modulo.appendChild(panel)
            panel.appendChild(panelBody);
            panel.appendChild(panelFooter);
            panelFooter.appendChild(row);
            panelBody.appendChild(contenedorTabla);
            resultados.appendChild(modulo);

            //assign classes and id to elements
            contenedorTabla.id = 'resultados-' + i + '-servicio';
            modulo.className = 'servicio-' + i;
            panel.className = 'panel panel-default';
            row.className = 'row';
            col1.className = 'col-md-6 col-xs-6';
            col2.className = 'col-md-6 col-xs-6';
            panelBody.className = 'panel-body';
            panelFooter.className = 'panel-footer';

            genera_tabla('resultados-' + i + '-servicio');

            //Add thead
            var tablaHead = document.querySelectorAll('.resultados-' + i + '-servicio thead tr th');
            tablaHead[0].innerHTML = 'Nombre impuesto';
            tablaHead[1].innerHTML = 'Valor del impuesto';

            //Array to store taxes results
            var mArray = []

            //Loop taxes array
            for (var j = 0; j < impuestos.length; j++) {
                var nombreImpuesto = impuestos[j].nombre_impuesto;
                //Tax value
                var k = impuestos[j].valor;

                //Service value
                var l = servicios[i].valor;

                //Math operation get percentage 
                var m = (k * l) / 100;

                //Add [m] result to array 
                mArray.push(m);

                //Add row to html results each tax name and value
                agregaFila('.resultados-' + i + '-servicio', nombreImpuesto, '$ ' + m);

            }

            //Get service module
            var tablaBody = document.querySelectorAll('.resultados-' + i + '-servicio');

            //Sum all taxes per service            
            var sumaImpuestos = 0;

            for (var x = 0; x < mArray.length; x++) {
                sumaImpuestos += mArray[x];
            }

            //Add result to dom
            var titR2 = document.createTextNode('$ ' + sumaImpuestos);
            col2.appendChild(titR2);

        }

        //Delete calculate buton
        var el = document.querySelectorAll('.well #calcular');
        el[0].parentNode.parentNode.removeChild(el[0].parentNode);

    } else {
        alert('No hay valores para calcular');
    }

}

function genera_tabla(id) {
    // Get container element
    var contenedor = document.getElementById(id);
    // Create element <table> and <tbody> <thead>
    var tabla = document.createElement("table");
    var tblBody = document.createElement("tbody");
    var thead = document.createElement('thead');
    var tr = document.createElement('tr');
    var thNombre = document.createElement('th');
    var thValor = document.createElement('th');

    tr.appendChild(thNombre);
    tr.appendChild(thValor);
    thead.appendChild(tr);
    tabla.appendChild(thead);

    // place <tbody> inside <table>    
    tabla.appendChild(tblBody);
    // place <table> inside container
    contenedor.appendChild(tabla);
    //Add bootstrap class to table
    tabla.className = 'table ' + id;
}

//Add row to table
function agregaFila(clase, nombre, valor) {
    //Get table
    var tabla = document.querySelectorAll(clase);
    //Insert row at the end of the table
    var fila = tabla[0].children[1].insertRow(-1);
    //Insert cell 1
    var celda1 = fila.insertCell(0);
    //Insert cell 2
    var celda2 = fila.insertCell(1);
    //Insert data from name
    celda1.innerHTML = nombre;
    //Insert data from value
    celda2.innerHTML = valor;
}
<html class="no-js" lang="">

<head>
  <meta charset="utf-8">
  <meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">
  <title></title>
  <meta name="description" content="">
  <meta name="viewport" content="width=device-width, initial-scale=1">
  <style>
    body {
        padding-top: 50px;
        padding-bottom: 20px;
    }
  </style>
  <link rel="stylesheet" href="css/main.css">
  <script src="js/vendor/modernizr-2.8.3.min.js"></script>
</head>

<body>
  <nav class="navbar navbar-inverse navbar-fixed-top" role="navigation">
    <div class="container">
      <div class="navbar-header">
        <a class="navbar-brand center-block" href="#">Taxes</a>
      </div>
    </div>
  </nav>
  <div class="container">
    <div class="row">
      <div class="col-md-12">
        <h2>Services</h2>
        <div class="panel panel-default">
          <div id="servicios" class="panel-body">
            <div class="no-agrego"><span class="glyphicon glyphicon-pencil"></span> Add new service</div>
          </div>
          <div class="panel-footer">
            <a id="agregar-valor" data-toggle="modal" data-target="#modalServ" class="btn btn-lg btn-default btn-block"> <span class="glyphicon glyphicon-plus-sign" aria-hidden="true"></span> Add service</a>
          </div>
        </div>
      </div>
      <div class="col-md-12">
        <h2>Tax</h2>
        <div class="panel panel-default">
          <div id="impuestos" class="panel-body">
            <div class="no-agrego"><span class="glyphicon glyphicon-pencil"></span> Add new tax</div>
          </div>
          <div class="panel-footer">
            <a id="agregar-impuesto" data-toggle="modal" data-target="#modalImp" class="btn btn-lg btn-default btn-block"> <span class="glyphicon glyphicon-plus-sign" aria-hidden="true"></span> Add tax</a>
          </div>
        </div>
        <div class="well">
          <a id="calcular" class="btn btn-lg btn-primary btn-block"> <span class="glyphicon glyphicon-cog" aria-hidden="true"></span> Calculate</a>
        </div>
      </div>
      <div id="genResultados" class="col-md-12">

      </div>
    </div>
    <hr>
    <footer>
    </footer>
    <!-- Modal  Servicio-->
    <div class="modal fade" id="modalServ" tabindex="-1" role="dialog">
      <div class="modal-dialog" role="document">
        <div class="modal-content">
          <div class="modal-header">
            <button type="button" class="close" data-dismiss="modal" aria-label="Close"><span aria-hidden="true">&times;</span></button>
            <h4 class="modal-title">Add service</h4>
          </div>
          <div class="modal-body">
            <div class="agrega-servicio">
              <div class="form-group">
                <label for="nombre-servicio">Service name</label>
                <input type="text" class="form-control" id="nombre-servicio" placeholder="Ex: Logo Design">
              </div>
              <div class="form-group">
                <label for="num-valor">Service value</label>
                <input type="number" class="form-control" id="valor-servicio" placeholder="Ex: 40000">
              </div>
              <button type="submit" class="btn btn-primary" id="agrega-valor">Add</button>
              <button type="button" class="btn btn-default" data-dismiss="modal">Close</button>
            </div>
          </div>
        </div>
      </div>
    </div>
    <!-- Modal  Impuesto-->
    <div class="modal fade" id="modalImp" tabindex="-1" role="dialog">
      <div class="modal-dialog" role="document">
        <div class="modal-content">
          <div class="modal-header">
            <button type="button" class="close" data-dismiss="modal" aria-label="Close"><span aria-hidden="true">&times;</span></button>
            <h4 class="modal-title">Add tax</h4>
          </div>
          <div class="modal-body">
            <div class="agrega-impuesto">
              <div class="form-group">
                <label for="nombre-imp">Tax name</label>
                <input type="text" class="form-control" id="nombre-imp" placeholder="Ex: VAT">
              </div>
              <div class="form-group">
                <label for="num-imp">Tax value (%)</label>
                <input type="number" class="form-control" id="num-imp" placeholder="Ex: 6">
              </div>
              <button type="submit" class="btn btn-primary" id="agrega-impuesto">Add</button>
              <button type="button" class="btn btn-default" data-dismiss="modal">Close</button>
            </div>
          </div>
        </div>
      </div>
    </div>
  </div>
  <!-- /container -->
  <!--   <script src="//ajax.googleapis.com/ajax/libs/jquery/1.11.2/jquery.min.js"></script> -->
  <script src="js/vendor/jquery-1.11.2.min.js"></script>
  <script src="js/vendor/bootstrap.min.js"></script>
  <script src="js/main.js"></script>
</body>

</html>

\$\endgroup\$
3
\$\begingroup\$

Advice 1

You break the Single Responsibility principle in calcularImpuestos().

The name, I guess, lead to a function that calculate something. But inside you do other staff like design a part of the HTML and style.

You should move this code inside a different function designed to handle HTML only. And have just calculation inside this function.

Advise 2

You use mainly javascript DOM API to create the UI, this is not so good.

You should write your HTML and use a different approach introducing template engines or write your own short version.

This helps maintainability on the code, as with the plain HTML you could have a better idea about what should appear on the page. And enhancements will also be easy to do.

Another good reason is you can easily divide the view/presentation from the model and the logic. (this is not strictly connected to MVC patterns, instead it is always a good design principle)

Advice 3

In agregaServicio() and agregaImpuesto() you have broke the Don't Repeat Yourself principle.

The code is pretty much the same, you could write a single function that do the task, with few parameters.

You could still have your 2 different functions without parameters, that will call the implementation, with the proper values:

function agregaServicio() {
    return aggrega('servicio');
}

function agregaImpuesto() {
    return aggrega('impuesto');    
}

function aggrega(what) {
    // here the code...
}

This is just an example. You could choose to pass all the different ids, classes as parameters.

Advice 4

You should wrap your all your code inside a function, to have your own private name space, and avoid to have conflicts with other code.

This is not strictly necessary in your case, but it is a good practice to use a module pattern, so you could reuse your code in different places.

Advice 5

You should choose a name convention and just follow.

I saw you use camel case, and even underscore:

  • calcularImpuestos()

  • genera_tabla(id)

\$\endgroup\$
  • \$\begingroup\$ Thanks, appreciate it alot for take your time to answer, i will definitely take note on your advices \$\endgroup\$ – Juansei Dec 10 '16 at 22:37

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.