10
\$\begingroup\$

I have a very basic version of a media player built using JavaFX. The project is well segregated and has a different packages for controllers, utils and supporter classes.

My project follows the MVC pattern and it contains the following important parts:

  • All the views are in FXML files.
  • These views have dedicated controllers for them, all inside the com.ita.controller package.
  • Some custom controls / dialogs inside the com.ita.ui package.
  • Utility classes inside the com.ita.util.
  • The Main class extends Application and is the entry point. It is also responsible for invocation and loading the views into the stage.

Main.java

public class Main extends Application {

    @Override
    public void start(Stage primaryStage) throws Exception {
        FXMLLoader loader = new FXMLLoader(getClass().getResource("/com/ita/fxml/mediaplayer.fxml"));
        BorderPane pane = loader.load();
        Scene scene = new Scene(pane, 650, 400);
        primaryStage.setScene(scene);
        MediaPlayerController controller = ((MediaPlayerController) loader.getController());
        // Load Playlist FXML and inject controller/root
        FXMLLoader playListLoader = new FXMLLoader(getClass().getResource("/com/ita/fxml/playlist.fxml"));
        playListLoader.load();
        controller.injectPlayListController((PlaylistController) playListLoader.getController());
        controller.injectPlayListRoot(playListLoader.getRoot());
        bindSize(controller, scene);
        controller.setStage(primaryStage);
        primaryStage.show();
        controller.applyDragAndDropFeatures(scene);
    }

    private void bindSize(MediaPlayerController controller, Scene scene){
        controller.timerSliderWidthProperty().bind(scene.widthProperty().subtract(500));
        controller.mediaViewWidthProperty().bind(scene.widthProperty());
        controller.mediaViewHeightProperty().bind(scene.heightProperty().subtract(70));
    }

    public static void main(String[] args) {
        launch(args);
    }
}

PlaylistController.java

public class PlaylistController implements Initializable {

    @FXML
    private Button add;

    @FXML
    private Button delete;

    @FXML
    private ListView playList;

    private ObservableList playListFiles =FXCollections.observableArrayList();
    private ObjectProperty<Path> selectedMedia = new SimpleObjectProperty<>();
    private ObjectProperty<Path> deletedMedia = new SimpleObjectProperty<>();

    @Override
    public void initialize(URL location, ResourceBundle resources) {
        playList.setOnMouseClicked((click) -> {
            if (click.getClickCount() == 2) {
                if (playList.getSelectionModel().getSelectedItem() != null) {
                    selectedMedia.setValue((Path) playList.getSelectionModel().getSelectedItem());
                }
            }
        });
    }

    @FXML
    void add(ActionEvent event) {
        FileChooser chooser = new FileChooser();
        chooser.getExtensionFilters().addAll(
                new FileChooser.ExtensionFilter("Files",
                        PropertiesUtils.readFormats()));
        List<Path> listOfFiles = new ArrayList<Path>();
        listOfFiles = FileUtils.convertListFiletoListPath(chooser.showOpenMultipleDialog(((Button) event.getSource()).getScene().getWindow()));
        if (listOfFiles != null) {
            listOfFiles.stream().forEach(System.out::println);
            listOfFiles.stream().forEach(playListFiles::add);
            playListFiles.stream().forEach(System.out::println);
            playList.setItems(playListFiles);
        }
    }

    @FXML
    void delete(ActionEvent event) {
        if (playList.getSelectionModel().getSelectedItem() != null) {
            if(null!=playListFiles || !playListFiles.isEmpty()) {
                deletedMedia.setValue((Path) playList.getSelectionModel().getSelectedItem());
                playListFiles.remove(playList.getSelectionModel().getSelectedItem());
                playList.setItems(playListFiles);
            }
        }
    }

    public ObservableList listViewItems(){
       return playListFiles;
    }

    public ObjectProperty<Path> selectedFile(){
        return selectedMedia;
    }

    public ObjectProperty<Path> deletedFile() {
        return deletedMedia;
    }
}

PropertiesUtils.java

public class PropertiesUtils {
    public static Map<String, String> readDetails()
    {
        Map<String, String> map = new HashMap<String, String>();
        try{
            InputStream in = PropertiesUtils.class.getClassLoader().getResourceAsStream("details.properties");
            Properties p = new Properties();
            p.load(in);
            map.put("name", p.getProperty("name"));
            map.put("version", p.getProperty("version"));
            map.put("link", p.getProperty("link"));
        }
        catch (Exception e) {
             e.printStackTrace();
        }
        return map;
    }

    public static List<String> readFormats()
    {
        List<String> formats = null;
        try
        {
            InputStream in = PropertiesUtils.class.getClassLoader().getResourceAsStream("details.properties");
            Properties p = new Properties();
            p.load(in);
            formats = Arrays.asList(p.getProperty("formats").split(","));
        }
        catch (Exception e) {
            e.printStackTrace();
        }
        return formats;
    }
}

I wanted to add more functionality into the project, but before doing that I just wanted to know if my code follows:

  • Code standards that an open source project should. I am aware of the fact that proper documentation is missing, and I am working on the same.
  • Naming conventions.
  • Coupling. I have got some reviews that my code is very tightly coupled. If you have the same view, please write some suggestions on how to overcome it.
  • Am I writing the utility classes as they should be written? Mostly, the method definitions and declaring them static.

GitHub

\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

