bubble sort in livescript #213
Conversation
| # which sorts given array (with numbers) in place | ||
| bubbleSort = (arr) !-> | ||
| # determine ending (0-based) index of the array | ||
| if (end = arr.length - 1) < 1 |
There was a problem hiding this comment.
I'm not the biggest fan of this approach, it just obfuscates what you're actually trying to do. If you just made the check:
if arr.length < 2 returnYou wouldn't need the comment at all, it would be self-explanatory. Keep in mind, most of the time we're aiming for readability
There was a problem hiding this comment.
it does two operations.. assigning and checkup.
ppl "should" understand operator/expression precedence order in programming, right? let them solve, it's only for learning some stuff
There was a problem hiding this comment.
I know what it does, that's not the point. This is a learning resource so we should aim for nice, clear and readable code. The rest of this PR fit those criteria perfectly, this line did not.
| # which is determined by the way values are compared | ||
| if arr[i] > arr[j] | ||
| # values are moved up by swapping, | ||
| # temporary variable is used to do the swap |
There was a problem hiding this comment.
This comment is unnecessary, we can see that a temporary variable is used
| # the sorting makes sense only for 2 | ||
| # or more elements... | ||
| return | ||
| # iterate the array in reverse order, |
There was a problem hiding this comment.
This comment also just explains what is already very clear when reading the code. You can assume most people are familiar with for loops, it doesn't need a separate comment
There was a problem hiding this comment.
it's a book, so i thought why not) i do that for myself (writing "stupid" comments) at regular basis just to align code, dont like empty lines in-between blocks
Gathros added the Implementation 
| By the way, welcome to the Arcane Algorithm Archive! Thanks for the contribution. What I think about your code is pretty much the same as what @Gustorn already said. If you make these changes, we can add it to the Archive no problem! If you want, you can add your name at the end of the CONTRIBUTORS.md file as well. And since you're the first person to contribute livescript, you could do us a favor and add the language to book.json. You don't have to do the last one, though. |
| {% sample lang="js" %} | ||
| [import:1-11, lang:"javascript"](code/js/bubble.js) | ||
| {% sample lang="ls" %} | ||
| [import:1-11, lang:"livescript"](code/ls/bubble.ls) |
There was a problem hiding this comment.
You didn't adjust the line numbers. They should be 3-24 instead of 1-11 to include the entire function. Keep in mind that you will have to adjust these numbers again after you made the changes requested by @Gustorn!
There was a problem hiding this comment.
okay, i didnt know what those import meant, that's why copied as in javascript version.
There was a problem hiding this comment.
It's okay. I'm here to help ;)
| Never! ))) |
| I will actually block this merge until the commit names are respectable. Not because I have any problem with you personally but because it sets a bad precedent. In case @leios or @Butt4cak3 wants to veto me, go ahead and I'll approve. |
| I would like better commit names as well. What they are like now, is horrible. |
| @Gustorn I won't stop you. This is a community project, so a certain willingness to cooperate should be expected. |
| It has been over a week and there was not a single sign of cooperation. So... @Gustorn, it's your call to do whatever you deem necessary. |
| Thank you for your contribution, unfortunately I don't think I can merge this in good conscience with the current commit names. Feel free to either message me here or open a new PR if you decide to follow the advice. |
5 participants
No description provided.