Fix pick type result not showing issue. #21
Conversation
There was a problem hiding this comment.
Remember to test the code, if I'm not wrong you have some syntax errors in here that should make it not run.
Otherwise fine job on the addition of getWinner for pick elections.
For ballot.coffee thanks for spotting my bug with having removed the i variable. I think it's not the most elegant or best software-development implementation of solving that though, look at my comments in the actual code.
| @@ -367,10 +367,12 @@ Ballots.after.insert((userId, ballot) -> | |||
| return | |||
| user = User.fetchOne(userId) | |||
| toIncrement = {} | |||
|
|
|||
| i = 0 | |||
There was a problem hiding this comment.
Ahh, nice catch, I can see that when refactoring here https://github.com/billg1990/nyu-vote/commit/00c41cf3d35db2bb6bbfa2f284aa197543440118#diff-fa24aef3a316c83d6f7b8c08f600a667 I changed a for loop with an i into a python-style loop, and I hadn't noticed the i was needed. Curious though, because I did this last August and it hasn't shown up until now! Maybe there weren't any pick elections since it went live?
There was a problem hiding this comment.
I think a cleaner way though, is actually do just do what was done before I changed it in the above linked to changes on line 362 and 363, I really don't like the i++ in the middle of this loop, changes the value of i right in the middle of the code, could lead to a lot of future nasty bugs. Could we just quickly change that?
| for question in ballot.questions | ||
| if question?.options?.type != "pick" | ||
| if Election.fetchOne(ballot.electionId).questions[0].options.type != "pick" |
There was a problem hiding this comment.
I must say I have no idea why you did this. For one there's an obvious bug in the fact that it constantly checks only questions[0] so if the first question is not pick none of the following questions will be looked at as this code will run every time, and if the first one is pick it'll run for all. It would work with questions[i] but you've fetched a question that we already have locally in the loop and also removed the ?. which in my opinion is one of the nicest features in coffee script. Can we just simply remove this change?
Unless of course you have a good argument for it that I'm missing
| @@ -83,6 +83,25 @@ Template.electionsAdminResults.helpers | |||
| return information | |||
| getWinner: () -> | |||
| questionId = @_id | |||
| election = Election.getActive() | |||
| question = getQuestion(questionId) | |||
There was a problem hiding this comment.
This should throw an error as questionId is not defined right? You need to pass it in as a function argument like for example votes is in the corresponding html file in the same folder <tr><td>Votes</td><td>{{votes _id ../_id ../../votes }}</td></tr> and of course declare that argument in this function as well
| ## get votes for the question | ||
| winner = "" | ||
| winnerscore = 0 | ||
| for v in question.choices |
There was a problem hiding this comment.
Maybe name the variable a bit more descriptively? Like for example choice instead of v
Hi, a new pull request. Please review, and edit it.