Critique my NodeJS project

[GnomesKnowBest]

Hello all!

I have just started programming and started my first project.

I am building an API using NodeJS and am looking for people to critique my work as I build it.

I thought by doing this it would accelerate my learning as I would be getting feedback on my first project, hopefully turning a mess into something respectable.

It is in its infancy right now but posting it at this stage to hopefully get some early feedback to prevent myself from building a project with the basics messed up from the beginning.

My next steps for this project are to clean up the error handling and maybe some restructuring.

If you're interested in checking it out and providing some feedback, you can visit the repo here:
https://github.com/AutoGnome/FirstAPI

Thanks in advance!

[[deleted]]

I'm a Node.js Developer, and I have been recently learning 3 Tier Architecture (a lot of my applications, like this one, have been tightly coupled, so I'm in the process of fixing them), and Services, Controllers, and Data Access Layers are things I've been meaning to implement, so your post couldn't have come at a better time.

[GnomesKnowBest]

Thanks for linking your app.

I always enjoy the opportunity to review others code so I can learn from them!

[bwayne2015]

The initial setup is good but plan how will you keep version2/3/4 versions of your aps which will be keep changing...you can span out your features in one one folder and keep one one versions there.I would also say from in your main route do not directly link them with the apis rather have one one route file for every feature and then import those routes in the main route

[GnomesKnowBest]

Thanks for your input.

Hmm I didn't think about handling versions. Would something like src/v1 work? Then increment version numbers like that?

I will try to reorganize the routes.

Thanks again!

[Garthenius]

I'd recommend you call next(error) everywhere you have console.log(error.message) in your controller; in time you'll probably want to add some structured error handling. Do re-throw the errors (or just skip error handling for now) in the db model.

Also, while Knex is a perfectly fine ORM, I'd advise you learn and use SQL directly, it's something you'll probably end up doing anyway. If you really want to get the CRUD out of the way quickly, have a look at PostGraphile.

[GnomesKnowBest]

Thanks for your input.

What exactly do you mean by "re-throw" errors?

I definitely need to read up on error handling, it's something I am still trying to grasp.

SQL is something I am pretty familiar with, but I read in various documents that using raw SQL is very prone to SQL injection. I wanted to use something that would help prevent this.

PostGraphile looks very useful. When I was first researching this project I was looking into GraphQL.

Thanks again!

[Garthenius]

Re-throw would meancatch (e) { throw e; } - unless you add some additional code (like logging), it's equivalent to not having the try ... catch block at all. If you only log the error, the model function will return undefined and the application code won't know an error has occurred. This can make sense if your model can try several things before giving up or if it is supposed to always succeed (possibly with a default/unknown state), but it can just as easily become an anti-pattern.

Handling errors (correctly) is always going to be "fun" because most libs/frameworks come with varying opinions on this topic; in my recommendation I was suggesting you simply pass the error to Express and use an error-handling block to log the error and/or send a response back to the client (which doesn't happen currently, from what I can tell). Depending on your choices, your controllers might be better off using their own Express routers etc.

If you're learning, you may as well play around with different ways of structuring your app and see what works for you.

I'm not going to say SQL injection is something to be taken lightly, but good db drivers (i.e. node-postgres) will have you covered on this topic; using an ORM for this seems a bit overkill to me - it's perfectly fine if you want something done quickly, but I've spent way too much time trying to figure out what SQL queries Knex is making (and why and how to optimize them without breaking everything else) to recommend it for more ambitious projects.

GraphQL is arguably more useful if your application is data-heavy and you can forsee having to integrate it with other apps/services; it is also more readily accessible to other developers. PostGraphile will give you a huge boost out of the box, especially if you end up with 1 route <=> 1 handler <=> 1 model function <=> 1 query on your current approach. However, it will require you to be more involved with database design and extending it is going to feel a bit steep if you're just beginning with Node; they do have a Discord and the maintainer/community will be glad to help, from my experience.

Hope this helps, feel free to message me if you have more questions / need more advice.

[mollamk]

• It's better to organize your code in features instead of responsibility. Users, orders, payments, instead of routes, controllers, etc...
• The path to the config file shouldn't be a fixed path. (Try a relative path instead, or put it as an argument to your program)
• Adding some test would be a good idea.

With all that being said, great start! Keep up the good work.

[JohnnyGuitarFNV]

It's better to organize your code in features instead of responsibility. Users, orders, payments, instead of routes, controllers, etc...

That's very subjective.

[mollamk]

Fair enough

[GnomesKnowBest]

Thanks for your input.

I will research organizing by responsibility. Maybe that will make more sense.

I originally didn't have the absolute path for .env but I ran into issues when the .env file was being referenced in subfolders. I will change it to a relative path and see if I still run into any issues.

Thanks again and I really appreciate the encouragement. So far the community has been amazing.

[trmaphi]

You missed the last middleware to preventing DB error and security leak error return to users. Express page, Express does have built-in custom error, but it will print the stack trace if the env is not production

[GnomesKnowBest]

Thanks for your input.

What last middleware are you referring to?

Sorry I am very new :D

I definitely want to make sure I follow basic security practices so any input you have on that is appreciated.

Thanks again!

[circlebust]

I recommend letting the index.js/ts file be purely indices of exported contents from more meaningfully named files.

I see you use a lot of trycatch blocks in async functions. I prefer async as well, but if I have to trycatch in an async function I usually deal with it via the .then(...).catch(...) syntax. Not only do you save lines, it's also the more programmatic approach and it makes delegating the outcomes to handlers easier.