Code review feedback

bin/mkapplink.js

@@ -1,5 +1,7 @@
 #!/usr/bin/env node
 
+// OB/TL: this whole folder is dead code
+
 'use strict';
 
 // 'bin/setup' is a symlink pointing to this file, which makes a

comments.md

@@ -0,0 +1,6 @@
+- Linter?
+- Watch out for functions that don't return anything
+- Remove dead code
+- Remove dead code
+- More "catch"-ing
+- Tests?

functions/index.js

@@ -5,19 +5,22 @@ const admin = require('firebase-admin');
 admin.initializeApp(functions.config().firebase);
 const _ = require('lodash');
 
+// OB/TL: comment this file with exactly what you just told me
 exports.getSongId = functions.https.onRequest((req, response) => {
+  // OB/TL: consider splitting me up into smaller corporations
   let reqSong = req.body;
   const userToken = req.body.userToken;
   const urlTitle = getURL(req.body.title);
   const urlArtist = getURL(req.body.artist);
-  let foundId;
+  let foundId; // <= OB/TL: this let is no longer necessary
   admin.database().ref(`/songs/${urlTitle}/${urlArtist}`).once('value')
     .then(snapshot => {
       let id = snapshot.child(reqSong.service).val();
       if (id) {
         console.log('found id', id);
         response.send(id.toString());
       } else if (reqSong.service === 'appleId') {
+        // OB/TL: nested promise chains
         axios.get(makeiTunesSongQuery(reqSong.title, reqSong.artist))
           .then(res => res.data)
           .then(json => {
@@ -27,6 +30,11 @@ exports.getSongId = functions.https.onRequest((req, response) => {
           })
           .catch(console.error);
       } else {
+        /* OB/TL: can also do `axios.get(url, {
+          params: {
+            term: 'some string'
+          }
+        }) */
         axios.get(makeSpotifySongQuery(reqSong.title, reqSong.artist), {
             headers: {
               Authorization: `Bearer ${userToken}`
@@ -46,6 +54,7 @@ exports.getSongId = functions.https.onRequest((req, response) => {
     .catch(console.error);
 });
 
+// OB/TL: dead code shouldn't be in master!
 // exports.savePlayistToSpotify = functions.https.onRequest((req, res) => {
 //   //given a DB playlist ID and spotify user token, saves the playlist from the database to spotify
 //   const userToken = req.body.userToken;
@@ -79,6 +88,7 @@ exports.getSongId = functions.https.onRequest((req, response) => {
 //     });
 // });
 
+// OB/TL: comment describing what this does / how / why
 exports.sentPendingWatch = functions.database.ref(`/users/{uid}/pending`).onWrite(function (event) {
   let uid = event.params.uid;
   let pending = Object.keys(event.data.val());
@@ -123,6 +133,8 @@ function makeSpotifySongQuery(songTitle, songArtist) {
 }
 
 function getURL(str) {
+  // OB/TL: regexes are expensive to create, define this above if possible
+  // OB/Tl: ...but instead here, just use a string '.'
   return encodeURIComponent(str).replace(/\./g, function (cha) {
     return '%' + cha.charCodeAt(0).toString(16);
   });

js/Routers/index.js

@@ -1,3 +1,4 @@
+// OB/TL: split up into multiple files
 import React from 'react';
 import {
   TabNavigator,

js/components/friends/friendRequests.js

@@ -71,7 +71,7 @@ export default class FriendRequests extends Component {
                         <Text>{friend.fullname}</Text>
                         <Text note >@{friend.username}</Text>
                       </Body>
-
+                        {/* OB/TL: consider moving styles to file (like in other places) */}
                         <Button small style={{backgroundColor: "#FC642D", margin: 5}} onPress={() => this.acceptRequest(friend.userId)}>
                                 <Icon name="ios-add" />
                           </Button>

js/components/playlists/myPlaylists.js

@@ -54,6 +54,7 @@ export default class MyPlaylists extends Component {
   };
 
   componentDidMount() {
+    // OB/TL: unnecessary `Promise.resovle`s
     const currentUser = firebase.auth().currentUser.uid;
     Promise.resolve(this.getUserPlaylists(currentUser)).then(playlistArr => {
       this.setState(
@@ -76,6 +77,7 @@ export default class MyPlaylists extends Component {
   }
 
   render() {
+    // OB/TL: dead code
     console.log(this.state.playlists)
     return (
       <Container>

js/components/profile/index.js

@@ -328,6 +328,7 @@ export default class Profile extends Component {
                 }
 
                 {this.state.token ?
+                  // OB/TL: maybe this an be condensed via refactoring
                 <SwipeRow
                   rightOpenValue={-75}
                   body={
@@ -462,7 +463,7 @@ export default class Profile extends Component {
                 </CardItem>
               </Card>
             </Content>
-          : <Spinner color="#FC642D" />}
+          : <Spinner color="#FC642D" /> /* OB/TL: invert this ternary */}
       </Container>
     );
   }

js/components/profile/updatePassword.js

@@ -15,6 +15,7 @@ import {
 } from 'native-base';
 import styles from '../login/style';
 import { Field, reduxForm } from 'redux-form';
+// OB/TL: dead dependency
 
 export default class UpdatePassword extends Component {
   render() {

utils/database.js

@@ -2,12 +2,13 @@ import * as firebase from 'firebase';
 import axios from 'axios';
 
 export default class Database {
-  static savePlaylistToDatabase(playlists, providerId) {
+  static savePlaylistToDatabase(playlists, providerId) { // OB/TL: naming here
+    // OB/TL: be wary when you're not returning anything
     const currentUser = firebase.auth().currentUser;
     playlists.forEach(playlist => {
       let newSong = {};
       playlist.songs.forEach((song, index) => {
-        this.findOrCreateSong(song, providerId);
+        this.findOrCreateSong(song, providerId); // <= OB/TL non-blocking
         newSong[index] = {};
         newSong[index].artist = song.artist;
         newSong[index].title = song.title;
@@ -32,6 +33,8 @@ export default class Database {
   static getAllFriends() {
     let user = firebase.auth().currentUser;
     return firebase.database().ref(`/users/${user.uid}/friends`).once('value');
+    // OB/TL: consider using `.on` for realtime updates!
+    // OB/TL: consider chaining off this promise to resolve to the snapshot value
   }
 
   static getPendingFriends() {
@@ -40,6 +43,7 @@ export default class Database {
   }
 
   static requestFriend(recievingUser, sendBack) {
+    // OB/TL: watch out, no return value
     let user = firebase.auth().currentUser;
     firebase
       .database()
@@ -61,12 +65,12 @@ export default class Database {
     firebase
       .database()
       .ref(`/users/${user.uid}/playlists/${playlistId}`)
-      .set('original');
+      .set('original'); // <= OB/TL: might want to set to true/false for performance
   }
 
   //get playlist from id
   static getPlaylistFromId(pid) {
-    return Promise.resolve(
+    return Promise.resolve( // OB/TL: `Promise.resolve` unecessary her
       firebase.database().ref(`/playlists/${pid}`).once('value')
     );
   }
@@ -143,14 +147,15 @@ export default class Database {
     firebase.database().ref(`/users/${friend}/friends/${user.uid}`).remove();
   }
 
-  static ignoreMe() {
+  static ignoreMe() { // OB/TL: dead code!
     let user = firebase.auth().currentUser;
     firebase
       .database()
       .ref(`/users/${user.uid}/pending`)
       .on('child_added')
       .then(snapshot => {
         let pending = snapshot.val();
+        // OB/TL: nested promise chains
         firebase
           .database()
           .ref(`/users/${user.uid}/sent`)
@@ -175,6 +180,7 @@ export default class Database {
           });
       });
   }
+  // OB/TL: dead code
   static saveApplePlaylists(playlists, providerId) {
     playlists.forEach(playlist => {
       let newSong = {};
@@ -216,6 +222,7 @@ export default class Database {
   }
 
   static saveMultiPlaylists(playlists, providerId) {
+    // OB/TL: global? use let or const (or var)
     for (playlistName in playlists) {
       if (playlists.hasOwnProperty(playlistName)) {
         const playlist = playlists[playlistName];
@@ -271,6 +278,7 @@ export default class Database {
     } catch (err) {
       console.log(err);
       alert(err);
+      // OB/TL: maybe toast
     }
   }
 
@@ -293,6 +301,8 @@ export default class Database {
           .database()
           .ref(`playlists/${databasePlaylistId}`);
         let external = [];
+        // OB/TL: possible control flow / async issues in this function
+        // OB/TL: flatten promise chains, consider splitting into multiple functions
         firedata.on('value', function(snapshot) {
           const playlist = snapshot.val();
           console.log('Importing: ', playlist);