Well, I had spent a while going over that code example, and trying to figure out what I didn't like about it and why, and wrote up this whole review on it that expressed my thoughts, just to find out that I ended up writing a really long monster, which is why I simply started with that first half. But, as I already have a second half written, and I don't want that work to go to waste, I'll go ahead and post the rest of my thoughts.
Keep in mind that I'm totally cool if you don't agree with these thoughts or my conclusion, but it should hopefully at least express my point of view on the matter. I'm also able to be swayed in my stance given a sufficiently convincing argument - someone has recently convinced me that it's ok (and maybe even encouraged) to write longer functions, as long as you follow certain techniques to organize the logic within the function - looking at what I had written with fresh eyes and the perspective this person gave me, I see that that have I probably subdivided your code snippet into too many functions, but I'll just leave the review the way I had written it. In the end, I think it just comes down to how flexible we want our codebases to be, and as we both work in very different settings, we're going to have very different needs related to flexibility (a point that I addressed near the end).
So, here's the second half of my review on your code snippet.
Each of those function definitions are doing a lot of low-level database logic, which makes it difficult to read any particular function. You have to read the entire thing, step by step, to understand what's going on. Perhaps we can make this code easier to read over and understand at a glance with just a little bit of refactoring. Let's move all of the low-level database accessing logic together, into its own place.
I'm also going to toss your regex logic. You seem to be abusing a replacer function parameter in a pretty unconventional and unnecessary way. I'll just simplify that to use URLSearchParams() to parse the query parameters. This change is mostly for me, to help me work with your code, but I wanted to explicitly call out why I did it.
Full code
let globalLogoutListenerList1 = [];
let moduleSqlite = require("sqlite3");
let http = require("http");
let sqliteDb1;
function middlewareLogoutHandler1(req, res) {
globalLogoutListenerList1.forEach(function (listener) {
listener(req, res);
});
}
const databaseActions = {
initializeTables: sqlliteDb => sqlliteDb.exec(`
CREATE TABLE IF NOT EXISTS cached_session (
sessionid TEXT PRIMARY KEY NOT NULL,
userid INTEGER NOT NULL
);
CREATE TABLE IF NOT EXISTS server_log (
timestamp TEXT NOT NULL,
message TEXT NOT NULL
);
`),
insertDummyTestSession: sqlliteDb => sqlliteDb.exec(`
INSERT INTO cached_session VALUES (\u0027session1\u0027, \u0027user1\u0027);
`),
async getSession(sqliteDb, sessionid) {
return new Promise((resolve, reject) => {
sqliteDb.all(`
SELECT * FROM cached_session WHERE sessionid = ?
`, [
sessionid
], function (err, data) {
if (err) return reject(err);
resolve(data);
});
});
},
addLogEntry(sqliteDb, { timestamp, message }) {
sqliteDb.all(`
INSERT INTO server_log VALUES(?, ?);
`, [
timestamp, message
]);
},
removedCachedSession(sqliteDb, sessionid) {
sqliteDb.run(`
DELETE FROM cached_session WHERE sessionid = ?
`, [
sessionid
]);
},
};
async function logLogoutEvent(req) {
let sessionid = new URLSearchParams(req.urlParsed.search).get("sessionid");
let data = await databaseActions.getSession(sqliteDb1, sessionid);
if (!data) {
return "";
}
data = {
message: `userid ${data[0].userid} logout`,
timestamp: new Date().toISOString()
};
console.log("inserting to sqlite-log: ", data);
databaseActions.addLogEntry(sqliteDb1, data);
}
function invalidateSqlCacheOnLogout(req) {
let sessionid = new URLSearchParams(req.urlParsed.search).get("sessionid");
console.log(`invalidating sessionid ${sessionid}`);
databaseActions.removedCachedSession(sqliteDb1, sessionid);
}
function handleRequests(req, res) {
req.urlParsed = require("url").parse(req.url);
if (req.urlParsed.pathname === "/logout") {
middlewareLogoutHandler1(req, res);
}
res.end();
}
async function startHttpServer(requestHandler, { port }) {
await new Promise(resolve => {
http.createServer(requestHandler).listen(port, resolve);
});
}
(async function() {
sqliteDb1 = new moduleSqlite.Database(":memory:");
databaseActions.initializeTables(sqliteDb1);
databaseActions.insertDummyTestSession(sqliteDb1);
globalLogoutListenerList1.push(logLogoutEvent, invalidateSqlCacheOnLogout);
await startHttpServer(handleRequests, { port: 8080 });
console.log("\n\nserver listening on port 8080");
// test logout api
http.get("http://localhost:8080/logout?sessionid=session1");
// exit process after 500 ms
setTimeout(process.exit, 500);
}());
There we go! Now we don't have a bunch of low-level database access logic cluttering our higher-level functions. Plus, the database access logic is all centralized in a single place, so if we ever need to update the shape of any table (a semi-common need), we just have to edit a single point, instead of refactoring our entire codebase.
Before:
function logLogoutEvent(req) {
req.urlParsed.search.replace((
/[?&]sessionid=([^&]*)/
), async function (ignore, sessionid) {
let data = await new Promise(function (resolve) {
sqliteDb1.all(`
SELECT * FROM cached_session WHERE sessionid = ?
`, [
sessionid
], function (err, data) {
if (err) {
throw err;
}
resolve(data);
});
});
if (!data) {
return "";
}
data = {
message: `userid ${data[0].userid} logout`,
timestamp: new Date().toISOString()
};
console.log("inserting to sqlite-log: ", data);
sqliteDb1.all(`
INSERT INTO server_log VALUES(?, ?);
`, [
data.timestamp, data.message
]);
});
}
After:
const databaseActions = {
...
addLogEntry(sqliteDb, { timestamp, message }) {
sqliteDb.all(`
INSERT INTO server_log VALUES(?, ?);
`, [
timestamp, message
]);
},
...
}
async function logLogoutEvent(req) {
let sessionid = new URLSearchParams(req.urlParsed.search).get("sessionid");
let data = await databaseActions.getSession(sqliteDb1, sessionid);
if (!data) {
return "";
}
data = {
message: `userid ${data[0].userid} logout`,
timestamp: new Date().toISOString()
};
console.log("inserting to sqlite-log: ", data);
databaseActions.addLogEntry(sqliteDb1, data);
}
That really cleans it up.
But, there's one more thing. I mentioned before that one of the valuable reasons for this refactor is that it allows you to look in one place to see all of the database actions. It becomes extremely easy to update the shape of a table, because you know exactly where to find all of the code that uses that table. Almost. There's actually nothing stopping a programmer from using that table directly, and there's no way to know if someone is doing direct calls to the database without searching through the whole codebase. Unless, we introduce some encapsulation, to provide some guarantees to future readers that database access only happens within that section of the codebase. There's many ways to achieve this - it could be private module-level variables that you don't export, or a factory function. It's really all the same. I'm just going to use a class for it.
Full Example
let globalLogoutListenerList1 = [];
let moduleSqlite = require("sqlite3");
let http = require("http");
let datastore
function middlewareLogoutHandler1(req, res) {
globalLogoutListenerList1.forEach(function (listener) {
listener(req, res);
});
}
class DataStore {
#sqliteDb;
constructor() {
this.#sqliteDb = new moduleSqlite.Database(":memory:");
Object.freeze(this);
}
initializeTables = () => this.#sqliteDb.exec(`
CREATE TABLE IF NOT EXISTS cached_session (
sessionid TEXT PRIMARY KEY NOT NULL,
userid INTEGER NOT NULL
);
CREATE TABLE IF NOT EXISTS server_log (
timestamp TEXT NOT NULL,
message TEXT NOT NULL
);
`)
insertDummyTestSession = () => this.#sqliteDb.exec(`
INSERT INTO cached_session VALUES (\u0027session1\u0027, \u0027user1\u0027);
`)
async getSession(sessionid) {
return new Promise((resolve, reject) => {
this.#sqliteDb.all(`
SELECT * FROM cached_session WHERE sessionid = ?
`, [
sessionid
], function (err, data) {
if (err) return reject(err);
resolve(data);
});
});
}
addLogEntry({ timestamp, message }) {
this.#sqliteDb.all(`
INSERT INTO server_log VALUES(?, ?);
`, [
timestamp, message
]);
}
removedCachedSession(sessionid) {
this.#sqliteDb.run(`
DELETE FROM cached_session WHERE sessionid = ?
`, [
sessionid
]);
}
}
async function logLogoutEvent(req) {
let sessionid = new URLSearchParams(req.urlParsed.search).get("sessionid");
let data = await datastore.getSession(sessionid);
if (!data) {
return "";
}
data = {
message: `userid ${data[0].userid} logout`,
timestamp: new Date().toISOString()
};
console.log("inserting to sqlite-log: ", data);
datastore.addLogEntry(data);
}
function invalidateSqlCacheOnLogout(req) {
let sessionid = new URLSearchParams(req.urlParsed.search).get("sessionid");
console.log(`invalidating sessionid ${sessionid}`);
datastore.removedCachedSession(sessionid);
}
function handleRequests(req, res) {
req.urlParsed = require("url").parse(req.url);
if (req.urlParsed.pathname === "/logout") {
middlewareLogoutHandler1(req, res);
}
res.end();
}
async function startHttpServer(requestHandler, { port }) {
await new Promise(resolve => {
http.createServer(requestHandler).listen(port, resolve);
});
}
(async function() {
datastore = new DataStore();
datastore.initializeTables();
datastore.insertDummyTestSession();
globalLogoutListenerList1.push(logLogoutEvent, invalidateSqlCacheOnLogout);
await startHttpServer(handleRequests, { port: 8080 });
console.log("\n\nserver listening on port 8080");
// test logout api
http.get("http://localhost:8080/logout?sessionid=session1");
// exit process after 500 ms
setTimeout(process.exit, 500);
}());
Perfect! Overall, we didn't really add that much bloat to the code. All we did was organize it a bit, using coding practices that are pretty standard for any language.
I get the feeling that you like to write your JavaScript code so that it has optimum flexibility, i.e. you don't like putting "arbitrary" restrictions on your code. - I hope you realize that this comes at a huge cost - it's very much not scalable. A repository that's created for maximum flexibility is one with absolutely no organization. Say you keep all of your data public, just in case someone, somewhere needs to use or edit it. And, say you keep all of your methods public for the same reason. And, if you're ok opening up permissions on your functions and data to allow anyone to access it freely, you must also be ok freely accessing functions and data from anywhere in the codebase. This, is the very definition of spaghetti code. Code that has no rules or constraints. Code that's free to access whatever from wherever it wants to. We place restrictions on our codebase in order to add predictability to it. Sure it's possible that some future developer might want to do a direct database query, and if they really want to do so, they can simply flip the private "#sqliteDb" property to public and do it. Nothing's stopping that future developer from making stuff public. However, as long as it's private, any future readers can be certain that requests to that database instance are only coming out of that single class, no where else. And it's exactly because of that kind of restriction that this sort of code becomes much more readable and maintainable. You can rely on things being true, without having to assume. You don't have to read the whole codebase in order to simply add a column to a table, you only need to read the part that matters, because you can see that the restrictions that are in place are preventing any other part of the application from touching the database instance.
Of course, this is a moving slider. Too much organization is also a bad thing. The more you organize your code, the more rigid you make it, and the more refactoring you have to do if changes come along that go against the way it's been organized. So, there's a balance. Everything discussed thus far is true for any programming language out there, it's nothing unique to JavaScript. People end up with large codebases in all of the major general purpose languages out there, and they have to figure out the right amount of organization to put into it - how much to break apart the functions, what kinds of abstractions to put in, etc.
