Grading and Feedback PR [DO NOT MERGE]

README.md

@@ -2,7 +2,7 @@
 "SWAPI races" is a game based on the characters and environment of Star Wars.  User will choose a character which will determine his alliance.  He will be assigned a starter ship, based on his alliance, that he will use to race.  The user will race against a non-alliance Bounty Hunter (computer) and/or another user.
 
 # MOTIVATION
-This project is being created to demonstrate a relational database.
+This project is being created to demonstrate a ~~relational~~ NoSQL database.
 
 # API REFERENCE
 We are using the Star Wars API SWAPI for our characters, vehicles, planets, and more.
@@ -16,8 +16,14 @@ https://swapi.co/
 - Post-race, the user will either receive money/credit for his win or nothing for his loss.
 - Lastly, if the user wins the race, he can take his winnings and purchase an upgraded vehicle.
 
+# API
+
+List out api calls with more technical style documentation
+
 # STYLE
 We are using Promises to handle our asynchronous code, creating modules for repeated code, and using whitespace for paragraph separation only.
 
 # GROUP MEMBERS
-Our group is called Ctr8 and we are Robin, Andrew, Eli and Kate.
+Our group is called Ctr8 and we are Robin, Andrew, Eli and Kate.
+
+// initial grading/feedback commit

lib/models/planet.js

@@ -6,4 +6,10 @@ const planetSchema = new Schema ({
     name: RequiredString
 });
 
+planetSchema.statics.getRandomPlanetId = function () {
+    return this.aggregate([{
+        $sample: { size: 1 }
+    }]).then(([ planet ]) => planet._id);
+};
+
 module.exports = mongoose.model('Planet', planetSchema);

lib/models/race.js

@@ -1,5 +1,6 @@
 const mongoose = require('mongoose');
 const Schema = mongoose.Schema;
+const User = require('./user');
 
 const raceSchema = new Schema({
     planet: {
@@ -27,4 +28,37 @@ const raceSchema = new Schema({
     }
 });
 
+raceSchema.statics.getExpiredRaces = function() {
+    // shouldn't these be "less than" current date?
+    return this.find({active: true, endTime: {$lt: new Date()}});
+};
+
+raceSchema.statics.getAvailableRaces = function() {
+    // Don't they need to be active too?
+    return this.find({active: true, endTime: {$gt: new Date()}});
+};
+
+raceSchema.statics.findByIdAndAddUser = function(userId) {
+    // can't register twice, use $addToSet
+    return this.findByIdAndUpdate(this.id, { $addToSet: { users: userId } }, { new : true } )
+        .lean()
+        .then(race => {
+            if(!race) throw { code: 404, message: 'Incorrect id' }; 
+            return race;
+        });
+};
+
+raceSchema.methods.cancel = function() {
+    return Promise.all([
+        this.remove(),
+        User.findByIdAndRemove(this.users[0])
+    ]);
+};
+
+raceSchema.methods.complete = function(winner) {
+    this.active = false;
+    this.winner = winner;
+    return this.save();
+};
+
 module.exports = mongoose.model('Race', raceSchema);

lib/models/user.js

@@ -26,4 +26,12 @@ userSchema.methods.comparePassword = function(pass) {
     return bcrypt.compareSync(pass, this.hash);
 };
 
+userSchema.statics.findByIdAndWinPrize = function(userId, prize) {
+    // Retrieve, adjust, and save same user object
+    this.findById(userId).then(user => {
+        user.bankroll += prize;
+        return user.save();
+    });
+};
+
 module.exports = mongoose.model('User', userSchema);

lib/models/vehicle.js

@@ -8,6 +8,7 @@ const vehicleSchema = new Schema ({
         type: Number,
         required: true
     },
+    // shouldn't this be a number???
     max_atmosphering_speed: RequiredString
 });
 

lib/routes/characters.js

@@ -5,7 +5,7 @@ const Character = require('../models/character');
 router
     .get('/', validateAuth, (req, res, next) => {
         Character.find().lean()
-            .then(found => res.json(found))
+            .then(characters => res.json(characters))
             .catch(next);
     });
 

lib/routes/planets.js

@@ -4,7 +4,7 @@ const validateAuth = require('../utils/validateAuth')();
 
 router 
     .get('/', validateAuth, (req, res, next) => {
-        return Planet.find()
+        return Planet.find().lean()
             .then( found => {
                 return res.json(found);
             })

lib/routes/races.js

@@ -8,14 +8,22 @@ router
             .then(race => res.json(race))
             .catch(next);
     })
+
     .get('/', (req, res, next) => {
-        Race.find()
+        // nice job on query, select, populate for this find.
+        // I moved get race login into Race model
+        Race.getAvailableRaces()
             .select('name _id endTime')
-            .where('endTime').gt(Date.parse(new Date))
             .populate({path: 'planet', select: 'name'})
             .lean()
             .then(race => res.json(race))
             .catch(next);
+    })
+
+    .put('/:id/users', validateAuth, (req, res, next) => {
+        return Race.findByIdAndAddUser(req.params.id, req.user.id)
+            .then(race => res.json(race))
+            .catch(next);
     });
 
 module.exports = router;

lib/routes/users.js

@@ -13,7 +13,9 @@ module.exports = router
         if (!password) {
             throw { code: 401, error: 'password required' };
         }
-
+        // because you're not enforcing name uniqueness, 
+        // once second user chooses same name, one of the two accounts
+        // will be forever broken :(
         const user = new User(req.body);
         user.bankroll = 50000;
         user.generateHash(password);
@@ -40,81 +42,59 @@ module.exports = router
             .catch(next);
     })
 
