Cleanup bubble sort code for C++ #244

mika314 · 2018-07-12T05:26:51Z

remove random generator function
use nested loop instead of is_sorted var

Butt4cak3 · 2018-07-12T11:10:25Z

I don't have much to do with the C and C++ code in this project, but I know that most other language implementations (C#, Java, JavaScript, Julia, Matlab, Python, Racket?, Rust, Swift) have no optimization whatsoever. The chapter also talks about the pure, unoptimized version of it, so I don't know why it's there in the first place.

And that's it for the simplest bubble sort method. Now, as you might imagine, computer scientists have optimized this to the fiery lakes of Michigan and back, so we'll come back to this in the future and talk about how to optimize it. For now, it's fine to just bask in the simplicity that is bubble sort.

I think all implementations should follow the same procedure, so I would actually vote to remove the optimization from all implementations to make it match both the chapter text and the Julia version, which sort of serves as the reference implementation for all chapters.

Other than that, there was no reason to remove the random generator function. Random inputs to test the programs are fine.

I also don't understand why you felt the need to rename the .cpp file. That seems to me like a purely preferential thing. We don't have a convention for how to name source files and I don't think we need one. In addition to that, you renamed it so that Git wasn't even able to track it. For future reference, you should try to always move files with git mv to avoid that.

I think that's all I have to say.

mika314 · 2018-07-12T20:53:26Z

My PR actually closer to Julia implementation, Julia implementation does not have is_sorted var. I removed random generation because implementation is confusing for inexperienced programmers and does not provide to much value. I renamed the file to make the name consistent with c implementation.

Butt4cak3 · 2018-07-12T21:30:58Z

It's different, not closer. It doesn't have the is_sorted variable, but it has a third nested loop, which the Julia implementation also doesn't have. In addition to that, I'm pretty sure your implementation is even slower than the standard one without any optimization.

The implementation of the random generator is not subject of this chapter. It doesn't really matter as long as it works.

Most of my other points still stand, but I'll let someone who actually knows C and C++ go ahead and properly review this.

mika314 · 2018-07-13T03:08:22Z

Third nested loop is just a bug. I pushed updated version.

fix the bug with netsted loop
renamed back bubblesort.cpp
match algorithm with Jilia

zsparal · 2018-07-13T06:46:39Z

chapters/algorithms/bubble_sort/code/c++/bubblesort.cpp

