How to improve this: Hell callback?

Asked

Viewed 178 times

8

I am finishing the development of an ad system, the system uses the redis server to save data and display campaigns directly from ram memory, since the pathology of these systems require a lot of disk consumption. Every time I need to get the values spread through collections on my server redis and save in two relational tables on my mysql server. However, I am too young for asynchronous programming and do not master the promises, although the code below seems to work well. I would not like to leave you with this essay because it becomes difficult to maintain the code.

This routine inserts the cost of each campaign, including user connections, campaign impressions, and ad clicks on affiliate websites. In other words, I need a loop for each zone created by the editors, adding up the number of connections in those zones, then adding to that collection the impressions and clicks of each campaign and sending over 15-second intervals to the mysql server.

SalvarStatsCustosCampanha = function(){
  cliente_redis.smembers("campanhas_disponiveis",function(err,campanhas){
    var multi                                    = cliente_redis.multi();
    for(var i= 0; i<campanhas.length;i++){
      multi.smembers("zona_membros_campanhas:"+campanhas[i]);
    }
    multi.exec(function(err,zonas){
      async.forEachOf(zonas,function(lista,indice,callback){
        var desconectados                        = 0;
        var conectados                           = 0;
        async.each(lista,function(zona,fn){
          desconectados                         += (typeof(global.desconectados_zona[zona]) != 'undefined')? global.desconectados_zona[zona] : 0;
          conectados                            += (typeof(global.conectados_zona[zona])    != 'undefined')? global.conectados_zona[zona]    : 0;
          fn();
        },function(err){
          cliente_redis.hmget("campanha:"+campanhas[indice],['id_usuario','cliques','impressoes','custo'],function(err,info){
            let campanha                         = {};
            campanha.id_anunciante               = info[0];
            campanha.id_campanha                 = campanhas[indice];
            campanha.conectados                  = desconectados;
            campanha.impressoes                  = (info[2] == null)? 0 : info[2];
            campanha.cliques                     = (info[1] == null)? 0 : info[1];
            campanha.custo                       = (info[3] == null)? 0 : info[3];
            if(campanha.cliques                 == 0){
              campanha.ctr                       = 0;
              campanha.ctr                       = campanha.ctr.toFixed(3);
            }else campanha.ctr                   = (parseFloat(campanha.cliques) / parseFloat(campanha.conectados)).toFixed(3);
            campanha.data                        = moment().format("YYYY-MM-DD");
            campanha.fechamento                  = moment().format();
            var data                             = {};
            data.campanha                        = campanha;
            data.data                            = moment().format("YYYY-MM-DD");
            fnCampanha.GetCustoCampanhaDia(data,function(data,custo){
              CampanhaSangria                    = function(campanha){
                cliente_redis.hmset("campanha:"+campanha.id_campanha,{"cliques": 0,"custo" : 0,"impressoes": 0});
              }
              if(typeof(custo) != 'undefined'){
                data.campanha.id_custo           = custo.id_custo;
                fnCustos.AlterarCustos(data.campanha,CampanhaSangria);
              }else{
                fnCustos.AdicionarCustos(data.campanha,CampanhaSangria);
              }
            });
          });
        });
      });
    });
  });
}

The same logic is employed for the campaign zones responsible for linking the ads on affiliated sites, only the number of impressions is not computed because the zone receives per socket in real time dozens of campaigns so I only effective the accounting of impressions for campaigns.

SalvarStatsRendimentosZona = function(){
  cliente_redis.smembers('zonas_disponiveis',function(err,zonas){
    if(zonas.length > 0){
      Falha                                       = function(){
        console.log("O processo de update de rendimentos falhou");
      }
      Sucesso                                     = function(data,zona){
        cliente_redis.hmget("zona:"+data.zona.id_zona,['cliques','rendimentos'],function(err,cliques){
          if(cliques != null){
            let rendimento                        = {};
            rendimento.conectados                 = (!isNaN(global.desconectados_zona[zona.id_zona]))? global.desconectados_zona[zona.id_zona]: 0;
            /* Se não houver conexões sobre a zona no dia corrente não há necessidade de alterar ou inserir apenas ignoramos
            esse registro até que alguma conexão exista */
            if(parseInt(rendimento.conectados) > 0){
              rendimento.id_zona                  = zona.id_zona;
              rendimento.id_publicador            = zona.id_publicador;
              rendimento.cliques                  = cliques[0];
              if(rendimento.cliques              == 0){
                rendimento.ctr                    = 0;
                rendimento.ctr                    = rendimento.ctr.toFixed(3);
              }else rendimento.ctr                = (parseFloat(rendimento.cliques) / parseFloat(rendimento.conectados)).toFixed(3);
              rendimento.ganhos                   = parseFloat(cliques[1]).toFixed(3);
              rendimento.fechamento               = moment().format();
              data.rendimento                     = rendimento;
              data.data                           = moment().format("YYYY-MM-DD");
              fnRendimento.GetRendimentosZonaDia(data,function(data,rendimentos){
                ZonaSangria                       = function(){
                  cliente_redis.hmset("zona:"+data.zona.id_zona,{"cliques": 0,"rendimentos" : 0});
                  cliente_redis.hmset("stats:zonas:"+data.zona.id_zona,{"desconectados" : 0});
                  global.desconectados_zona[data.zona.id_zona] = 0;
                }
                data.rendimento.data              = data.data;
                if(typeof(rendimentos)           != 'undefined'){
                  data.rendimento.id_rendimento   = rendimentos.id_rendimento;
                  fnRendimento.AlterarRendimento(data.rendimento);
                  ZonaSangria();
                }else{
                  fnRendimento.AdicionarRendimentos(data.rendimento);
                  ZonaSangria();
                }
              });
            }
          }
        });
      }
      zonas.forEach(function(id_zona){
        var data                              = {};
        data.zona                             = {};
        data.debug                            = 'zonas disponiveis';
        data.zona.id_zona                     = id_zona;
        data.sucesso                          = Sucesso;
        data.sucesso_redis                    = Sucesso;
        data.falha                            = Falha;
        fnZona.GetZonaById(data);
      });
    }
  });
}

