diff --git a/bin/mkapplink.js b/bin/mkapplink.js index 3f7d7cb..63d3f02 100644 --- a/bin/mkapplink.js +++ b/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 diff --git a/comments.md b/comments.md new file mode 100644 index 0000000..6f59760 --- /dev/null +++ b/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? diff --git a/functions/index.js b/functions/index.js index ab2ecc3..4608198 100644 --- a/functions/index.js +++ b/functions/index.js @@ -5,12 +5,14 @@ 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(); @@ -18,6 +20,7 @@ exports.getSongId = functions.https.onRequest((req, response) => { 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); }); diff --git a/js/Routers/index.js b/js/Routers/index.js index 60347d9..56b7065 100644 --- a/js/Routers/index.js +++ b/js/Routers/index.js @@ -1,3 +1,4 @@ +// OB/TL: split up into multiple files import React from 'react'; import { TabNavigator, diff --git a/js/components/friends/friendRequests.js b/js/components/friends/friendRequests.js index 0d357f0..3886e48 100644 --- a/js/components/friends/friendRequests.js +++ b/js/components/friends/friendRequests.js @@ -71,7 +71,7 @@ export default class FriendRequests extends Component { {friend.fullname} @{friend.username} - + {/* OB/TL: consider moving styles to file (like in other places) */} diff --git a/js/components/playlists/myPlaylists.js b/js/components/playlists/myPlaylists.js index c35efa5..44b4465 100644 --- a/js/components/playlists/myPlaylists.js +++ b/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 ( diff --git a/js/components/profile/index.js b/js/components/profile/index.js index cf08fd0..c455d8f 100644 --- a/js/components/profile/index.js +++ b/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 - : } + : /* OB/TL: invert this ternary */} ); } diff --git a/js/components/profile/updatePassword.js b/js/components/profile/updatePassword.js index 7ffeba4..166345e 100644 --- a/js/components/profile/updatePassword.js +++ b/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() { diff --git a/utils/database.js b/utils/database.js index 072928e..a73f6fe 100644 --- a/utils/database.js +++ b/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,7 +147,7 @@ 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() @@ -151,6 +155,7 @@ export default class Database { .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);