My Basic Tax Calculator

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 = 'Valo

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)

StackExchange Code Review Q#149156, answer score: 3