Finally, I have to run these two routines simultaneously to avoid clearing the global variable of the script that keeps the number of visitors disconnected before saving to my database, since after saving to both tables I assign zero to it, in each campaign zone. Losing disconnections would be a risk for the statistical calculation of the application. I need to prevent the socket disconnection event from confusing the results that are being saved in the database when my setInterval runs because it is a global scope variable, so I used the new async await standard by changing the version of my Node.js server from 6 to 8, to simplify encoding and run functions simultaneously. How can I improve this code? I would like something vertical that allows me better code maintenance in the future. 

setInterval(function(){
  async function ExecuteActions(){
    await SalvarStatsCustosCampanha();
    await SalvarStatsRendimentosZona();
  }
  ExecuteActions();
},configs.ttl_save_stats);

I know the question is comprehensive and time-consuming analysis plus any opinion would be valid to me!

enter image description here

javascript node.js async

  • You are using at least version 8 of Node?

  • Yes, I installed it yesterday. But the code was all developed in version 6. So I didn’t have async and await support.

  • 1

    From version 8 you can use async and await, which will greatly facilitate the maintenance and understanding of the code. If it is possible to use this version I can rewrite the code for you using the new features of ECMAScript and applying the standard of AirBNB coding in Javascript

  • If you give me a simple example of working, it would already help me a lot not need to be exactly following my code can be a brief explanation about the functioning of async and await. Yesterday I analyzed the documentation but it does not seem to me comprehensive of examples.

  • So, there’s actually a lot of stuff that you can improve on your code, so I think it’s easier to extract the concepts and rewrite. One thing, for example, is that you are doing everything in one function and that already greatly increases the complexity. There are several things that can be improved and I write a complete answer about it, but for that I need to know if version 8 will be available or not

  • Yes yesterday I upgraded my server to version 8

  • 1

    I recommend that you try to reduce the question to a simpler example, something that demonstrates your problem without all this complexity of your application. It would be especially useful if you could post something that is "executable" locally, for example a code that populates Redis and allows you to run your code. The chances of you getting good answers would be much higher with something like this.

  • The code works perfectly, but it is exactly the complexity that generated Hell callback, I need a more functional way to write this without so much confusion in the scopes to facilitate future maintenance. Even with the advent of async and await I don’t have the knack of how to modularize this so that I have access to the variables in each scope.

Show 3 more comments

1 answer

2


First let’s solve your problem setInterval. To ensure that there will not be two simultaneous executions of the same code, I suggest you change to setTimeout and restart the counter after running the root function (prinicpal) as follows:

const execucaoRaiz = async () => {
  try {
    // Garantirá que as duas funções executarão em paralelo
    const promessas = [
      salvarStatsCustosCampanha(),
      salvarStatsRendimentosZona(),
    ];

    // O código só prosseguirá quando as duas execuções forem completadas
    await Promise.all(promessas);

    // Eventual código adicional após a execução das duas funções principais
    // ...
  } catch(e) {
    console.error(e);
  }

  // Garante que o timer será reiniciado mesmo que o código ocasione algum erro
  iniciarTimer();
};

const iniciarTimer = async () => {
  setTimeout(execucaoRaiz, configs.ttl_save_stats);
}

// Executará a primeira vez após o tempo determinado
iniciarTimer();

Right after that amendment, I suggest that you use promises in the case of redis. I don’t know exactly what the execution of the module is like, but looking at your code I believe the structure callback be the one agreed by the Node community (function callback(erro, resultado) {}). Thus it is possible to use the function util.promisify to turn them into promises:

const { promisify } = require('utils');

// ...

const smembers = promisify(cliente_redis.smembers);

const campanhas = await smembers('campanhas_disponiveis');

// ...

Finally turn your functions into async so that you can use the await within them:

const salvarStatsCustosCampanha = async () => {
  // Lógica da função
}

It will be imperative that you break the code in more functions to facilitate the understanding of it in future maintenance.

In my examples above I used the Airbnb Style Guide as a basis. Following it will make your code cleaner and readable and also apply some good practices to it.

I described in general because your code is not executable, so it becomes difficult to rewrite it without the possibility to test the result.

Browser other questions tagged javascript node.js async

You are not signed in. Login or sign up in order to post.