-        swap(*it, *(it + 1));
-        is_sorted = false;
+      if (*(it2 + 1) < *it2) {
+        std::swap(*it2, *(it2 + 1));


You could use std::iter_swap(it2, it2 + 1);

here commit "Use iter_swap instead of swap 6e44fdf"

mika314 · 2018-07-15T16:19:44Z

BUMP

leios · 2018-07-15T21:19:52Z

Hey @AntonTe, sorry for taking so long to get to this PR. For the record BUMPs are not really necessary and lead to spam for everyone following the algorithm archive. Please avoid using them in the future, if you can. We have a lot of PR's and are getting to them when we have time.

As far as this PR is concerned, it is difficult to review because it is basically an update to your personal preference here, which means we need to discuss style. It is hard to do so for both C and C++ at the same time.

In this case, the C++ code is notably cleaner; however, I am not sure about the C code. Would it be possible to split the PR into 2?

mika314 · 2018-07-16T05:06:30Z

This PR now is only C++.
C implemenation you can find here: #254

mika314 · 2018-07-20T05:19:26Z

@Gustorn can you please re-approve this PR, because I rebased it to fix automerge

Butt4cak3 · 2018-07-21T07:56:56Z

contents/bubble_sort/bubble_sort.md

@@ -14,6 +14,8 @@ This means that we need to go through the vector $$\mathcal{O}(n^2)$$ times with
 [import:9-27, lang:"csharp"](code/csharp/BubbleSort.cs)
 {% sample lang="c" %}
 [import:11-21, lang:"c_cpp"](code/c/bubble_sort.c)
+{% sample lang="cpp" %}


There is already a C++ code import further down. No need to create another one. In fact, this causes the page to render incorrectly.

Should be good now.

- remove random generator function - use nested loop instead of is_sorted var - match the bubble sort C++ algorithm with Julia

Butt4cak3 · 2018-07-22T10:51:52Z

The import propery works now. For the future, please don't amend your commits and force push after you made requested changes. It's helpful for us as reviewers to see only what you changed after the review, or else we have to look through the entire code again.

Since @Gustorn already approved the code itself, I will go ahead and merge this one.

mika314 · 2018-07-23T05:33:54Z

@Butt4cak3 Got it, from now I'll make commits without squish and rebase.

Gathros added the Implementation Edit This provides an edit to an algorithm implementation. (Code and maybe md files are edited.) label Jul 12, 2018

mika314 force-pushed the bubble_sort branch from ed44ffa to fb1207f Compare July 13, 2018 03:05

zsparal suggested changes Jul 13, 2018

View reviewed changes

mika314 force-pushed the bubble_sort branch 2 times, most recently from 737a20f to 8da692c Compare July 16, 2018 04:57

mika314 changed the title ~~Cleanup bubble sort code for C and C++~~ Cleanup bubble sort code for C++ Jul 16, 2018

Butt4cak3 mentioned this pull request Jul 16, 2018

Rework Bubble Sort C++ #251

Closed

mika314 force-pushed the bubble_sort branch from 8da692c to 536653f Compare July 19, 2018 05:59

zsparal previously approved these changes Jul 19, 2018

View reviewed changes

mika314 dismissed zsparal’s stale review via 1b69439 July 20, 2018 05:17

mika314 force-pushed the bubble_sort branch from 536653f to 1b69439 Compare July 20, 2018 05:17

zsparal previously approved these changes Jul 20, 2018

View reviewed changes

Butt4cak3 requested changes Jul 21, 2018

View reviewed changes

Cleanup bubble sort code for C++

4177dd6

- remove random generator function - use nested loop instead of is_sorted var - match the bubble sort C++ algorithm with Julia

mika314 dismissed zsparal’s stale review via 4177dd6 July 21, 2018 21:22

mika314 force-pushed the bubble_sort branch from 1b69439 to 4177dd6 Compare July 21, 2018 21:22

Merge branch 'master' into bubble_sort

06f0d9e

Butt4cak3 approved these changes Jul 22, 2018

View reviewed changes

Butt4cak3 merged commit 3783a49 into algorithm-archivists:master Jul 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Cleanup bubble sort code for C++ #244

Cleanup bubble sort code for C++ #244

Uh oh!

mika314 commented Jul 12, 2018

Uh oh!

Butt4cak3 commented Jul 12, 2018 •

edited

Loading

Uh oh!

mika314 commented Jul 12, 2018

Uh oh!

Butt4cak3 commented Jul 12, 2018

Uh oh!

mika314 commented Jul 13, 2018

Uh oh!

zsparal Jul 13, 2018

Uh oh!

mika314 Jul 13, 2018

Uh oh!

mika314 commented Jul 15, 2018

Uh oh!

leios commented Jul 15, 2018

Uh oh!

mika314 commented Jul 16, 2018

Uh oh!

mika314 commented Jul 20, 2018 •

edited

Loading

Uh oh!

Butt4cak3 Jul 21, 2018

Uh oh!

mika314 Jul 21, 2018

Uh oh!

Butt4cak3 commented Jul 22, 2018

Uh oh!

mika314 commented Jul 23, 2018

Uh oh!

Uh oh!

Uh oh!

Cleanup bubble sort code for C++ #244

Cleanup bubble sort code for C++ #244

Uh oh!

Conversation

mika314 commented Jul 12, 2018

Uh oh!

Butt4cak3 commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mika314 commented Jul 12, 2018

Uh oh!

Butt4cak3 commented Jul 12, 2018

Uh oh!

mika314 commented Jul 13, 2018

Uh oh!

zsparal Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

mika314 Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

mika314 commented Jul 15, 2018

Uh oh!

leios commented Jul 15, 2018

Uh oh!

mika314 commented Jul 16, 2018

Uh oh!

mika314 commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Butt4cak3 Jul 21, 2018

Choose a reason for hiding this comment

Uh oh!

mika314 Jul 21, 2018

Choose a reason for hiding this comment

Uh oh!

Butt4cak3 commented Jul 22, 2018

Uh oh!

mika314 commented Jul 23, 2018

Uh oh!

Uh oh!

Butt4cak3 commented Jul 12, 2018 •

edited

Loading

mika314 commented Jul 20, 2018 •

edited

Loading