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
    Commented Oct 16, 2022 at 11:10
  • \$\begingroup\$ Also, why shouldn't Runnables be used? \$\endgroup\$
    – morloq
    Commented Oct 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\$
    – Reinderien
    Commented Oct 16, 2022 at 13:17
  • \$\begingroup\$ Runnable is a less terse syntax in this case. \$\endgroup\$
    – Reinderien
    Commented Oct 16, 2022 at 13:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.