Code Review Asked by gss on October 24, 2020
I have written a selection sort code in java. I know its very elementary algorithm, but since i am learning, so wanted your input about the quality of the code. Please have a look at the code:
package selection_sort;
import java.util.Scanner;
public class SelectionSort {
int [] arrayToBeSorted;
Scanner scan=new Scanner(System.in);
SelectionSort(){
System.out.println("Enter the number of elements");
int total=scan.nextInt();
this.arrayToBeSorted=new int[total];
for(int i=0;i<total;i++) {
System.out.println("Enter the "+i+" number of elements");
this.arrayToBeSorted[i]=scan.nextInt();
}
}
public void Sort() {
int minIndex,min;
boolean swapRequired=false;
for(int i=0;i<arrayToBeSorted.length;i++) {
minIndex=i;
min=this.arrayToBeSorted[i];
for(int j=i+1;j<this.arrayToBeSorted.length;j++) {
if(this.arrayToBeSorted[j]<min) {
min=this.arrayToBeSorted[j];
minIndex=j;
swapRequired=true;
}
}
if(swapRequired) {
swap(i,minIndex);
}
}
for(int x:this.arrayToBeSorted) {
System.out.print(x+" ");
}
}
public void swap(int i,int j) {
//System.out.println("swap called, pos="+i+"and minindex="+j);
int temp=this.arrayToBeSorted[i];
this.arrayToBeSorted[i]=this.arrayToBeSorted[j];
this.arrayToBeSorted[j]=temp;
//System.out.println("------------");
}
public static void main(String[] args) {
SelectionSort s=new SelectionSort();
s.Sort();
}
}
Also about Analysis, as selection is O(n2), is it because I am using nested for loop. Is there any tool which tells us about complexity of code .Thanks in advance and please be patient if its very naive
int [] arrayToBeSorted;
Scanner scan=new Scanner(System.in);
Without any modifier, members and functions are package-private
. Which means that they are accessible from the same package, but not by instances extending the class. That's an extremely odd thing, actually, when looking at it from an object-oriented point of view. You want to either make them private
, or if extending classes should be able to access them, protected
.
Same goes for the constructor.
System.out.println("Enter the number of elements");
int total=scan.nextInt();
this.arrayToBeSorted=new int[total];
You could use a List
/ArrayList
instead of an array, which would mean that you can receive as many numbers from the users as they want without having to specify the count beforehand. An empty input could be used for declaring the end of the list. However, that has the downside that you must use Integer
instead of int
, so it' is kinda hard to tell what is better. It also has the downside that detecting an empty input is that readily available from Scanner
, as you'd need to use nextLine
for that, with manual conversion to int
.
You could also emulate a List
by dynamically growing the array. Either by doing it for every item, performance does not matter in this use-case, or by doubling the size when needed.
this.arrayToBeSorted=new int[total];
You only require the this
modifier if you have a variable with the same name in the same scope, for example in a constructor:
public ValueContainer(int value) {
this.value = value;
}
It's common practice to omit this
if not needed. If you ever find yourself in the situation that it is confusing whether an instance, static or local variable is used, you did something wrong there anyway.
for(int i=0;i<total;i++) {
I'm a very persistent advocate for using "real" names for variables in loops.
for (int counter = 0; counter < total; counter++) {
// or
for (int index = 0; index < total; index++) {
public void Sort() {
The Java naming conventions state that methods/functions should be lowerCamelCase.
int minIndex,min;
Personally I'd avoid declaring multiple variables on the same line. It makes it easy to miss declaration.
boolean swapRequired=false;
That's a great example of a great name for a variable, thank you!
for(int x:this.arrayToBeSorted) {
System.out.print(x+" ");
}
That, unfortunately, is a bad name for a variable. Only use "x", "y" and "z" when working with dimensions. "value" or "number" would be a great name here.
Overall, it looks valid. I did not run it to test the functionality, though. What you should change is that you have logic in the constructor. Nobody expects constructors to be filled with logic, because you can't see the intent of logic from them. They should be as nothing doing as possible. That means that you should move the logic into sort
or, even better, into main
. Your Sorter should not be concerned with input at all, it should only be concerned with sorting values, ideally, and another class (InputReader
?) should be concerned with getting the input. So what you could do is something like this:
public static final void main(String[] args) {
InputReader inputReader = new InputReader();
int[] values = inputReader.readValues();
Sorter sorter = new Sorter();
sorter.sort(value);
// TODO Print sorted output.
}
That also has the upside that you don't need to hold state in Sorter
at all, which would make it thread-safe by default.
Correct answer by Bobby on October 24, 2020
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP