Hot answers tagged

14

In all, this is a nice program. In particular, it compiled and ran (almost) flawlessly on Linux, so keep up the good work on portability! Here are some things that may help you improve your program. Fix the bug It's a subtle bug, but there is a problem with the Engine class. The destructor is this: SortVis::Engine::~Engine() { SDL_DestroyWindow(...


12

Since the goal is the best possible implementation of this algorithm, I'd suggest the following. However, faster algorithms do exist. To conform to PEP8 make sure you have two blank lines after your imports and surrounding function definitions. Since, you aren't editing each item, but rather adding and removing items until the list is sorted, I'd use the ...


11

In-place sort Your selection_sort is an in-place sort, so there's no need to return the same list you were given. In fact, returning the list is confusing, because it somewhat implies that you would be returning something different from what you were given. You can just drop the return, here and in similar functions. Failure modes if sublist_increment // ...


10

Adding to @Reinderien's review, here are a few more points: Testing The test code has some repeated statements for every function. It would be better to put that into a for loop like this: sorting_algorithms = [ ("Selection Sort", selection_sort), ... # Wrap shell_sort into a lambda to make it a single-argument function for testing ("Shell ...


10

Code Style Use the standard JavaDoc markup when writing comments. You will be able to generate documentation and it allows you to use ready made tools for documenting stuff like method parameters and related classes. Also people expect to see JavaDoc style comments and are used to reading them, so by rolling out your own style you're adding unnecessary ...


9

Your code looks well-formatted, it's easy to read and to follow, and the explanation you gave matches the code exactly. Well done. :) for item in my_list: This statement looks strange since in the body of this for loop, you neither use item nor my_list. You can express the idea of that code more directly: for _ in range(len(my_list)): The variable _ is ...


9

First thing first, make it into a function, such that your main will be int main(){ const int siz = 6; int arr[siz] = {4,6,3,1,3,8}; std::cout << "Before sort\n"; printarr(arr,siz); sort(arr, siz); std::cout << "After sort\n"; printarr(arr,siz); } Since you've tagged it c++, do not use c-style ...


8

You have done a great job. Here’s my suggestions: Sort the include directives in alphabetical order. Instead of using the generic name Iter, it may be a good idea to express the random access requirement: something like RandomIt. Iterators should be passed by value, not const reference. Don’t be afraid to copy iterators — they are intended to be ...


8

I agree with @Reinderien that this shouldn't be a class. You can see evidence for this in your constructor: def __init__(self, input_list:list)->list: self.input_list = input_list self.__iter__() You are constructing the object (and calling the constructor) simply to call self.__iter__(). There is no reason for the creation of an object here ...


8

let me know what I can improve in this code. For a real-world code review, code should be correct, maintainable, reasonably efficient, and, most importantly, readable. Writing code is a process of stepwise refinement. Here's your code. Consider it as a first draft. func BubbleSort(arr []int) []int { keepRunning := true for keepRunning { ...


8

About aesthetics I used the "long" form of all math statements, avoided the use of pointers, and used only while loops for aesthetic purposes (simply put, I think the code is prettier when it is written like that.) While making the code look aesthetically pleasing can be beneficial (especially if it makes it more readable), you should not give that a ...


8

