omnibus issue: code review feedback

[shawndrost]

Overall, great work! My comments are mostly cosmetic, except the ones marked MAJOR.

• copia/server.js

  Lines 32 to 54 in 2f0dcf2

  	// Connection event handlers
  	mongoose.connection.on('open', function() {
  	console.log('Database opened', config.db.uri, ' with options ', config.db.options);
  	app.available = true;
  	});
  	mongoose.connection.on('error', function(err) {
  	console.log('server.js => Database connection error', err);
  	app.available = false;
  	});
  	mongoose.connection.on('disconnected', function(err) {
  	console.log('server.js => Database lost connection', err);
  	app.available = false;
  	});
  	mongoose.connection.on('connected', function() {
  	console.log('Database regained connection' );
  	app.available = true;
  	});
  	// Connect to database
  	app.db = mongoose.connect(config.db.uri, config.db.options);

  -- maybe move to app/db/init.js
• DONE what's this guy doing here? https://github.com/Copia/copia/blob/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/config/express.js
• add a "reset" section to common.css; remove unused comments
• THE DREADED .DS_Store -- someone needs to update their global .gitignore https://github.com/Copia/copia/blob/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/public/img/.DS_Store
• This is a minor details, but prefer margins to <br>s:

  copia/public/views/index.html

  Line 6 in 2f0dcf2

  <br>

• Unused?: https://github.com/Copia/copia/blob/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/public/views/about.html
• since this rather ugly line repeats, consider a helper function:

  copia/public/views/borrow.html

  Line 4 in 2f0dcf2

  <div class="form-group" ng-class="{ 'has-error' : loanRequestForm.debtAmount.$dirty && loanRequestForm.debtAmount.$invalid, 'has-success' : loanRequestForm.debtAmount.$dirty && !loanRequestForm.debtAmount.$invalid }">

• suggestion: prefer classnames like "primary" or "deemphasize" to "white" in case the color scheme changes later (

  copia/public/views/borrowConfirm.html

  Line 10 in 2f0dcf2

  <h4 class="white">Loan Request Confirmation: </h4>

  )
• careful about relative paths:

  copia/public/views/lend.html

  Line 11 in 2f0dcf2

  <img src="../img/spinner.gif">

