0
\$\begingroup\$

I'm currently writing an activity reporting system based on users; upon looking through a lot of files I'm noticing that I do a lot of if statements that clearly can be rewritten;

How would you improve this structure of file? I can then take any ideas/improvements from this file & apply them to the rest of the project.

Any improvements are welcome!

const conf = require('../../conf');
const globals = require('../../globals');
const ping_history = require('./ping_history');
const ConsumerAccounts = require('../models/Consumeraccounts');
const redis = globals.redis();

module.exports = active = {};

active.prefix = 'activeusers';

active.expires_secs = globals.secs('3 minutes');

active.payout = {
    amount_cents: conf.consumerportal.country_payments["default"].daily_cents,
    prefix: 'activepayouts',
    history_prefix: 'activepayoutshistory',
    threshold_secs: globals.secs('1 hours'),
    increment_secs_per_ping: 60
};

active.mark_installed = mark_installed = async function(uuid) {
    await ConsumerAccounts.findOne({_id: uuid}).then(result => {
        if (result.software_installed) {
            return false;
        } else {
            return result.mark_installed(uuid);
        }
    }).catch(err => {

    });
};

active.payout_increment = async function(uuid, cb) {
    let amount_cents, amount_owed, history_key, k, max_amount, ref, ref1, ref2, ref3, ref4, ref5, ref6, ref7, ref8, tally_key, v;
    let today = await globals.today();
    tally_key = [this.payout.prefix, globals.today(), uuid].join(':');
    history_key = [this.payout.history_prefix, globals.today(), uuid].join(':');
    await redis.incrby(tally_key, parseInt(this.payout.increment_secs_per_ping));
    await redis.get(tally_key).then((tally) => {
        if (tally && (+tally) >= this.payout.threshold_secs) {
            let history = redis.get(history_key).catch((err) => {
                throw err;
            });
            ConsumerAccounts.findOne({_id: uuid}).then((consumer) => {
                ping_history.add(uuid, today);
                //checks here!
                if (!consumer.payout_eligible) {
                    return cb(null, false);
                }
                redis.set(history_key, 1);
                amount_cents = this.payout.amount_cents;
                max_amount = conf != null ? (ref3 = conf.consumerportal) != null ? (ref4 = ref3.country_payments) != null ? (ref5 = ref4["default"]) != null ? ref5.payout_cap_cents : void 0 : void 0 : void 0 : void 0;
                if (typeof consumer !== "undefined" && consumer !== null ? (ref6 = consumer.questionnaire) != null ? ref6.country : void 0 : void 0) {
                    ref7 = conf.consumerportal.country_payments;
                    for (k in ref7) {
                        v = ref7[k];
                        if (k === consumer.questionnaire.country) {
                            amount_cents = v != null ? v.daily_cents : void 0;
                            max_amount = v != null ? v.payout_cap_cents : void 0;
                        }
                    }
                }
                if ((conf != null ? (ref8 = conf.consumerportal) != null ? ref8.limit_amount_owed : void 0 : void 0) && this.payout_eligible) {
                    amount_owed = (function(_this) {
                        return function() {
                            let cents, i, len, ref10, ref9, x;
                            cents = 0;
                            ref10 = (ref9 = _this.revenue_history) != null ? ref9 : [];
                            for (i = 0, len = ref10.length; i < len; i++) {
                                x = ref10[i];
                                cents = x.amount_cents;
                            }
                            return cents;
                        };
                    })(this)();
                    if (amount_owed >= max_amount) {
                        this.payout_eligible = false;
                        consumer.mark_ineligible();
                        return false;
                    }
                }
                consumer.assign_revenue(globals.today(), amount_cents);
                consumer.record_event({
                    event: 'daily_payout_assigned',
                    amount_cents: amount_cents
                });
            }).catch(err => {
                console.log('Consumer account was not found');
            });
        } else {
            console.log("We haven't hit the correct number yet!");
            console.log(tally + " - " + this.payout.threshold_secs);
        }

    }).catch((err) => {
        console.log(err);
        console.log("Error Getting Tally Key!")
    });
};

