Style
- Use proper styling. You should use more whitespaces, this will make your code more readable. Instead of
num_sort=new
use num_sort = new
, insead of i!=min_index
use i != min_index
, etc
- Follow the Java naming conventions: variable and function names should use camelCase, not snake_case.
- You should include documentation that explains the user how the class is used. For example, without looking at the code I would not expect that creating a new instance of the class automatically sorts the list, so it is something you must make clear in the documentation.
Comments
Commenting the code is good, but too many comments can make a simple function look complicated. The general rule is that code tells you how, comments tell you why.
For example, I think int min = this.num_sort.get(i);
does not need to be commented; it's pretty clear that you are getting the i-th item of the list.
Also, leave a whitespace between //
and the text, it's more readable.
Code structure
It does not make sense to have this in a class; new SelectionSort(userlist)
is not the way someone would expect to sort a list. I am going to make some recommendations that for the class design, but for the rest of the review I will restructurate the code so that it is more conventional.
As a user I expect select_sort
to sort the list and that's all; I decide whether I want to print the list or not (what if I want to print it differently to you?). So you should move the printing code to the main
function.
Class design
If you were to continue with the current class design, there must be some changes:
- Do not sort the list in the construction. It is not the expected behaviour. Let the user call
sortSelect()
. Actually, I think selectionSort()
would be a better name.
- Is
exchange
a method that is meant to be used by the user? I don't think so, so make it private
instead of public
so only the class code can use it.
num_sort
does not seem like an intuitive name; I had to look at the code to know what it is. Just calling it list
would be better.
- You don't care how the list is internally implemented, you only care that it is a list. So use the more generic
List
interface instead of ArrayList
.
- You are assigning your list twice: on
num_sort=new ArrayList<>()
and on this.num_sort=userlist;
. The first one is not necessary, because it will always be replaced in the constructor. Furthermore, since you never reassign the list again, you should declare it final
.
private final List<Integer> list;
public SelectionSort(List<Integer> userList) {
this.list = userList;
}
Alternative design
- In my opinion, your selection sort algorithm should just be one
static
method that takes the list as input. That would make it similar to the Collections.sort()
method that already exists in Java.
Scanner
is a resource that must be closed. You can make Java close it automatically by using a try with resources.
try (Scanner in = new Scanner(System.in)) {
// your code
}
- You can still keep the convenience method
exchange
(as static
and taking the list as parameter), but since it is quite short and you only call it in one place I think you could just write the code directly. If you decide to keep the function, I would change the parameter names to just i
and j
, and the method name to swap
(it's more common). Since it's the same doing exchange(current_elem, new_min)
than exchange(new_min, current_elem)
those parameter names do not make much sense. BUT you don't even need to implement that method, you can just use Collections.swap()
Final code
/**
* Sorts the given list using a selection sort algorithm.
* @param list The list to be sorted.
*/
public static void selectionSort(final List<Integer> list) {
// outer for loop to pick one element at a time
// e.g if list is 20,12,3,4,5,6,98----this loop will first pick 20
for (int i = 0; i < list.size(); i++) {
int min = list.get(i);
int minIndex = i;
// Go over the rest of the list and compare minimum by rest of list. If number smaller than min is found, then it is assigned as new min
// So go over list starting from 12 and comparing 20 with each of them and finding the new min, in this case it will be 3
for (int j = i + 1; j < list.size(); j++) {
if (list.get(j) < min) {
min = list.get(j);
minIndex = j;
}
}
if (i != minIndex) {
Collections.swap(list, i, minIndex);
}
}
}
public static void main(String[] args) {
try (Scanner in = new Scanner(System.in)) {
System.out.println("Enter how many numbers you want to sort");
int count = in.nextInt();
System.out.println("Enter the numbers you want to sort");
List<Integer> list = new ArrayList<>();
for(int i = 0; i < count; i++) {
list.add(in.nextInt());
}
selectionSort(list);
System.out.println("Sorted entered are: ");
for (int x : list) {
System.out.print(x + " ");
}
}
}