Insertion sort allows a little known optimization. As coded, each iteration of an inner loop performs two comparisons: j > 0 and data[j - 1] > test_value. It is possible to get away with one: if (test_value < data[0]) { // No need to compare data anymore. Just shift. for (j = i; j > 0; j--) { data[j] = data[j - 1]; } } else { ...


7

It's a very unusual style to begin identifiers with uppercase letters. Include <stdbool.h> and use the bool type for flags. Many of the variables can be moved to smaller scopes. I think you should use for loops where appropriate - grouping the initialisation, predicate and advance clauses together improves clarity. Obviously this code lacks the ...


7

Observations Very interesting question. The numbers I came up with when running the program are 10248 distinct values sorted Selection sort on 16384 items: 353 ms Insertion sort on 16384 items: 176 ms Which makes the insertion sort twice as fast as the selection sort. This is on Windows 10 using Visual Studio 2019 on a 4 year old Lenovo Thinkpad P50 with ...


6

This is mostly pretty good. Only a couple of things: Delete this - print(self.input_list) You should leave printing to the caller. Also - why the class at all? This really boils down to a single function. You only have one member variable, and only one method. There's another issue - this class results in "surprising mutation". Iterating over it ...


6

We could make it better by making it more maintainable by simplifying the code and reducing duplication in the three for-loops. The problem is essentially checking that all successive pairs in the array are either equal or have the same sort order, so let's code it that way: import static java.lang.Integer.compare; import static java.lang.Integer.signum; ...


5

Rationale Given that this question and your previous question, that I've seen, both mangled testing and implementation, I think you should properly setup your Python project environment. Since you have tests you should use something like unittest or pytest. Since I would setup a test directory and a source directory I can't just import se_229598, and so ...


5

I'd suggest the following changes: Use a loop instead of recursion Use for/of instead of .forEach() push a single value instead of using an array with one element in it cache the lowest value so far so you don't have to constantly refetch it on every comparison Use a temporary array for the sort so the function is non-destructive to the source array (...


5

Review findMinimum means to me that you find the minimum value in an array of items. Since your function returns the index instead, call it indexOfMinimum. Prefer the use of const over let if you only assign a variable once: let minIndex = findMinimum(toSort); -> const minIndex = findMinimum(toSort);. Use arrow notation to write more compact functions: ...


5

Errors You use printf() to print the number of testcases etc. and give it more arguments than specified in the format string (clang: -Wformat-extra-args). You have conflicting types for mergeSort() as you declare mergesort() (lower-case s!) at the top and the use of mergeSort() in main() implicitly declares int mergeSort() which differs from void mergeSort(...


5

Im impressed that you used selection sort. The standard bubble sort method would be slower. Have a look at the bubble sort method anyway. int n = arr.length, r; for (int i = 0; i < n; i++) { for (int (j = i + 1); j < n; j++) { if (arr[i] > arr[j]) { r = arr[i]; arr[i] = arr[j]; arr[j] = ...


5

Edge cases As some comments have pointed out, your current code doesn't handle length-0 arrays, or length-1 arrays, or arrays where all elements are equal (e.g. [5, 5, 5, 5, 5]). In all of these cases, your while-loop at the very beginning of the method will hit an "array index out of bounds" exception. This needs to be fixed. Logic flow ...


5

I agree with everything user3629249 posted. Some other possible improvements are: Use getopt() or even getopt_long() if possible These functions take care of parsing command line arguments for you, which not only makes your life easier (especially if you add more flags), but also gives the user of your application the same command line experience as they ...


5

As you're already working with iterators you might as well accept iterators as arguments instead of the std::vector (as all STL algorithms do). That way it will work with other containers as well, even with raw pointers. The comparison can also be customized like in std::sort. Maybe the caller wants to sort in descending order or compare structures by some ...


5

You're right that the duplicate code in the sorters looks bad. To solve this, each sorter should only define the compare function, and all the rest should be done by the enum class. Or not even this. It took me a little while of experimenting until the code compiled, but here it is: enum class AudioRecordSort( private val cmp: Comparator<...


5

Instead of that while-loop, I'd use a for-loop and use the common loop/index variable i. Instead of a separate status variable, I'd simply use i % 2. I'd deduplicate the two cases. Putting the current two array elements into variables avoids duplicating the longer array[i] and array[i + 1] and simplifies the swap. As Joop Eggen points out, it's probably ...


5

As the main point of the question is about performance and not refactoring, I will address the performance of the code. Unfortunately, the question doesn't include actual numbers, just my Insertion sort was way faster than my Selection sort on random input (about one fourth the running time), and Insertion was a clear winner even for its worst case of ...


5

Some notes: I would avoid running a loop while changing i inside, it is very unclear to the reader. I prefer using two nested loops. The swap can be done in one step less I would initialize a size variable and use it throughout the code instead of hard-coding 6. Printing the array to the console should be in a separate function (as well as the sorting ...


5

That's not bubble sort. More like an overly eager selection sort. Bubble sort swaps neighbors.


4

I find the big points covered well: class and methods have one clear, documented purpose the API follows the well-known java.util.Arrays (if not to the point of documenting RuntimeExceptions thrown) I'd try to get rid of magic literals and code replication: size counts = new int[Byte.MAX_VALUE-Byte.MIN_VALUE+1] (or 1<<Byte.SIZE?), use for (int ...


Only top voted, non community-wiki answers of a minimum length are eligible