Skip to content

Grading PR with comments and feedback [DO NOT MERGE] #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
martypdx wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Grading PR with comments and feedback [DO NOT MERGE] #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2edb379
initial commit
martypdx Mar 9, 2017
c80f253
comments, feedback and suggestions
martypdx Mar 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# dbSimsPDX
A backend game of life.

**Nice Job on README**

# [Visit the game on Heroku](https://dbsimspdx.herokuapp.com/)

The goal of the game is to retire as young as possible with a networth of $1,000,000!
Expand Down
Empty file removed lib/models/admin.model.js
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions lib/routes/assets.routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ assetRouter
})

/* __NOT__ a feature for a user */
// so how do you prevent them from accessing????
.post('/', bodyParser, (req, res, next) => {
new Asset (req.body).save()
.then(asset => {
Expand Down
6 changes: 3 additions & 3 deletions lib/routes/jobs.routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ const jobRouter = Router();
jobRouter
.get('/', (req, res, next) => {
Job.find()
.lean()
// .select(only needed properties)
.then(jobs => res.send(jobs))
.catch(next);
})
.post('/', bodyParser, (req, res, next) => {
return new Job(req.body).save()
.then(job => {
res.send(job);
})
.then(job => res.send(job))
.catch(next);
});

Expand Down
241 changes: 149 additions & 92 deletions lib/routes/user.routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const express = require('express');
const Router = express.Router;
const userRouter = Router();
const mongoose = require('mongoose');
const User = require('../models/user.model');
const Asset = require('../models/asset.model');
const Job = require('../models/job.model');
Expand All @@ -10,7 +9,6 @@ const Education = require('../models/education.model');
const token = require('../auth/token.js');
const bodyParser = require('body-parser').json();
const ensureAuth = require('../auth/ensure-auth')();
const ensureRole = require('../auth/ensure-roles')();

function hasUserNameAndPassword(req, res, next) {
const user = req.body;
Expand All @@ -24,28 +22,29 @@ function hasUserNameAndPassword(req, res, next) {
}

function getJob(query) {
let entryJob = {};
return Job.find( { jobLevel: 'Entry' } )
.then(jobs => {
//TODO: investigate using aggregate $sample
if(!jobs.length) return;
let job = jobs.filter(j => {
return j.jobType === query;
});
entryJob.start_date = new Date();
entryJob.months_worked = 0;
entryJob.job_name = job[0]._id;
return entryJob;
});
// it's really this easy to have mongo do the work:
return Job.aggregate([
{ $match: { jobLevel: 'Entry', jobType: query } },
{ $sample: { size: 1 } }
])
.then(jobs => {
if(!jobs.length) return;
return {
start_date: new Date(),
months_worked: 0,
job_name: jobs[0]._id,

};
});
}

function getFirstEducation() {
let firstEd = {};
// don't define variables in broader scope than needed!
// let firstEd = {};
return Education.findOne( { educationLevel: 'High School' } )
.then(school => {
if(!school) return;
firstEd = school._id;
return firstEd;
return school._id;
});
}

Expand All @@ -54,6 +53,10 @@ userRouter
res.send({ valid: true });
})

/* don't put comment bands like this in your code. :(
Find a way to beter modularize your code
*/

//////////////////////////////USER/////////////////////////////////

.post('/signup', bodyParser, hasUserNameAndPassword, (req, res, next) => {
Expand All @@ -65,23 +68,23 @@ userRouter
error: `username ${data.username} already exists`
};
else {
data.retired = false;
data.age = 18;
data.bank_account = Math.floor(Math.random()*200000) + 1;
data.networth = data.bank_account;
data.assets = [];
data.activities = [];
data.original_signup = new Date();
data.last_sign_in = data.original_signup;
data.roles = 'user';
return getJob('Unskilled');
return Promise.all([
getJob('Unskilled'),
getFirstEducation(),
]);
}
})
.then(job => {
.then(([job, education]) => {
data.retired = false;
data.age = 18;
data.bank_account = Math.floor(Math.random()*200000) + 1;
data.networth = data.bank_account;
data.assets = [];
data.activities = [];
data.original_signup = new Date();
data.last_sign_in = data.original_signup;
data.roles = 'user';
data.job = job;
return getFirstEducation();
})
.then(education => {
data.education = education;
return new User(data).save();
})
Expand Down Expand Up @@ -115,88 +118,142 @@ userRouter
let data = req.body;
let desiredAsset = {};
let savedAsset = {};
const username = req.params.username;
Asset.findById(data._id)
.then(asset => {
desiredAsset = asset;
});
return User.findOne({username: username})
.then(user => {
const cash = user.bank_account;
if(desiredAsset.purchase_price > cash) {
res.status(400).send({error: 'You do not have funds for this purchase'});
} else {

user.bank_account = user.bank_account - desiredAsset.purchase_price;
savedAsset.purchase_date = new Date();
savedAsset.asset_name = desiredAsset._id;
user.assets.push(savedAsset);
}
return user.save();
})
.then(user => res.send(user))
.catch(next);

// security hole: you are allowing any user to change another user's
// data. If this is intended to be the same as logged in user,
// you need to use req.user.id

// Also, these are parallel, not sequention actions:

Promise.all([
Asset.findById(data._id),
User.findById(req.user.id)
])
.then(([desiredAsset, user]) => {

const cash = user.bank_account;
if(desiredAsset.purchase_price > cash) {
// use your error handler via next:
throw { code: 400, error: 'You do not have funds for this purchase'};
}
// throw exists, so no need to else here
user.bank_account = user.bank_account - desiredAsset.purchase_price;
savedAsset.purchase_date = new Date();
savedAsset.asset_name = desiredAsset._id;
user.assets.push(savedAsset);
return user.save();
})
.then(user => res.send(user))
.catch(next);
})

.post('/:username/activities', bodyParser, (req, res, next) => {

function getActivityOutcome(user, activity) {
// The code in this route is a mess, I've cleaned it up as
// example

// original function was mixing asynchronous and synchronous behavior
// (has user.save(), but returns a value)

// this could go at top of module...
const REWARD_MESSAGE = 'YOU WON THE LOTTERY!!!! Congratulations!!! You have achieved or surpassed a total networth of $1M dollars. You have won the dbSimsPDX game and are able to retire at the young age of ' + `${user.age}` + ' years old ... !!!';
const NO_REWARD_MESSAGE = 'You got what you paid for! No reward today. Womp.';

function isReword(odds, networth) {
let randomNum = Math.floor(Math.random()*100 + 1);
if (randomNum <= activity.rewardOdds) {
user.activities.wonReward = true;
return randomNum <= odds && networth > 1000000;
}

// this functionality should be moved onto user model:
// user.addActivity(activity)
function getActivityOutcome(user, activity) {
const wonReward = isReword(activity.rewardOdds, user.networth);
const newActivity = { activity_name: activity._id };
user.activities.wonReward = wonReward;
user.activities.push(newActivity);
activity.rewardMessage = NO_REWARD_MESSAGE;

if (wonReward) {
user.bank_account = user.bank_account + activity.rewardAmount;
user.networth = user.networth + activity.rewardAmount;
user.activities.push(savedActivity);
if(user.networth > 1000000) {
activity.rewardMessage = 'YOU WON THE LOTTERY!!!! Congratulations!!! You have achieved or surpassed a total networth of $1M dollars. You have won the dbSimsPDX game and are able to retire at the young age of ' + `${user.age}` + ' years old ... !!!';
activity.rewardMessage = REWARD_MESSAGE;
}
user.save();
return activity.rewardMessage;
} else {
user.activities.wonReward = false;
user.activities.push(savedActivity);
user.save();
return 'You got what you paid for! No reward today. Womp.';
}

return user
.save()
.then(() => ({ message: activity.rewardMessage }));
}

let reqActivityId = req.body;
let desiredActivity = {};
let savedActivity = {};
const username = req.params.username;

Promise.all([
Activity.findById(req.body),
User.findById(req.user.id)
])
.then(([desiredActivity, user]) => {
const cash = user.bank_account;

if(desiredActivity.purchasePrice > cash) {
throw { code: 400, error: 'You do have funds for this purchase' };
}

user.bank_account -= desiredActivity.purchasePrice;
user.networth -= desiredActivity.purchasePrice;
return getActivityOutcome(user, desiredActivity);
})
.then(message => res.send(message))
.catch(next);

// request body is a string? use an object instead: { id: <id> }
// let reqActivityId = req.body;
// let desiredActivity = {};
// let savedActivity = {};
// const username = req.params.username;

// You've cut and pasted this comment WITHOUT
// understanding what it means :(
// (it doesn't apply here)
//TODO: investigate using aggregate $sample
Activity.findById(reqActivityId)
.then(activity => {
if(!activity) return;
else {desiredActivity = activity;}
});
return User.findOne( { username: username } )
.then(user => {
const cash = user.bank_account;

if(desiredActivity.purchasePrice > cash) {
res.status(400).send({error: 'You do have funds for this purchase'});
} else {
user.bank_account = user.bank_account - desiredActivity.purchasePrice;
user.networth = user.networth - desiredActivity.purchasePrice;
savedActivity.activity_name = desiredActivity._id;

// Activity.findById(reqActivityId)
// this then block does nothing
// .then(activity => {
// if(!activity) return;
// else {desiredActivity = activity;}
// });

// There is no connection from previous promise and this one.
// Are are relying on unguaranteed order of execution
// return User.findOne( { username: username } )
// .then(user => {
// const cash = user.bank_account;

// if(desiredActivity.purchasePrice > cash) {
// res.status(400).send({error: 'You do have funds for this purchase'});
// } else {
// user.bank_account = user.bank_account - desiredActivity.purchasePrice;
// user.networth = user.networth - desiredActivity.purchasePrice;
// savedActivity.activity_name = desiredActivity._id;

return getActivityOutcome(user, desiredActivity);
}
})
.then(message => {
res.send(message);
})
.catch(next);
// return getActivityOutcome(user, desiredActivity);
// }
// })
// .then(message => {
// res.send(message);
// })
// .catch(next);


})

.post('/:username/education', bodyParser, (req, res, next) => {
let reqEdId = req.body;


const username = req.params.username;

// WAY TOO NESTED!
// return promises and handle in next "this"
Education.findById(reqEdId)
.then(desiredEd => {
if(!desiredEd) return;
Expand Down
Loading