ref. https://gist.github.com/silverark/2fd88d03fddbe373fce87da80afdc3ce
Line 22:
This connection string should be global, and reusable for all the operations, and not recreated for every method
Line 22:
It’s a really bad code practice to put the connection string inside the code, the common usage is to declare the variables inside a .env file for different environments, but never inside the code itself,
The variables, user, password and dbname should not be public.
Line 23, 43:
Should be avoided use select *, as unwanted fields would be returned, so a good practice would be to return just the needed fields
Line 26:
Here we have vulnerability to sql injection as we are just passing a string to it directly, so would be great also to validate the possible values for the orderby and throw if it’s passeed an incorrect
Line 38:
Comments for unused code should be removed, and we may have just code that is effectively being used.
- Normally I prefer to use some
ORMto avoid manipulation of the sql queries directly, and makes the code more readable, and often there’re some ORM’s that allow the easy switch between other databases - It seems the naming for the method OrderBYAndGetAll is not following the pattern, it would be great to use the By instead of BY