I am developing a plugin for intellij that basically monitors a log file and displays the data in a tool window within the IDE in real-time. I have managed to get it working but I would like to know the areas where I can improve on. I'm specifically interested in the structure of my code and the log file monitoring method followed.
The createToolWindowContent method is called when the log panel is opened for the first time.
public class LogToolWindowLoader implements ToolWindowFactory {
private UIOperations ui;
private LogFileFunctions log_func;
private ToolWindow toolWindow;
@Override
public void createToolWindowContent(Project project, ToolWindow toolWindow) {
this.toolWindow = toolWindow;
//this generates the log viewer GRID within the IDE
ui = new UIOperations(toolWindow);
ui.createTable();
//Create new thread to retrieve data from log file. this is a continuous process, hence the new Thread
new Thread(){
public void run(){
log_func = new LogFileFunctions();
log_func.connectToLog();
while(true){
String[] result = log_func.getData();
if (result!=null) {
//GUI updated on the Event Dispatcher Thread
SwingUtilities.invokeLater(new Runnable() {
@Override
public void run() {
ui.updateTable(result);
}
});
}
else {
try {
Thread.sleep(1000);
} catch (InterruptedException e) {
e.printStackTrace();
}
}
}
}
}.start();}
UI operations are handled in this class:
public class UIOperations {
private ToolWindow toolWindow;
private DefaultTableModel table_model;
private JBTable table;
private final String headers[]= {"header1","header2","header3","header4","header5","header6"};
public UIOperations(ToolWindow toolWindow){
this.toolWindow = toolWindow;
}
public void createTable(){
JComponent panel = new JPanel(new BorderLayout()); //new BorderLayout extends panel completely inside the tool Window
panel.setBackground(JBColor.WHITE);
toolWindow.getComponent().add(panel);
//set column names
table_model = new DefaultTableModel(headers,0);
table = new JBTable(table_model);
JScrollPane scrollPane = new JBScrollPane(table);
panel.add(scrollPane);
//table formatting
table.setShowHorizontalLines(false);
table.setShowVerticalLines(false);
table.setFillsViewportHeight(true);//sets table background colour
System.out.println("Log Viewer Table Generated..");
}
public void updateTable(String[] str){
table_model.addRow(str);
}
}
Log file processing is handled in this class:
public class LogFileFunctions {
private BufferedReader in;
//reads the log file to retrieve data.
public String[] getData(){
//System.out.println(System.getProperty("user.dir")); gives the executing directory
String str; //holds string read from file
String[] result=null; //holds tokenized string
try {
str = in.readLine();
if (!(str == null)) {
result = str.split("[|]");
}
}catch (FileNotFoundException e) {
e.printStackTrace();
} catch (IOException e) {
e.printStackTrace();
}
return result;
}
//connect FileReader to log file
public void connectToLog() {
try {
FileReader fileReader = new FileReader(getClass().getResource("log.txt").getPath());
in = new BufferedReader(fileReader);
System.out.println("Log File location: " + getClass().getResource("log.txt").getPath());
} catch (FileNotFoundException e) {
System.out.println("ERROR: log.txt file not found at " + getClass().getResource("log.txt").getPath() );
}
}
}
EDIT: I would appreciate pointers on how to optimize my code
1 Answer 1
LogToolWindowLoader.java
Constant or user property
Thread.sleep(1000);
This is a nice example of magic number.
It's better that you create at least a private static final int for it, but even better could be the possibility to change that value by a property file, with a default of 1000 if the property isn't there.
Like this you could give the users easily control that value, because in mine opinion there could be people who want it slower or faster.
To the infinitive
while(true){
I never like to see this hack.
It's a infinitive loop with no possibility to stop it.
Why not make it a variable and give the user a possibility to stop loop?
UIOperations.java
Constants
private final String headers[]= {"header1","header2","header3","header4","header5","header6"};
A good usage of the final word and array but you create this array for each instantiation of the class.
Better could make that field also static so you have instantiated this variable once for all the different instantiations of UIOperations.
Final keyword in other situations
private ToolWindow toolWindow;
This field could be final because you set it in the constructor and there is no setter for setting this at a later point.
LogFileFunctions.java
Javadoc preffered
//reads the log file to retrieve data.
//connect FileReader to log file
This could be better set as java doc in stead of normal comments.
Like this if you generate the javadoc, people who read that will also see your comment.
same or not the same
if (!(str == null)) {
What the hell?
I hope this is old code created by resolving problems because otherwise this is actually your biggest issue (In mine personally insight).
For the moment you check if it's equal and then reverse the result.
There is nothing wrong with just checking for not equal :
if (str != null) {
Naming
String str; //holds string read from file
String[] result=null; //holds tokenized string
Code should be self explaining, so if you need to put comment behind a variable name to explain it, there is something wrong.
Mine suggestion is :
String lineFromFile;
String tokenizedLine=null;
Opening and closing resources
As last in this class is for me the absence of closing your resources.
This is just a nice example to create memoryleaks.
Summary
You see you have good experience in programming, it's a nice and readable code with really tiny things I could point out.
I hope this review could help you in one or more points.