Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code refactoring #20

Closed
Rylern opened this issue Apr 28, 2023 · 2 comments · Fixed by #21
Closed

Code refactoring #20

Rylern opened this issue Apr 28, 2023 · 2 comments · Fixed by #21

Comments

@Rylern
Copy link
Contributor

Rylern commented Apr 28, 2023

I've been investigating ways to reduce code in the LogViewerController class.

I think the best method is to divide the main window into several components, for example the menu, the table, the toolbar, and the footer (or less components). To do this, we would have one FXML file for each component and one main FXML file for the window (that references each component). This means we would also have one controller for each component and one main controller for the window.

The difficulty here is the communication between the different controllers. To avoid tight coupling, we could use the Mediator Pattern:

"The essence of the Mediator Pattern is to "define an object that encapsulates how a set of objects interact". It promotes loose coupling by keeping objects from referring to each other explicitly, and it allows their interaction to be varied independently. Client classes can use the mediator to send messages to other clients, and can receive messages from other clients via an event on the mediator class."

(taken from here. This link also gives an example)

So, based from this:

  • Do you know better ways to refactor the code?
  • Is it relevant for such a small project?
@petebankhead
Copy link
Member

My answer to both is "I don't know" :)

Currently, I think the controller class is 350 lines - including many blank lines and imports - which seems ok to me.

I feel like one control per FXML could defeat the purpose of FXML in bringing controls together. It might increase complexity rather than reduce it.

However the controller could probably be simplified... in different (conflicting?) ways.

For example, I see it includes the LogMessageListChangeListener and ThreadSetChangeListener. These are currently classes but could be replaced by methods (since they both implement a @FunctionalInterface with a single method). Alternatively, they could become classes in separate files - but then they would need to be passed an object that encapsulates the properties that they need to update. I'm not sure which approach is preferable.

Some of the static methods could also be moved elsewhere if needed - perhaps separating out the filtering part (since the concept of filtering makes sense independently of the UI).

One thing to think about is that JavaFX has the concept of separating 'controls' and 'skins'. There's a description at https://foojay.io/today/custom-controls-in-javafx-part-iv/ but I admit I find it hard to get my head around it all...

If I was doing it on my own, I'd probably conclude that the basic design is ok, the controller class isn't too long, and I'd keep the refactoring simple. But one of the goals of this project is to figure out how to use JavaFX better - so anything you think could help maintainability and be useful in later, larger projects is worth exploring.

@Rylern Rylern linked a pull request May 1, 2023 that will close this issue
@Rylern
Copy link
Contributor Author

Rylern commented May 1, 2023

OK, thanks for the information. I just created a pull request with the changes you suggested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants