Clean up nodeJS function

Question

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);
};
```

Answer 1

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.

• Curtis: thankyou very much! my main issue is trying to rewrite the payout_increment function; seems so clunky!