2
\$\begingroup\$

I programmed a little sorting visualizer a while back and I have been meaning to get some feedback on the overall implementation and on what I could do better to have cleaner code or a "best practice" version.

I did my best to use OOP programming.

I have done some projects in the past so this is not my first one and I wouldn't say that I am a complete beginner but I guess I still have a lot to learn and to improve upon so any feedback is appreciated dearly! :)

Main class:

public class Main{ public static void main(String[] args) { new Frame(); } } 

Panel class where the main logic of the program takes place:

import javax.swing.*; import java.awt.*; import java.awt.event.*; public class Panel extends JPanel implements ActionListener{ private int[] array = new int[Constants.ELEMENTS.getValue()]; private SelectionSort selectionSort; private InsertionSort insertionSort; private BubbleSort bubbleSort; private JButton[] buttons; private JButton selectionSortButton = new JButton("selection sort"); private JButton reset = new JButton("reset"); private JButton insertionSortButton = new JButton("insertion sort"); private JButton bubbleSortButton = new JButton("bubble sort"); public Panel() { this.buttons = new JButton[] {selectionSortButton, insertionSortButton, bubbleSortButton, reset}; this.selectionSort = new SelectionSort(); this.insertionSort = new InsertionSort(); this.bubbleSort = new BubbleSort(); this.setPreferredSize(new Dimension(Constants.WIN_WIDTH.getValue(), Constants.WIN_HEIGHT.getValue())); this._initArray(); this.setOpaque(true);//otherwise backgroundcolor won't be visible for(JButton button : buttons){ button.addActionListener(this); this.add(button); } } public void _initArray() { for(int i = 0; i < this.array.length; i++) { array[i] = (int)(Math.random() * Constants.RANGE.getValue()+1); } } @Override public void paintComponent(Graphics g) { super.paintComponent(g); this.setBackground(Color.ORANGE); for(int i = 0; i < this.getArray().length; i++) { g.drawRect(i, 800 - array[i], 1, array[i]); g.fillRect(i, 800 - array[i], 1, array[i]); } } public int[] getArray() { return this.array; } public void render(int time) { repaint(); try{ Thread.sleep(time); } catch(Exception e) { e.printStackTrace(); } } public void resetArray() { Thread t = new Thread(new Runnable() { @Override public void run() { for(int i = 0; i < getArray().length; i++) { getArray()[i] = (int)(Math.random() * Constants.RANGE.getValue()+1); render(1); } } }); t.start(); } public void initSort(SortingInterface sortint, int start) { Thread t = new Thread(new Runnable() { @Override public void run() { for(int i = start; i <= getArray().length; i++) { sortint.sort(getArray(), i); render(10); } } }); t.start(); } @Override public void actionPerformed(ActionEvent e) { if(e.getSource() == this.reset) { this.resetArray(); } if(e.getSource() == this.selectionSortButton){ initSort(selectionSort, 0); } if(e.getSource() == this.insertionSortButton) { initSort(insertionSort, 1); } if(e.getSource() == this.bubbleSortButton) { initSort(bubbleSort, 0); } } } 

Frame class:

import javax.swing.*; public class Frame extends JFrame{ public Frame() { this.getContentPane().add(new Panel()); this.setTitle("Sorting visualizer"); this.pack(); this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE); this.setResizable(false); this.setVisible(true); this.setLocationRelativeTo(null); } } 

Interface for all the sorting algorithms:

public interface SortingInterface { public void sort(int[] array, int i); } 

My implementation of Selection sort:

public class SelectionSort implements SortingInterface{ @Override public void sort(int[] array, int i) { int min = i; for(int j = i+1; j < array.length; j++) { if(array[j] < array[min]) { min = j; } } if(min != i) { int temp = array[i]; array[i] = array[min]; array[min] = temp; } } } 

My implementation of Insertion sort:

public class InsertionSort implements SortingInterface{ @Override public void sort(int[] array, int i) { if(array[i] < array[i-1]) { for(int j = 0; j < i; j++) { if(array[i] < array[j]) { int temp = array[i]; array[i] = array[j]; array[j] = temp; } } } } } 

My implementation of Bubble Sort:

public class BubbleSort implements SortingInterface{ @Override public void sort(int[] array, int i) { for(int j = i+1; j < array.length; j++) { if(array[i] > array[j]) { int temp = array[i]; array[i] = array[j]; array[j] = temp; } } } } 

Enum for all (or most I think) constants, not because it is best practice (not sure about that actually) but because I felt like using an enum:

public enum Constants { WIN_WIDTH(1200), WIN_HEIGHT(800), ELEMENTS(1200), RANGE(WIN_HEIGHT.getValue()-100); public final int value; private Constants(int value) { this.value = value; } public int getValue() { return this.value; } } 

I have been programming since June. I started with Mooc.fi java part 1 and 2 and then got a little into the Odin Project. But I stopped after the Tic-Tac-Toe project as I realized that web development really isn't it for me. I did the sorting visualizer like a month ago and ever since I kind of did not really have any further project ideas... So if someone has some interesting ideas, I'm all in :)