Depending on who you ask, this piece of code could be changed slightly to reduce nesting

@FXML
void delete(ActionEvent event) {
    if (playList.getSelectionModel().getSelectedItem() != null) {
        if(null!=playListFiles || !playListFiles.isEmpty()) {
            deletedMedia.setValue((Path) playList.getSelectionModel().getSelectedItem());
            playListFiles.remove(playList.getSelectionModel().getSelectedItem());
            playList.setItems(playListFiles);
        }
    }
}

there really isn't a reason for putting in another if statement, just merge the condition statements into one condition

@FXML
void delete(ActionEvent event) {
    if ((playList.getSelectionModel().getSelectedItem() != null) &&
            (null!=playListFiles || !playListFiles.isEmpty()) {
        deletedMedia.setValue((Path) playList.getSelectionModel().getSelectedItem());
        playListFiles.remove(playList.getSelectionModel().getSelectedItem());
        playList.setItems(playListFiles);
    }
}

Same here

@Override
public void initialize(URL location, ResourceBundle resources) {
    playList.setOnMouseClicked((click) -> {
        if (click.getClickCount() == 2) {
            if (playList.getSelectionModel().getSelectedItem() != null) {
                selectedMedia.setValue((Path) playList.getSelectionModel().getSelectedItem());
            }
        }
    });
}

I just put the conditionals together, does the exact same thing as your code, but slightly clearer

@Override
public void initialize(URL location, ResourceBundle resources) {
    playList.setOnMouseClicked((click) -> {
        if ((click.getClickCount() == 2) && (playList.getSelectionModel().getSelectedItem() != null)) {
                selectedMedia.setValue((Path) playList.getSelectionModel().getSelectedItem());
        }
    }
});
\$\endgroup\$
3
  • 1
    \$\begingroup\$ That was a nice catch. \$\endgroup\$ Jul 23, 2015 at 14:12
  • \$\begingroup\$ I'd even prefer to go for an early return in Guard-Clause style I think \$\endgroup\$
    – Vogel612
    Jul 23, 2015 at 14:26
  • \$\begingroup\$ these wouldn't be a good spot for that, there isn't a whole lot going on in these methods for a Guard clause to be worth while. the Mouse Click specifically should stay this way, IMO. \$\endgroup\$
    – Malachi
    Jul 23, 2015 at 14:28
3
\$\begingroup\$

This is on top of the other good points already mentioned.

Catch the most specific Exception type

Catching Exception is dangerous, because it can mask something truly unexpected. For example if you're really expecting an IOException, but an EmptyStackException happens (truly unexpected), it will be caught by catch (Exception e) and you may never even notice.

Secondly, catching the most specific exception helps readers of the code, by telling what exactly can go wrong with that piece of code.

Limit the scope of try blocks

It's good to limit the scope of try blocks to the smallest possible, to prevent exceptions thrown from unexpected places.

Avoid code duplication

The duplicated "details.properties" string literal immediately jumps into the eye. This should be in a private static final String constant.

On closer look, the entire logic reading the properties file is duplicated.

Closing resources

The input streams used when reading the properties file are not closed.

Naming

Single-letter variable names like p are not great. Just spell out "properties".

Braces

Instead of this:

public static List<String> readFormats()
{
    // ...
}

The common style for placing braces in Java:

public static List<String> readFormats() {
    // ...
}

Using Java 7

It's reasonable to require Java 7 with your utility library. Java 6 is no longer officially supported, those who still use are running a risk, and should be urged to switch ASAP.

As of Java 7, you can simplify the initialization of parameterized types using the diamond <> operator. So instead of this:

    Map<String, String> map = new HashMap<String, String>();

You can write like this:

    Map<String, String> map = new HashMap<>();

Suggested implementation

Putting the above points together, the implementation becomes:

private static Properties readProperties() {
    InputStream in = PropertiesUtils.class.getClassLoader().getResourceAsStream("details.properties");
    Properties properties = new Properties();
    try {
        properties.load(in);
    } catch (IOException e) {
        e.printStackTrace();
    }
    try {
        in.close();
    } catch (IOException e) {
        e.printStackTrace();
    }
    return properties;
}

public static Map<String, String> readDetails() {
    Map<String, String> map = new HashMap<>();
    Properties properties = readProperties();
    map.put("name", properties.getProperty("name"));
    map.put("version", properties.getProperty("version"));
    map.put("link", properties.getProperty("link"));
    return map;
}

public static List<String> readFormats() {
    Properties properties = readProperties();
    return Arrays.asList(properties.getProperty("formats").split(","));
}
\$\endgroup\$
1
\$\begingroup\$

As far as I see, your code is well written but just my two cents.

Appending @Malachi answers as you don't need the nesting. May be he forgot to tell about:

if (click.getClickCount() == 2) {
            if (playList.getSelectionModel().getSelectedItem() != null) {
                selectedMedia.setValue((Path) playList.getSelectionModel().getSelectedItem());
            }
        }

This can also be grouped to one:

if (click.getClickCount() == 2 && playList.getSelectionModel().getSelectedItem() != null) {
                selectedMedia.setValue((Path) playList.getSelectionModel().getSelectedItem());
        }

Other than that, if you are using Java 7 or 8, you should be using try with resources instead of try-catch as that is less error prone.

And instead of returning null, you should be returning an empty collection . As far as I know, returning null is not a very good approach.

\$\endgroup\$

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.