feat: javascript-server #28
Conversation
Added commands used
feat: Add VS-CODE.md (#39512)
change format
Docs: features and plugins of vscode (#39512)
feat: MERN Stack,12 factor app (#39513)
feat: Intro to Node.js & NPM (#39515)
| { | ||
| str=str+" "; | ||
| } | ||
| for(let j=0;j<=i;j++) |
There was a problem hiding this comment.
No proper indentation is done over here in this file
| let str=""; | ||
| for(let k=0;k<n-i-1;k++) | ||
| { | ||
| str=str+" "; |
| @@ -0,0 +1,3 @@ | |||
| import diamond from "./diamond.js"; | |||
| import equilateral from "./equilateral.js"; | |||
| if(email.match(regex)) | ||
| { | ||
| return true; | ||
| } |
| { | ||
| return true; | ||
| } | ||
| else |
There was a problem hiding this comment.
Do we really need else statement over here?
There was a problem hiding this comment.
no, we do not need else statement
| return false; | ||
| let data=permissions[moduleName]; | ||
| let tmp=false; | ||
| if(data[permissionType]===undefined) |
| let invalidEmails=[]; | ||
| user.forEach(function(value,index) | ||
| { | ||
| const {traineeEmail,reviewerEmail}=user[index]; |
There was a problem hiding this comment.
If we are getting value as first argument then why do we need to extract value using index?
| import { hasPermission, validateUser } from './utils/index'; | ||
| import { Iuser } from './interface'; | ||
| diamond(10); | ||
| equilateral(8); |
There was a problem hiding this comment.
Please a proper space between import statements and calling of functions.
| } | ||
| export { Iuser }; | ||
| interface Iauthority { | ||
| all: string[]; |
There was a problem hiding this comment.
Export at the end of file with single export statement.
| return false; | ||
| const data = permissions[moduleName]; | ||
| let tmp: boolean = false; | ||
| if (data[permissionType] === undefined) |
There was a problem hiding this comment.
What if value of data[permissionType] is null?
| import { userRepository } from '../../repositories/user'; | ||
| import SystemResponse from '../../libs/SystemResponse'; | ||
| import * as bcrypt from 'bcrypt'; | ||
| class Controller { |
There was a problem hiding this comment.
One line space is needed.
| import { userRepository } from '../../repositories/user'; | ||
| import SystemResponse from '../../libs/SystemResponse'; | ||
| import * as bcrypt from 'bcrypt'; | ||
| class Controller { |
There was a problem hiding this comment.
Better to name it TraineeController
| try { | ||
| const hash = await bcrypt.hash(password, 10); | ||
| email = email.toLowerCase(); | ||
| name = name.toLowerCase(); |
There was a problem hiding this comment.
Why are converting name to lowercase?
| let { name, email } = req.body; | ||
| try { | ||
| const hash = await bcrypt.hash(password, 10); | ||
| email = email.toLowerCase(); |
There was a problem hiding this comment.
Why do not we do it using the validation handler by using custom function?
There was a problem hiding this comment.
Because I have to make a custom function that will return a promise and i have to handle promise which will make my validation handler non-generic.
| const hash = await bcrypt.hash(password, 10); | ||
| email = email.toLowerCase(); | ||
| name = name.toLowerCase(); | ||
| const validity = await userRepository.findByEmail(email); |
There was a problem hiding this comment.
Why are we creating methods such as findByEmail(email)?
We should be creating function for findOne({email}) which will accept an object as it can be reused again by passing different arguments, Suppose I need to find a user by Id then I will need a another function which do not make sense to me.
| const validity = await userRepository.findByEmail(email); | ||
| if (!validity) { | ||
| const userData = await userRepository.create(req.user._id, { name, address, email, dob, mob, hobbies, role, password: hash }); | ||
| const message = 'Trainee added successfully'; |
| const userData = await userRepository.create(req.user._id, { name, address, email, dob, mob, hobbies, role, password: hash }); | ||
| const message = 'Trainee added successfully'; | ||
| userData.password = '*****'; | ||
| console.log(userData); |
There was a problem hiding this comment.
You can write the above code as below:
if(!validation) {
throw error;
}
Then continue coding
The above code looks good and also reduces the number of bolcks as we do not need else block now
| sortBy = { name: req.query.order}; | ||
| else | ||
| sortBy = {updatedAt: req.query.order}; | ||
| if (req.query.search !== undefined) { |
There was a problem hiding this comment.
Creating query for sort and search, There is too much of if and else, I do not think it was necessary, We may use switch.
And better to have helpers methods for these types of tasks that can handle it in more good manner
| else | ||
| sortBy = {updatedAt: req.query.order}; | ||
| if (req.query.search !== undefined) { | ||
| dataList = await userRepository.list('trainee', req.query.skip, req.query.limit, sortBy, {name: { $regex: req.query.search.toLowerCase()}}); |
There was a problem hiding this comment.
Why do we need to call list method of userRepository two times?
And why are we creating sortBy and even then passing limit and skip?
Why are we not taking a query object from controller to repository rather than passing arguments.
| dataList = await userRepository.list('trainee', req.query.skip, req.query.limit, sortBy, {name: { $regex: req.query.search.toLowerCase()}}); | ||
| const List = await userRepository.list('trainee', req.query.skip, req.query.limit, sortBy, {email: { $regex: req.query.search.toLowerCase()}}); | ||
| dataList = {...dataList, ...List }; | ||
| } |
There was a problem hiding this comment.
There are unnecessary calling of functions which can be reduced.
| update = async (req, res: Response) => { | ||
| console.log('----------Update Trainee----------'); | ||
| try { | ||
| const value = await userRepository.update(req, req.body.id, req.body.dataToUpdate); |
There was a problem hiding this comment.
Why are we sending req object to update method????
| const value = await userRepository.update(req, req.body.id, req.body.dataToUpdate); | ||
| if (value) { | ||
| const message = 'Trainee Data successfully Updated'; | ||
| const data = req.body.dataToUpdate; |
There was a problem hiding this comment.
We do not have to send the data that is being updated rather send the whole user after updation.
| SystemResponse.success(res, data, message); | ||
| } | ||
| else | ||
| return SystemResponse.failure(res, 'User data is not Updated', 'Email id already exist'); |
There was a problem hiding this comment.
Why are we sure that the error will be of Email duplication?
| console.log('----------Delete Trainee----------'); | ||
| try { | ||
| const value = await userRepository.delete(req, req.params.id); | ||
| if (value) { |
There was a problem hiding this comment.
Same comments as for above update method.
| } | ||
| me = (req, res: Response) => { | ||
| console.log('--------------me-------------'); | ||
| delete req.user.password; |
There was a problem hiding this comment.
Why are we not using try catch in controller?
| version: '1.0.0', | ||
| }, | ||
| securityDefinitions: { | ||
| Bearer: { |
There was a problem hiding this comment.
Why are we not taking these options from config??
| console.log(err); | ||
| } | ||
| else { | ||
| console.log(`Express app Successfully started on port : ${Port} `); |
| }); | ||
| } | ||
| catch (err) { | ||
| console.log(err); |
| @@ -0,0 +1,7 @@ | |||
| export interface IConfig { | |||
| Port: string; | |||
There was a problem hiding this comment.
Why are using Pascal Case over here for keys??
| Port : process.env.PORT, | ||
| NODE_ENV : process.env.NODE_ENV, | ||
| Key : process.env.SECRET_KEY, | ||
| MongoURL: process.env.MONGO_URL, |
There was a problem hiding this comment.
Pascal case for keys why??
Please use camel case
| @@ -0,0 +1,6 @@ | |||
| import { Server } from './Server'; | |||
| import config from './config/configuration'; | |||
| const server: Server = new Server(config); | |||
There was a problem hiding this comment.
One line space before this line
| import UserRepository from '../../repositories/user/UserRepository'; | ||
| import IUserModel from '../../repositories/user/IUserModel'; | ||
| interface IRequest extends Request { | ||
| user: IUserModel; |
There was a problem hiding this comment.
Move this interface to respective file or folder
| console.log(userData); | ||
| req.user = userData; | ||
| const role: string = userData.role; | ||
| if (decodedUser._id === req.body.id && req.method === 'PUT') |
There was a problem hiding this comment.
Why do we need this condition??
There was a problem hiding this comment.
so that the user can update his own data
| if (count === 0) { | ||
| const { Password } = config; | ||
| const hash = await bcrypt.hash(Password, 10); | ||
| UserRepository.create(undefined, { ...user, password: hash }); |
There was a problem hiding this comment.
Use try catch in this please
| } | ||
| update = (req, id, updatedData) => { | ||
| return super.update(req, id, updatedData); | ||
| } |
There was a problem hiding this comment.
No proper indentation.
Why eslint is not throwing errors?
| } | ||
| async delete(req, id) { | ||
| const oldData: any = await this.versionModel.findOne({originalId: id , deletedAt: undefined}).exec(); | ||
| return this.versionModel.findByIdAndUpdate( oldData._id , |
There was a problem hiding this comment.
Why are we passing req to this function rather we should be creating data and then sending the data to this function
| const oldData: any = await this.versionModel.findOne({originalId: id , deletedAt: undefined}).exec(); | ||
| const { name, address, email, dob, mob, hobbies, role, password} = oldData; | ||
| if ( updatedData.name !== undefined) | ||
| updatedData.name = updatedData.name.toLowerCase(); |
There was a problem hiding this comment.
Why lowercasing the name?
And why is the logic over here??
It should only contain the logic to updating document and making versions of the same.
THis function should not bother about the the data coming for updation
| return false; | ||
| } | ||
| } | ||
| public list(userRole, skipRecord, limitRecord, sortBy, searchBy) { |
There was a problem hiding this comment.
Why are we making query over here??
It should be passed by controller.
| import { Router } from 'express'; | ||
| import { traineeRouter, userRouter } from './Controllers'; | ||
| const mainRouter: Router = Router(); | ||
| mainRouter.use('/trainee', traineeRouter); |
Day 1 - Basic GIT Intro, features of VS CODE
Day 2 - MERN Stack,12 factor app
Day 3 - Intro to Node.js & NPM
Day 4 - Objects and Array
Day 5 - Packages in Node.js
Day 6 - Advantages of TypeScript
Day 7 - Setup of Express (OOps based)
Day 8 -HTTP Status code and handlers
Day 9 - Setting of Routes Skeletons (REST API)
Day 10 -Generic Routes Validations
Day 11 - Authentication and Authorization
Day 12 - Intro to Mongoose and promises
Day 13 - Models setup with initial seed
Day 14 - Profile Setup
Day 15 - Setup all the Entities
Day 16 - Magic of Await
Day 17 - APIs Completion
Day 18 - APIs Completion