-    .put('/getchar/:id', validateAuth, (req, res, next) => {
-        let userId = null;
-        const token = req.get('Authorization');
-        return tokenService.verify(token)
-            .then( payload => {
-                userId = payload.id;
-                return User.findById(userId).lean();
-            })
-            .then( found => {
-                if (found.character) throw {code: 400, error: 'you already have a character'};
-                return User.findByIdAndUpdate(userId, { character: req.params.id }, {new: true}).select('name email character vehicle bankroll');
-            })
-            .then( updated => {
-                res.json(updated);
-            })
-            .catch(next);
-    })
+    // not sure why you are re-authenticating user when validateAuth
+    // has already done this work. The middleware puts the payload on req.user
 
-    .put('/clearchar/:id', validateAuth, (req, res, next) => {
-        let userId = null;
-        const token = req.get('Authorization');
-        return tokenService.verify(token)
-            .then( payload => {
-                userId = payload.id;
-                return User.findByIdAndUpdate(userId, {character: null}, {new: true});
-            })
-            .then( updated => {
-                res.json(updated);
-            })
+    // REST paths are resources, meaning nouns. 
+    // "getchar" is neither a noun, nor does it match a PUT
+    // A user has a subresource of a character.
+    .put('/character/:id', validateAuth, (req, res, next) => {
+        // "PUT" means put this resource in this place. So doesn't make sense
+        // to throw an error if they already have a character, just replace
+        return User
+            .findByIdAndUpdate(req.user.id, { character: req.params.id }, { new: true })
+            .select('name email character vehicle bankroll')
+            .then(updated => res.json(updated))
             .catch(next);
     })
 
-    .put('/getvehicle/:id', validateAuth, (req, res, next) => {
-        let userId = null;
-        let userBankroll = null;
-        let savedVehicle = null;
-        const token = req.get('Authorization');
-        return tokenService.verify(token)
-            .then( payload => {
-                userId = payload.id;
-                return User.findById(userId).lean();
-            })
-            .then( found => {
-                userBankroll = found.bankroll;
-                return Vehicle.findById(req.params.id);
-            })
-            .then( found => {
-                savedVehicle = found;
-                if(savedVehicle.cost_in_credits > userBankroll) throw {code: 400, error: 'insufficient funds'};
-                userBankroll -= savedVehicle.cost_in_credits;
-                let updated = {
-                    vehicle: savedVehicle.id,
-                    bankroll: userBankroll
-                };
-                return User.findByIdAndUpdate(userId, updated, {new: true})
-                    .select('name email character vehicle bankroll');
-            })
-            .then( updated => {
-                res.json(updated);
-            })
-            .catch(next);
-    })
+    // Doesn't make sense to clear a character, wouldn't you just replace?
 
-    .put('/joinRace/:id', validateAuth, (req, res, next) => {
-        let userId = null;
-        const token = req.get('Authorization');
-        const id = req.params.id;
-        return tokenService.verify(token)
-            .then( payload => {
-                userId = payload.id;
-                return Race.findByIdAndUpdate(id, {$push: {users: userId}}, {new : true} ).lean();
+    .put('/vehicle/:id', validateAuth, (req, res, next) => {
+        Promise.all([
+            User.findById(req.user.id),
+            Vehicle.findById(req.params.id)
+        ])
+            .then(([user, vehicle]) => {
+                // TODO: throw error if no vehicle
+                const cost = vehicle.cost_in_credits;
+                if(cost > user.bankroll) throw {code: 400, error: 'insufficient funds'};
+
+                user.bankroll -= cost;
+                user.vehicle = vehicle._id;
+                return user.save();
             })
-            .then( race => {
-                if(!race) next ({ code: 404, message: 'Incorrect id' }); 
-                else res.json(race); 
+            .then(({ _id, name, email, character, vehicle, bankroll }) => {
+                // bit of a hack, but avoids second trip to database
+                return { _id, name, email, character, vehicle, bankroll };
             })
+            .then(updated => res.json(updated))
             .catch(next);
-    });
+    });
+
+// this needs to live on race route, not user
+// .put('/joinRace/:id', validateAuth, (req, res, next) => {
+//     let userId = null;
+//     const token = req.get('Authorization');
+//     const id = req.params.id;
+//     return tokenService.verify(token)
+//         .then( payload => {
+//             userId = payload.id;
+//             return Race.findByIdAndUpdate(id, {$push: {users: userId}}, {new : true} ).lean();
+//         })
+//         .then( race => {
+//             if(!race) next ({ code: 404, message: 'Incorrect id' }); 
+//             else res.json(race); 
+//         })
+//         .catch(next);
+// });

lib/routes/vehicles.js

@@ -5,7 +5,7 @@ const Vehicle = require('../models/vehicle');
 router
     .get('/', validateAuth, (req, res, next) => {
         Vehicle.find().lean()
-            .then(found => res.json(found))
+            .then(vehicles => res.json(vehicles))
             .catch(next);
     });
 

lib/scripts/create-race.js

@@ -2,33 +2,43 @@ const Race = require('../models/race');
 const Planet = require('../models/planet');
 const User = require('../models/user');
 
+// My making this explicit, next developer doesn't have to reverse engineer how it works
+const DISTANCE_MULTIPLIER = 12;
+const MIN = 100;
+const MAX = 10000;
+const getRandomDistance = () => {
+    return (Math.floor(Math.random() * (MAX - MIN + 1)) + MIN) * DISTANCE_MULTIPLIER;
+};
+
 module.exports = function () {
     console.log('Creating new race...');// eslint-disable-line
-    let enemy = {
+
+    // Why not pull from Character using $sample as well? 
+    const enemy = {
         name: 'Your rival',
         email: 'finalboss@aol.com',
         bankroll: 0
     };
-    let date = new Date();
-    date.setSeconds(date.getSeconds() + (process.env.CREATETIMER || 1000));
+
+    // these are parallel actions!
+    return Promise.all([
+        new User(enemy).save(),
+        Planet.getRandomPlanetId()
+    ])
+        .then(([enemy, planet]) => {
+            // move date to where it is used
+            let date = new Date();
+            date.setSeconds(date.getSeconds() + (process.env.CREATETIMER || 1000));        
 
-    return new User(enemy).save()
-        .then((got) => {
-            enemy = got;
-            return Planet.aggregate([{
-                $sample: {
-                    size: 1
-                }
-            }]);
-        })
-        .then(myPlanet => {
-            const randomNumber = (Math.floor(Math.random() * (10000 - 100 + 1)) + 100) * 12;
+            // I made this a function to better explain what is going on here
+            const distance = getRandomDistance();
             const newRace = {
-                planet: myPlanet[0]._id,
+                // handle pruning in actual find if possible
+                planet,
                 endTime: date,
                 active: true,
-                distance: randomNumber,
-                prize: randomNumber / 6,
+                distance,
+                prize: distance / 6,
                 users: [enemy]
             };
             return new Race(newRace).save();