\$\endgroup\$

    1 Answer 1

    2
    \$\begingroup\$

    In many cases your use of import * is a little too aggressive. You're filling the working namespace of your files with everything from swing and awt when you only need a small handful of symbols.

    A better name for SortingInterface is something like SortingAlgorithm.

    There should be nothing individually special about your selection / insertion / bubble sort references within Panel - they should all be treated the exact same.

    I find that you overuse this., and most of those prefixes should go away. This isn't Python, after all.

    _initArray is better structured as a member initialiser that uses a stream, instead of a method that mutates an existing member.

    Don't catch(Exception e). In this case you're looking for InterruptedException.

    In contemporary Java you should no longer be using Runnable, and instead just write inline lambdas.

    Factor out the common code from resetArray and initSort into another function.

    Writing one handler for multiple scenarios and then having to differentiate between those scenarios with if is an antipattern. Register a separate handler for each button.

    Your user interface allows for some very funny and incorrect behaviour: you allow for multiple concurrent reset or sort operations on the same data. This should be disallowed by disabling the buttons until any of those operations is done. A more usable enhancement that I have not shown is for you to never disable the reset button, and if the reset button is pushed during another operation, cancel its thread before continuing.

    Move your button name strings to name strings in your algorithm subclasses.

    You have some boundary failures in your sort implementations. The start index mechanism needs to go away, and you need to take more care that your indices make sense at the beginning and end of the array.

    Don't write a Constants file, and especially don't make it an enum. Move the constants to private static final members internal to the classes that need them.

    Suggested

    Main.java

    public class Main{ public static void main(String[] args) { new Frame(); } } 

    Panel.java

    import javax.swing.JButton; import java.awt.Color; import java.awt.Dimension; import java.awt.Graphics; import java.awt.event.ActionEvent; import java.util.Arrays; import java.util.List; import java.util.Random; import java.util.function.IntConsumer; import java.util.stream.IntStream; public class Panel extends JPanel { private static final int WIN_WIDTH = 1200, WIN_HEIGHT = 800, RANGE = WIN_HEIGHT - 100, ELEMENTS = 1200; private final Random rand = new Random(); private final int[] array = IntStream.generate(() -> rand.nextInt(RANGE)) .limit(ELEMENTS) .toArray(); private final SortingAlgorithm[] algos = { new SelectionSort(), new InsertionSort(), new BubbleSort() }; private final List<JButton> algoButtons = Arrays.stream(algos) .map(algo -> { JButton button = new JButton(algo.getName()); button.addActionListener(event -> initSort(algo)); return button; }) .toList(); private final JButton resetButton = new JButton("reset"); public Panel() { setPreferredSize(new Dimension(WIN_WIDTH, WIN_HEIGHT)); setOpaque(true); // otherwise, backgroundcolor won't be visible algoButtons.forEach(this::add); resetButton.addActionListener(this::resetArray); add(resetButton); } @Override public void paintComponent(Graphics g) { super.paintComponent(g); setBackground(Color.ORANGE); for (int i = 0; i < array.length; i++) { g.drawRect(i, 800 - array[i], 1, array[i]); g.fillRect(i, 800 - array[i], 1, array[i]); } } public int[] getArray() { return array; } public void render(int time) { repaint(); try { Thread.sleep(time); } catch (InterruptedException e) { e.printStackTrace(); } } private void enableButtons(boolean enabled) { resetButton.setEnabled(enabled); for (JButton button: algoButtons) button.setEnabled(enabled); } private void animate(IntConsumer consume, int time) { enableButtons(false); new Thread(() -> { for (int i = 0; i < getArray().length; i++) { consume.accept(i); render(time); } enableButtons(true); }).start(); } private void resetArray(ActionEvent e) { animate(i -> array[i] = rand.nextInt(RANGE), 1); } public void initSort(SortingAlgorithm algorithm) { animate(i -> algorithm.sort(array, i),10); } } 

    Frame.java

    import javax.swing.JFrame; public class Frame extends JFrame { public Frame() { getContentPane().add(new Panel()); setTitle("Sorting visualizer"); pack(); setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE); setResizable(false); setVisible(true); setLocationRelativeTo(null); } } 

    SortingAlgorithm.java

    public interface SortingAlgorithm { void sort(int[] array, int size); String getName(); } 

    SelectionSort.java

    public class SelectionSort implements SortingAlgorithm { @Override public String getName() { return "selection sort"; } @Override public void sort(int[] array, int target) { int min = target; for (int i = target+1; i < array.length; i++) { if (array[min] > array[i]) min = i; } if (min != target) { int temp = array[target]; array[target] = array[min]; array[min] = temp; } } } 

    InsertionSort.java

    public class InsertionSort implements SortingAlgorithm { @Override public String getName() { return "insertion sort"; } @Override public void sort(int[] array, int target) { if (target >= array.length - 1) return; if (array[target+1] < array[target]) { for (int i = 0; i <= target; i++) { if (array[target+1] < array[i]) { int temp = array[target+1]; array[target+1] = array[i]; array[i] = temp; } } } } } 

    BubbleSort.java

    public class BubbleSort implements SortingAlgorithm { @Override public String getName() { return "bubble sort"; } @Override public void sort(int[] array, int target) { int i = target; for (int j = i+1; j < array.length; j++) { if (array[i] > array[j]) { int temp = array[i]; array[i] = array[j]; array[j] = temp; } } } } 
    \$\endgroup\$
    4
    • \$\begingroup\$Thanks a lot! Those are some great tips! I will look into lambda functions and the other things you mentioned. Why is my use of an enum a bad idea?\$\endgroup\$
      – morloq
      CommentedOct 16, 2022 at 11:10
    • \$\begingroup\$Also, why shouldn't Runnables be used?\$\endgroup\$
      – morloq
      CommentedOct 16, 2022 at 11:31
    • 1
      \$\begingroup\$@morloq Enum in this case is not a good idea because an enum is supposed to describe a set of values that one variable may take. That is not a good fit for this case. Instead, the single class that uses these constants should enclose them as privates.\$\endgroup\$CommentedOct 16, 2022 at 13:17
    • \$\begingroup\$Runnable is a less terse syntax in this case.\$\endgroup\$CommentedOct 16, 2022 at 13:18

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.