active.ws_ping = async function(obj, cb) {
    let expires_secs, ref, val;
    if (!obj.uuid) {
        return cb(new Error('Property `uuid` required'));
    }
    expires_secs = (ref = obj.expires_secs) != null ? ref : this.expires_secs;
    await this.mark_installed(obj.uuid);
    await this.payout_increment(obj.uuid);
    val = 'u';
    return redis.set(this.prefix + ":" + obj.uuid, val, 'EX', expires_secs);
};

active.ping = async function(obj, cb) {
    let expires_secs, ref, val;
    if (!obj.ip) {
        return cb(new Error('Property `ip` required'));
    }
    if (!obj.port) {
        return cb(new Error('Property `port` required'));
    }
    if (!obj.relay_ip) {
        return cb(new Error('Property `relay_ip` required'));
    }
    if (!obj.relay_port) {
        return cb(new Error('Property `relay_port` required'));
    }
    if (!obj.uuid) {
        return cb(new Error('Property `uuid` required'));
    }
    expires_secs = (ref = obj.expires_secs) != null ? ref : this.expires_secs;
    await this.mark_installed(obj.uuid);
    val = obj.ip + "_" + obj.port + "_" + obj.relay_ip + "_" + obj.relay_port;
    return redis.setex(this.prefix + ":" + obj.uuid, expires_secs, val);
};
```
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
active.mark_installed = mark_installed = async function(uuid) {
    await ConsumerAccounts.findOne({_id: uuid}).then(result => {
        if (result.software_installed) {
            return false;
        } else {
            return result.mark_installed(uuid);
        }
    }).catch(err => {

    });
};

Note that async/await is just sugar syntax for promises. Few key things to remember when using it:

  • await "waits" for promises.
  • async functions return a promise.
  • Returned values from an async function resolves the promise it returns.
  • Thrown errors inside async rejects the promise it returns.

Nothing is wrong with chaining then in async/await, the syntax is perfectly legit. I often use it to inline some operations. However, it's usually not needed. If you're going async/await, go all-in with it.

This could be rewritten as:

active.mark_installed = mark_installed = async function(uuid) {
  try {
    const result = await ConsumerAccounts.findOne({_id: uuid})
    return result.software_installed ? false : result.mark_installed(uuid)
  } catch(error) {

  }
}

active.ws_ping = async function(obj, cb) {

active.ping = async function(obj, cb) {

This is an anti-pattern. You're using an async function (which returns a promise that should inform you if the asynchronous operation succeeded or failed), but then you pass in a callback for the error.

Instead of passing in the callback, have the caller attach a then with the callback as the error callback, or await the call in a try-catch.

active.ping = async function(obj) {

  // Throwing an error in an async effectively rejects the returned promise.
  if (!obj.ip) throw new Error('Property `ip` required')
  if (!obj.port) throw new Error('Property `port` required')
  if (!obj.relay_ip) throw new Error('Property `relay_ip` required')
  if (!obj.relay_port) throw new Error('Property `relay_port` required')
  if (!obj.uuid) throw new Error('Property `uuid` required')

  const ref = obj.expires_secs
  const expires_secs = ref != null ? ref : this.expires_secs

  await this.mark_installed(obj.uuid)

  const val = `${obj.ip}_${obj.port}_${obj.relay_ip}_${obj.relay_port}`

  return redis.setex(`${this.prefix}:${obj.uuid}`, expires_secs, val)
}

// Usage
active.ping(obj).then(result => {

}, error => {

})

// or
try {
  const result = await active.ping(obj)
} catch (error) {

}

Few other minor nitpicks:

  • Not a big fan of "declare all variables ahead of time". What happens is cognitive overhead, you scroll up to know what's there/what you can assign to. Declare variables as you need it.
  • Most of your variables are only ever assigned a value once. Use const for those variables.
  • Use Template Literals to construct interpolated strings.
  • JavaScript uses camelCase for naming. While your data could be snake-case due to its origins, use camelCase stay consistent on the JS side of things.
  • For temporary variables in blocks (i.e. loops, ifs), use let/const within the loop block. This way, you're sure that the variable is scoped to that block, and never leaks out to the outer scopes.
\$\endgroup\$
1
  • \$\begingroup\$ thankyou very much! my main issue is trying to rewrite the payout_increment function; seems so clunky! \$\endgroup\$
    – Curtis
    Jul 11, 2019 at 13:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.