4
\$\begingroup\$

I wrote this piece of code for a GUI i am making, which is a playlist interface. I was wondering if I could write all of this code out differently, but to the point where it functions exactly the same, as I am intrigued in learning different ways to write java to boost my flexibility and knowledge in the language.

package java;

import java.awt.*;
import java.awt.event.*;
import javax.swing.*;
import static coursework.VideoData.setRating;

public class UpdateVideos extends JFrame implements ActionListener {

    JTextField trackNo = new JTextField(2);
    JTextField newrate = new JTextField(2);
    TextArea content = new TextArea(6, 50);
    JButton apply = new JButton("Apply");


    public UpdateVideos() {
        setLayout(new BorderLayout());
        setBounds(100, 100, 400, 200);
        setTitle("Check Videos");
        setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);

        JPanel top = new JPanel();
        top.add(new JLabel("Enter Video Number:"));
        top.add(trackNo);       
        top.add(new JLabel("Enter Rating:"));      
        top.add(newrate);
        top.add(apply);
        apply.addActionListener(this);
        add("North", top);

        JPanel middle = new JPanel();

        middle.add(content);
        add("Center", middle);
        setResizable(false);
        setVisible(true);
    }

    public void actionPerformed(ActionEvent e) {

        String key = trackNo.getText();
        String name = VideoData.getName(key);
        Integer ratingnum;
        String newratenum;
        newratenum = newrate.getText();
        ratingnum = Integer.parseInt(newratenum);



        if (e.getSource() == apply) {
            if (name == null) {
                content.setText("No such video number");
            } else {
                setRating(key,ratingnum);
                content.setText(name + " - " + VideoData.getDirector(key));
                content.append("\nRating: " + stars(VideoData.getRating(key)));
                content.append("\nPlay count: " + VideoData.getPlayCount(key));
            }

        }
    }

    private String stars(int rating) {
        String stars = "";
        for (int i = 0; i < rating; ++i) {
        stars += "*";
        }
        return stars;
    }
}
\$\endgroup\$
0
4
\$\begingroup\$

At the danger of repeating myself (not to you, but generally regarding swing code):

  1. Don't learn swing. Swing has been EOL'd for over a year now and has been officially superseded by JavaFX. If you want to learn GUI programming for thick clients in java: Go with JavaFX

  2. Somebody has to have a tutorial that does this wrong ... UpdateVideos extends JFrame implements ActionListener is one of those lines that should make you shudder. It's a generally accepted wisdom that to write SOLID code, one should use Composition over Inheritance. This means that instead of saying "UpdateVideos is a JFrame" you should say: "UpdateVideos has a JFrame".

  3. This is one of the things that every swing program seems to have and I find it terrifying: setVisible(true); in the constructor... This is a violation of two things. First: The principle of single responsibility (SRP), the S in SOLID. A constructor is responsible for getting the object it initializes into a usable, valid state. This does explicitly not entail making the JFrame inside the object (or the JFrame object itself) visible. Second: The principle of least surprise. Let's say you go to a coffee shop and order a coffee... would you expect the the coffee you get to pour itself down your throat? Sure you'll want to drink it sooner or later, but on your own terms. It's not the coffee's (or the JFrame's) responsibility to make sure that you drink (or show) it, but yours.


After getting this out of the way, let's talk about your code: Aside from the two coding issues I already mentioned above your code is pretty clean. The only thing that really bothers me is how many magic numbers you use.

There is a minor optimization in stars. Consider the following code:

private String stars(int rating) {
    final char[] stars = new char[rating];
    Arrays.fill(stars, '*');
    return new String(stars);
}

This avoids the overhead of concatenating Strings and instead uses a char[] to basically build the string without an explicit for-loop. This is just a marginal benefit though and you might want to keep your method structure, but use a StringBuilder instead to increase performance.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the advice I will defiantly take your comments on board. I need all the experience I can get. I was wandering if you could help me re writing the whole of the if and else statements midway through the code in a different way but I want it to function the same way specifically, as I am looking for new ideas. I want it to function in exactly the same way but just want it written differently. \$\endgroup\$ – wavekootah Mar 7 '17 at 16:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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