• you may be using a script tag where you want a folder with >1 object in it (but I'm genuinely unsure):

  copia/public/views/loanDetails.html

  Lines 48 to 70 in 2f0dcf2

  	<script type="text/ng-template" id="modalConfirmation.html">
  	<div class="modal-header fundingConfirmation">
  	<h3 class='text-center'>Funding Confirmation</h3>
  	</div>
  	<div class="modal-body fundingConfirmation">
  	<p>You agree to fund {{loan.borrower_id.user.display_name}}s request for ${{loan.principal}}.00.</p>
  	<p>{{loan.borrower_id.user.display_name}}s agrees to refund you ${{loan.payback_amount}}.00 within {{loan.payback_days}} of receipt.</p>
  	<br>
  	<strong>Terms of Service</strong>
  	<p>Well, the way they make shows is, they make one show. That shows called a pilot. Then they show that show to the people who make shows, and on the strength of that one show they decide if theyre going to make more shows. Some pilots get picked and become television programs. Some dont, become nothing. She starred in one of the ones that became nothing.</p>
  	</div>
  	<div class="modal-footer fundingConfirmation">
  	<div class="button-group row confirmButtons">
  	<div class='col-xs-6 text-center'>
  	<button class='fundConfButton btn-outline-inverse btn' ng-click='cancelModal()'>Cancel</button>
  	</div>
  	<div class='col-xs-6 text-center'>
  	<button class='fundCancButton btn-outline-inverse btn' ng-click='confirmFunding()'>Confirm Funding</button>
  	</div>
  	</div>
  	</div>
  	</script>

• in general, more subdirectories in /public/views/ would be good, and might help push you towards more RESTful conventions. eg static/, account/, loans/
• remove unused console.log

  copia/public/js/filters.js

  Line 6 in 2f0dcf2

  console.log(loans);

• _.select?

  copia/public/js/filters.js

  Lines 7 to 13 in 2f0dcf2

  	var filtered = [];
  	angular.forEach(loans, function(loan){
  	if(loan.status === 'pending') {
  	filtered.push(loan);
  	}
  	});
  	return filtered;

• looks weird, but may be ok

  copia/public/js/directives.js

  Lines 32 to 33 in 2f0dcf2

  	link: function($scope, iElm, iAttrs, controller) {
  	}

• ngModel.$setValidity('paybackThreshold', loanAmt !== null && loanAmt <= paybackAmt);

  copia/public/js/directives.js

  Lines 32 to 33 in 2f0dcf2

  	link: function($scope, iElm, iAttrs, controller) {
  	}

• MAJOR: I struggled to read the services. Consider creating models, eg in public/js/models/loan.js would simplify https://github.com/Copia/copia/blob/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/public/js/services/borrowService.js
  • the constructor would would get these lines of code out of the service:

    copia/public/js/services/borrowService.js

    Lines 6 to 23 in 2f0dcf2

    	var today = function() {
    	var local = new Date();
    	local.setMinutes(local.getMinutes() - local.getTimezoneOffset());
    	return local.toJSON().slice(0,10);
    	};
    	this.loan = {
    	amount : {
    	loan : null,
    	payback : null
    	},
    	date : {
    	neededBy : today()
    	},
    	paybackDays : null,
    	category : null,
    	reason : null
    	};

  • a method named "reset" would takes the place of this chunk of code:

    copia/public/js/services/borrowService.js

    Lines 31 to 38 in 2f0dcf2

    	clearLoan : function(){
    	self.loan.amount.loan = null;
    	self.loan.amount.payback = null;
    	self.loan.date.neededBy = null;
    	self.loan.paybackDays = null;
    	self.loan.category = null;
    	self.loan.reason = null;
    	},

• it seems like you're half-using the universal self=this convention -- make sure you're consistent. example unused self:

  copia/public/js/services/cookieService.js

  Line 7 in 2f0dcf2

  var self = this;

• you can remove like 20 lines of code by writing a function that maps http responses onto promise resolutions, eg you could write promisify_http(d, $http.get(...)) in place of these chunks:

  • copia/public/js/services/lendService.js

    Lines 14 to 20 in 2f0dcf2

    	$http.get('/users/'+user_id+'/loans', {params: {session_token: session_token}})
    	.success(function(loans, status, headers, config) {
    	d.resolve(loans);
    	})
    	.error(function(data, status, headers, config) {
    	d.reject(data);
    	});

  • copia/public/js/services/lendService.js

    Lines 28 to 34 in 2f0dcf2

    	$http.get('/users/'+user_id+'/loans/'+loan_id, {params: {session_token: session_token}})
    	.success(function(loan, status, headers, config) {
    	d.resolve(loan);
    	})
    	.error(function(data, status, headers, config) {
    	d.reject(data);
    	});

  • etc
• in fact, am I correct in observing that all the service methods in that file are identical except for their urls? can you succinctly abstract that and clarify the code?
• pull these lines into a helper fn. if statements should look like if(isArr(object[key]))

  • copia/public/js/services/sanitize.js

    Line 11 in 2f0dcf2

    if (!Array.isArray(object[key]) && (typeof(object[key]) === 'object') && (object[key].getDate === undefined)){

  • copia/public/js/services/sanitize.js

    Line 13 in 2f0dcf2

    } else if (typeof(object[key]) !== 'string') {

• subdirectories might be good here as well: https://github.com/Copia/copia/tree/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/public/js/controllers
• do you need this? I can't remember. https://github.com/Copia/copia/blob/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/public/js/controllers/about.js
• get rid of unused code. (branches are good places to keep refactor ideas.)

  copia/public/js/controllers/borrow.js

  Lines 64 to 124 in 2f0dcf2

  	//functionality for DEBT NEEDED-BY pop-out calendar
  	//$scope.debtNeededOpened = false; //status of pop-out calendar
  	//$scope.debtNeededMin = new Date(); //earliest debt request is today
  	//var max = new Date();
  	//$scope.debtNeededMax = max.setDate(max.getDate() + 7*4); //latest debt request is 1 months from now
  	//$scope.debtNeededBySelected = false; //
  	/*$scope.$watch('loan.date.neededBy', function(neededBy){
  	//if neededBy is cleared, reset payback date
  	if(neededBy === null) {
  	$scope.debtNeededBySelected = false;
  	$scope.debtDueSelection = null;
  	} else {
  	$scope.debtNeededBySelected = true;
  	}
  	}, true)*/
  	//pop-out on glyphicon-calendar
  	/*$scope.openDebtNeeded = function($event) {
  	$event.preventDefault();
  	$event.stopPropagation();
  	$scope.debtNeededOpened = true;
  	};*/
  	//map drop down list selection to number of days
  	/*var optionsToDays = {
  	'One Week' : 7,
  	'Two Weeks' : 14,
  	'Three Weeks' : 21,
  	'One Month' : 28,
  	'Two Months' : 56,
  	'Three Months' : 84
  	};
  	var daysToOptions = {
  	7 : 'One Week',
  	14 : 'Two Weeks',
  	21 : 'Three Weeks',
  	28 : 'One Month',
  	56 : 'Two Months',
  	84 : 'Three Months'
  	}*/
  	//functionality for DEBT PAYBACK drop down menu
  	/*if($scope.loan.paybackDays === null) {
  	$scope.debtDueSelection = null; //drop down list selection
  	$scope.otherDebtDueSelected = false; //"is Other" option on drop down list selected?
  	} else if($scope.loan.paybackDays in daysToOptions) {
  	$scope.debtDueSelection = daysToOptions[$scope.loan.paybackDays];
  	} else {
  	$scope.debtDueSelection = 'Other (Enter # of days)';
  	}
  	$scope.$watch('debtDueSelection', function(selection){
  	if(selection === 'Other (Enter # of days)') {
  	$scope.otherDebtDueSelected = true; //'Other' option selected, display manual input box for # of days
  	} else {
  	$scope.otherDebtDueSelected = false;
  	$scope.loan.paybackDays = optionsToDays[selection];
  	}
  	}, true);*/

• isn't this unnecessary?

  copia/public/js/controllers/dashboard.js

  Line 20 in 2f0dcf2

  if ($scope.loan.length > 0){

• clean this up somehow:

  copia/public/js/controllers/dashboard.js

  Lines 21 to 36 in 2f0dcf2

  	for (var i = 0; i < $scope.loan.length; i++){
  	if ($scope.loan[i].status === 'pending'){
  	$scope.borrower = true;
  	$scope.pending = true;
  	$scope.borrowStatus = 'Pending';
  	$scope.borrowColor = {'color':'red'};
  	$scope.loanView = $scope.loan[i];
  	}
  	if ($scope.loan[i].status === 'funded'){
  	$scope.pending = false;
  	$scope.borrower = true;
  	$scope.borrowStatus = 'Funded';
  	$scope.borrowColor = {'color':'black'};
  	$scope.loanView = $scope.loan[i];
  	}
  	}

• move this line above line 43 -- it's identical except it fixes a potential bug:

  copia/public/js/controllers/dashboard.js

  Line 46 in 2f0dcf2

  LendRequest.cancelLoan($scope.loan[i]._id, $scope.session_token, $scope.user_id).then(function(){$timeout(function(){$route.reload();},0);});

• also break that line into two lines
• isn't this a bad sign? seems like you want a link. I could be wrong.

  copia/public/js/controllers/index.js

  Lines 7 to 13 in 2f0dcf2

  	$scope.signUp = function(){
  	$location.path( "/signup" );
  	};
  	$scope.signIn = function(){
  	$location.path( "/signin" );
  	};

• kind of a lot of leftover console.logs going on (eg

  copia/public/js/controllers/loan.js

  Lines 54 to 56 in 2f0dcf2

  	console.log('Confirmed from modal');
  	}, function () {
  	console.log('Cancel from modal');

  )
• you can safely omit these kinds of comments:

  copia/public/js/controllers/signin.js

  Lines 21 to 25 in 2f0dcf2

  	//store cookies
  	CookieService.storeCookies(user);
  	//redirect
  	$location.path( "/dashboard" );

• looks like signin and signup are identical except for path. any way to represent that in the code?
  • there's also another line of difference, but presumably (this line)[https://github.com/Copia/copia/blob/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/public/js/controllers/signup.js#L33] is useful in signin as well. that error is representative of the reason you'd want to merge these files somehow.
• can you change the client such that (this line)[https://github.com/Copia/copia/blob/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/app/controllers/loans.js#L29] isn't necessary?
• (this)[https://github.com/Copia/copia/blob/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/app/controllers/loans.js#L54] will fail, as (this)[https://github.com/Copia/copia/blob/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/app/controllers/loans.js#L62] will have already run.
• move to L56 or remove:

  copia/app/controllers/loans.js

  Line 48 in 2f0dcf2

  var addedKarma = loan.principal;

• maybe not 400:

  copia/app/controllers/transactions.js

  Line 45 in 2f0dcf2

  return res.send(400, 'transaction.js/create => Error, lender venmo account not available (loan funded?). Lender:' + lender);

• MAJOR: (this file)[https://github.com/Copia/copia/blob/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/app/controllers/transactions.js] should probably not deal with venmo, http, and mongoose -- it's just too much different stuff. maybe extend the transaction model to contain all the venmo detail work?
• replace these lines with }, cb); --

  copia/app/lib/venmoAPI.js

  Lines 18 to 20 in 2f0dcf2

  	}, function(err, response, body) {
  	cb(err, response, body);
  	});

• this line does nothing:

  copia/app/models/transaction.js

  Line 66 in 2f0dcf2

  return ;

• MAJOR: it's not clear what the difference is between app/services and app/controllers? they seem to have overlapping responsibilities.
• (indent)[https://github.com/Copia/copia/blob/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/app/services/transaction_service.js#L3-L5]
• (this)[https://github.com/Copia/copia/blob/2f0dcf2abd35b52b8ca4bad94c8cf225a32d4f11/app/services/user_service.js#L71] would be better done on the client side to reduce duplicate data transmission.