search

sampottinger / co_opencampaigndata

API service for serving Colorado TRACER data for opencampaigndata.org
GNU General Public License v3.0
10 stars 1 forks source link

Small stylistic cleanup for data_formatters. #9

Closed sampottinger closed 11 years ago

sampottinger commented 11 years ago

Issues #5, #6, and #8 fixed. All small stylistic changes to get data_formatters to conform to Google's JavaScript Style Guidelines as mandated in this project's README. This is in response to the rejection of pull request #7.

Although my committing behavior was a little messy (won't happen again), a code review would be lovely.

That being said, there is still a little inconsistency with regards to newlines and if statements, namely

if { ... } else { ... }

or

if { ... } else { ... }

Google's guidelines appear to be slient on this issue so I suggest it should not hold up the pull request. Although, I would be happy to go one direction or another if we want to insist on that conformity.

trinary commented 11 years ago

Thanks for cleaning up the PR! Generally I have used 2 space tabs but indented to match var declarations at the top of functions. I totally missed serializationStrategies which is really silly, sorry about that. Var naming you are absolutely right about.

Regarding newlines in else/if else statements, I've tended towards Crockford's recommendations. I use jshint instead of jslint to check my code because it is more flexible and adapts to usage better, but Crockford has good points to make about stylistic rigor in order to avoid problems with semicolon insertion, etc. His style guide is here: http://javascript.crockford.com/code.html. When writing c/c++ I was all about Allman brace style, but it just doesn't make sense in JS. :) I vote braces on the same line as else/if else.

:+1: on this PR though!

sampottinger commented 11 years ago

Great! Thanks for the code review. I agree with your comments about the braces but I will open an issue to adopt the standard in the README and take care of that across all existing code. That was an oversight on my part. :-).