From 958498292c8b52861a954b99a80f0eabf0a5eb4a Mon Sep 17 00:00:00 2001 From: David Blanchard Date: Sun, 10 Mar 2024 19:32:39 -0600 Subject: [PATCH] Add some high level review comments --- .env.example | 3 +++ ormconfig.js | 2 ++ src/config/races.ts | 5 ++++- src/index.ts | 13 ++++++++++++- src/routes/actions/achievements.ts | 4 ++++ src/routes/attack.ts | 23 ++++++++++++++++++++++- src/routes/spells/advance.ts | 10 ++++++++++ src/util/helpers.ts | 8 ++++++++ 8 files changed, 65 insertions(+), 3 deletions(-) create mode 100644 .env.example diff --git a/.env.example b/.env.example new file mode 100644 index 0000000..034fc09 --- /dev/null +++ b/.env.example @@ -0,0 +1,3 @@ +# Having an example .env file is useful because you can commit all of the expected environment +# variable names. That way if you have to install this project on a new machine you'll +# have a better idea of everything needed in order to run it. diff --git a/ormconfig.js b/ormconfig.js index 6a66167..4cf0e02 100644 --- a/ormconfig.js +++ b/ormconfig.js @@ -17,6 +17,8 @@ module.exports = { namingStrategy: new SnakeNamingStrategy(), entities: [rootDir + '/entity/**/*{.ts,.js}'], subscribers: [rootDir + '/subscriber/**/*{.ts,.js}'], + // You should really be using migrations to handle all of your schema + // updates as opposed to letting typeorm handle the synchronization. migrations: [rootDir + '/migrations/**/*{.ts,.js}'], cli: { entitiesDir: rootDir + '/entity', diff --git a/src/config/races.ts b/src/config/races.ts index 00b79b3..ad3a694 100644 --- a/src/config/races.ts +++ b/src/config/races.ts @@ -7,7 +7,10 @@ // const orc = 6 // const drow = 7 // const goblin = 8 - +/* + This is another thing that I think would probably be better as a database table + instead of being hardcoded here. +*/ const RACE_HUMAN = { name: 'Human', mod_offense: 0, diff --git a/src/index.ts b/src/index.ts index b604773..1f4f078 100644 --- a/src/index.ts +++ b/src/index.ts @@ -103,7 +103,11 @@ app.get('/api/perpetual/hello', (req, res) => { console.log(req.url) res.send('hello perpetual') }) - +/* + In my earlier PR I created and index file in your routes directory that exported a single router + to be consumed here as `app.use('/api', apiRoutes)` -- I'd recommend following that pattern + and moving all of these individual routes from here into that index file. +*/ app.use('/api/auth', authRoutes) app.use('/api/empire', empireRoutes) app.use('/api/useturns', useTurns) @@ -134,6 +138,10 @@ app.get('/debug-sentry', function mainHandler(req, res) { // The error handler must be registered before any other error middleware and after all controllers app.use(Sentry.Handlers.errorHandler()) +/* + I'd move this error handler and the Sentry initialization call to their own file + that you then import at the top of this one. +*/ // Optional fallthrough error handler app.use(function onError(err, req, res, next) { // The error id is attached to `res.sentry` to be returned @@ -164,6 +172,9 @@ function checkTime() { } else return false } +/* + I'd move all of this scheduler logic into a separate file. +*/ if (process.env.NODE_ENV === 'development') { let gameOn = false diff --git a/src/routes/actions/achievements.ts b/src/routes/actions/achievements.ts index 6e51f71..5feb341 100644 --- a/src/routes/actions/achievements.ts +++ b/src/routes/actions/achievements.ts @@ -1,6 +1,10 @@ import Empire from '../../entity/Empire' import { achievements } from '../../config/achievements' +/* + You've got a copy of this method that also lives in the EmpireSubscriber so it's + worth moving it to the utils directory and then importing it from there. +*/ function sortObject(obj: Record) { return Object.keys(obj) .sort() diff --git a/src/routes/attack.ts b/src/routes/attack.ts index eb0d057..390dbab 100644 --- a/src/routes/attack.ts +++ b/src/routes/attack.ts @@ -918,7 +918,28 @@ const attack = async (req: Request, res: Response) => { // won = true let buildLoss: buildLoss = {} let buildGain: buildGain = {} - + /* + One big general note is to try and keep your files on the smaller side + (think a couple hundred lines on the high end). + Not sure if you've heard of the DRY acronym when it comes to coding, but it stands for Don't Repeat Yourself. + It's a pretty easy thing to look out for that can help keep files to more manageable sizes. + The destroyBuildings calls you have below are a decent example of mostly repeated code that can be shortened + up with a pretty simple refactor. The only things that change from call to call are arguments 2-4 so I'd suggest + putting them in an array and then iterating through it and calling destroyBuildings for each item. + + It would look something like: + + const destroyBuildingsParams = [ + { pcloss: 0.07 * lowLand, pcgain: 0.7 * lowLand, type: 'bldCash' }, + { pcloss: 0.07 * lowLand, pcgain: 0.7 * lowLand, type: 'bldPop' }, + { pcloss: 0.07 * lowLand, pcgain: 0.5 * lowLand, type: 'bldTroop' }, + .... + ] + + destroyBuildingParams.forEach(({ pcloss, pcgain, type }) => { + destroyBuildings(attackType, pcloss, pcgain, type, defender, attacker, buildLoss, buildGain) + }) + */ destroyBuildings( attackType, 0.07 * lowLand, diff --git a/src/routes/spells/advance.ts b/src/routes/spells/advance.ts index 96542b5..1b16585 100644 --- a/src/routes/spells/advance.ts +++ b/src/routes/spells/advance.ts @@ -1,3 +1,13 @@ +/* + I would suggest moving the `spells` and `actions` directories out of the routes directory and + maybe into something like a `services` directory. + I'd also try to keep the files that are in the routes directory mapped to the route path + that it's responsible for. + For example: + `routes/admin.ts` is responsible for handling calls to `/api/admin` + Doing it this way creates a nice paradigm for knowing what your route tree + looks like just by perusing your file system. +*/ import { eraArray } from '../../config/eras' import Empire from '../../entity/Empire' import EmpireEffect from '../../entity/EmpireEffect' diff --git a/src/util/helpers.ts b/src/util/helpers.ts index f246b83..b07c8d2 100644 --- a/src/util/helpers.ts +++ b/src/util/helpers.ts @@ -1,3 +1,11 @@ +/* + Both makeId and slugify are examples of things I think are better to leverage a library for. + https://www.npmjs.com/package/uuid can be used for generating IDs. + Or you can also use the native `crypto.randomUUID()` if you're only using the v4 + uuid generator (which is all I've ever used). + As for slugify I would recommend using lodash: https://www.npmjs.com/package/lodash + It has a ton of useful utility methods and one of them is `kebabCase` which is equivalent to your slugify. +*/ export function makeId(length: number): string { let result = '' const characters =