5
\$\begingroup\$

I have rewritten my little chat application and tried to pay attention to former hints and tips. The viewer and the writer have to run as seperate applications. the Writer.class basically stores messages as property files. The directory, where the files get stored, is watched by the Viewer.class and gets scanned every 0.5 seconds.

I tried to implement a clear structure to my source code and to implement clean methods with clear names and unique resposibilities. I also tried to write clear comments.

What do you think about my program? Is there something that I can improve?

Viewer.java

import java.util.Arrays;
import java.util.List;
import java.util.ArrayList;
import java.util.Date;
import java.util.Properties;
import java.io.File;
import java.io.BufferedInputStream;
import java.io.FileInputStream;
import java.io.IOException;
public class Viewer {
 public static void main(String[] args) {
 Viewer viewer = new Viewer();
 viewer.view();
 }
 
 // contains all messages within two sub-dirs
 private File saveDir;
 // contains messages that have to be displayed
 private File currentDir;
 // contains messages that don't have to be displayed
 private File archiveDir;
 // determines the maximal amount of messages in the current directory
 private int currentMessagesCount;
 // last message read into program
 private File lastMessage;
 
 public Viewer() {
 // initialize members from file named config
 try (BufferedInputStream stream = new BufferedInputStream(new FileInputStream("config"))) {
 Properties config = new Properties();
 config.load(stream);
 currentMessagesCount = Integer.parseInt(config.getProperty("current_messages_count"));
 saveDir = new File(config.getProperty("save_dir"));
 } catch (IOException e) {
 e.printStackTrace();
 }
 
 currentDir = new File(saveDir, "current");
 archiveDir = new File(saveDir, "archive");
 lastMessage = new File("no last message");
 
 // create save directories if don't exist
 if (!saveDir.exists()) {
 saveDir.mkdir();
 }
 if (!currentDir.exists()) {
 currentDir.mkdir();
 }
 if (!archiveDir.exists()) {
 archiveDir.mkdir();
 }
 
 File[] messages = currentDir.listFiles();
 Arrays.sort(messages);
 }
 
 public void view() {
 printAllMessages();
 while (true) {
 try {
 Thread.sleep(500);
 } catch (InterruptedException e) {
 e.printStackTrace();
 }
 printNewMessage();
 checkLimit();
 }
 }
 
 // prints all messages immediately
 private void printAllMessages() {
 File[] messages = currentDir.listFiles();
 
 if (messages.length == 0) {
 return;
 }
 
 Arrays.sort(messages);
 for (int i = 0; i < messages.length; i++) {
 printMessage(messages[i]);
 }
 
 lastMessage = messages[messages.length - 1];
 }
 
 // prints newest if there is a more recent than last read message
 private void printNewMessage() {
 File[] messages = currentDir.listFiles();
 Arrays.sort(messages);
 
 if (messages.length == 0) {
 return;
 }
 
 // add all new messages to a list and print it out reversively
 List<File> newMessages = new ArrayList<>();
 for (int i = messages.length - 1; i >= 0 ; i--) {
 if (!messages[i].toString().equals(lastMessage.toString())) {
 newMessages.add(messages[i]);
 } else {
 break;
 }
 }
 
 while (newMessages.size() > 0) {
 File currentMessage = newMessages.get(newMessages.size() - 1);
 
 printMessage(currentMessage);
 newMessages.remove(currentMessage);
 lastMessage = currentMessage;
 }
 
 }
 
 private void printMessage(File file) {
 try (BufferedInputStream stream = new BufferedInputStream(new FileInputStream(file))) {
 Properties currentMessage = new Properties();
 currentMessage.load(stream);
 System.out.println(currentMessage.getProperty("date") + " (" + currentMessage.getProperty("nickname") + ")");
 System.out.println(currentMessage.getProperty("content") + "\n");
 } catch (IOException e) {
 e.printStackTrace();
 } 
 }
 
 // checks if there are too much messages in currentDir
 private void checkLimit() {
 File[] messages = currentDir.listFiles();
 Arrays.sort(messages);
 int numOfSuperfluousMessages = messages.length - currentMessagesCount;
 if (numOfSuperfluousMessages > 0) {
 for (int i = 0; i < numOfSuperfluousMessages; i++) {
 messages[i].renameTo(new File(archiveDir, messages[i].getName()));
 }
 }
 }
}

Writer.java

import java.util.Scanner;
import java.io.BufferedInputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileWriter;
import java.io.BufferedWriter;
import java.io.IOException;
import java.util.Calendar;
import java.util.Date;
import java.util.Properties;
public class Writer {
 public static void main(String[] args) {
 System.out.print("Your nickname: ");
 String nickname = scanner.nextLine();
 
 Writer writer = new Writer(nickname);
 writer.chat();
 }
 
 private static Scanner scanner = new Scanner(System.in);
 private String nickname;
 // directory in which messages get stored
 // initialized by file named config
 private File currentDir;
 
 public Writer(String nickname) {
 this.nickname = nickname;
 try (BufferedInputStream stream = new BufferedInputStream(new FileInputStream("config"))) {
 Properties config = new Properties();
 config.load(stream);
 currentDir = new File(config.getProperty("save_dir"), "current");
 } catch (IOException e) {
 e.printStackTrace();
 }
 
 // create current dir if not exists
 if (!currentDir.exists()) {
 currentDir.mkdirs();
 }
 }
 
 public void chat() {
 while (true) {
 System.out.print(nickname + " > ");
 String newContent = scanner.nextLine();
 long millis_bygone = new Date().getTime();
 
 File newMessage = new File(currentDir, new Long(millis_bygone).toString());
 
 // create file
 try {
 newMessage.createNewFile();
 } catch (IOException e) {
 e.printStackTrace();
 }
 
 // store message to file in currentDir
 try (BufferedWriter br = new BufferedWriter(new FileWriter(newMessage))) {
 br.write("nickname: " + nickname + "\n");
 
 // create date string
 Calendar cal = Calendar.getInstance();
 StringBuilder dateString = new StringBuilder();
 dateString.append(cal.get(Calendar.DAY_OF_MONTH) + "." + (cal.get(Calendar.MONTH) + 1) + "." + cal.get(Calendar.YEAR) + " ");
 dateString.append(cal.get(Calendar.HOUR_OF_DAY) + ":" + cal.get(Calendar.MINUTE));
 br.write("date: " + dateString + "\n");
 
 br.write("content: " + newContent + "\n");
 } catch (IOException e) {
 e.printStackTrace();
 }
 }
 }
}
asked Jul 10, 2020 at 8:04
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Very good implementation, commented and easy to understand. There is no much to improve but I am providing you my suggestions.

Configuration

  • Viewer and Writer cannot start without the configuration file, in that case it's better to provide a message to the user and exit gracefully
  • The logic to read the configuration file is duplicated, so it's better to move it in its own class

Exception handling

  • There are many operations with files but the exceptions are ignored

In your case you could handle exceptions in two ways: stop the application or ignore it. For example if the Viewer cannot read a message from a file is better to stop the application:

try (BufferedInputStream stream = new BufferedInputStream(new FileInputStream(file))) {
// ...
} catch (IOException e) {
 throw new RuntimeException("Error: unable to read the message in: "+file.getName());
}

The main will catch the runtime exception, print the error message and exit.

public static void main(String[] args) {
 try {
 Viewer viewer = new Viewer();
 viewer.view();
 } catch (Exception e) {
 System.out.print("Error: " + e.getMessage());
 }
}

The second way is easier, for example if the Writer cannot write to a file we can just print a warning and continue:

try (BufferedWriter br = new BufferedWriter(new FileWriter(newMessage))) {
// ...
} catch (IOException e) {
 System.out.println("Warning: cannot write to file " + newMessage);
}

Encapsulation

The chat message has its own format: a nickname (sender), a date and content. But this format is not clearly represented in a schema. Creating a class for the message will make it more evident and easier to change or extend.

Having more classes also means that you will need to build a jar file instead of running the .class directly. If your requirement is to keep the whole application inside a single .class file then you can put more classes in the same file.


Mismatch between name and behaviour

The method printMessage does more than printing a message to the console, it reads a file, parse it and finally prints it to console. It can be improved by splitting that logic.


Minor changes

  • Use Java 8 LocalDateTime and Instant instead of Date (more readable)
  • Let the user quit the chat in order to gracefully exit and close the Scanner

This is the code refactored:

public class ConfigFileReader {
 private String saveDir;
 private Integer currentMessagesCount;
 
 public ConfigFileReader(String fileName) {
 Properties config;
 try (BufferedInputStream stream = new BufferedInputStream(new FileInputStream(fileName))) {
 config = new Properties();
 config.load(stream);
 } catch (IOException e) {
 throw new RuntimeException("Cannot read config file");
 }
 // Read save_dir property or set it to the local folder if not found
 saveDir = config.getProperty("save_dir",".");
 // Read current_messages_count property or set it to 1 if not found
 currentMessagesCount = Integer.parseInt(config.getProperty("current_messages_count","1"));
 }
 
 public String getSaveDir() {
 return saveDir;
 }
 public Integer getCurrentMessagesCount() {
 return currentMessagesCount;
 }
 
}

The ChatMessage class:

public class ChatMessage {
 private String sender;
 private String content;
 private String date;
 
 public ChatMessage(String sender, String content) {
 this.sender = sender;
 this.content = content;
 this.date = now();
 }
 
 public ChatMessage(String sender, String content, String date) {
 this.sender = sender;
 this.date = date;
 this.content = content;
 }
 
 // Returns current date and time
 private String now() { 
 return LocalDateTime.now()
 .format(DateTimeFormatter.ofPattern("dd.MM.yyyy HH:mm"));
 }
 // Write message to file in currentDir
 public void saveToFile(File currentDir, String fileName) {
 File newMessage = new File(currentDir, fileName);
 // Create file
 try {
 newMessage.createNewFile();
 } catch (IOException e) {
 throw new RuntimeException("Cannot create file "+ fileName);
 }
 // Write message in the file
 try (BufferedWriter br = new BufferedWriter(new FileWriter(newMessage))) {
 br.write("nickname: " + this.sender + "\n");
 br.write("date: " + this.date + "\n");
 br.write("content: " + this.content + "\n");
 } catch (IOException e) {
 throw new RuntimeException("Cannot write to file "+ fileName);
 }
 }
 
 public static ChatMessage fromFile(File file) {
 ChatMessage result = null;
 try (BufferedInputStream stream = new BufferedInputStream(new FileInputStream(file))) {
 Properties currentMessage = new Properties();
 currentMessage.load(stream);
 String date = currentMessage.getProperty("date");
 String nickname = currentMessage.getProperty("nickname");
 String content = currentMessage.getProperty("content");
 result = new ChatMessage(nickname,content,date);
 } catch (IOException e) {
 throw new RuntimeException("Error: unable to read the message in: "+file.getName());
 }
 return result;
 }
 public String getSender() {
 return sender;
 }
 public String getDate() {
 return date;
 }
 public String getContent() {
 return content;
 }
}

The Writer:

public class Writer {
 public static void main(String[] args) {
 System.out.print("Your nickname: ");
 String nickname = scanner.nextLine();
 try {
 new Writer(nickname).chat();
 } catch (Exception e) {
 System.out.print("Error: " + e.getMessage());
 } finally {
 scanner.close();
 }
 }
 private static Scanner scanner = new Scanner(System.in);
 private String nickname;
 // directory in which messages get stored
 // initialized by file named config
 private File currentDir;
 public Writer(String nickname) {
 this.nickname = nickname;
 String saveDir = new ConfigFileReader("config").getSaveDir();
 currentDir = new File(saveDir, "current");
 // create current dir if not exists
 if (!currentDir.exists()) {
 currentDir.mkdirs();
 }
 }
 public void chat() {
 System.out.println("Start chatting or type quit to exit.");
 while (true) {
 System.out.print(nickname + " > ");
 String newContent = scanner.nextLine();
 if (newContent.strip().equalsIgnoreCase("quit"))
 break;
 // long millis_bygone = new Date().getTime();
 // String fileName = new Long(millis_bygone).toString();
 // Java 8
 String fileName = String.valueOf(Instant.now().toEpochMilli());
 ChatMessage message = new ChatMessage(nickname, newContent);
 try {
 message.saveToFile(currentDir, fileName);
 } catch (Exception e) {
 System.out.println("Warning: cannot write to file " + fileName);
 }
 }
 }
}

And finally the Viewer:

public class Viewer {
 public static void main(String[] args) {
 try {
 new Viewer().view();
 } catch (Exception e) {
 System.out.print("Error: " + e.getMessage());
 }
 }
 // contains all messages within two sub-dirs
 private File saveDir;
 // contains messages that have to be displayed
 private File currentDir;
 // contains messages that don't have to be displayed
 private File archiveDir;
 // determines the maximal amount of messages in the current directory
 private int currentMessagesCount;
 // last message read into program
 private File lastMessage;
 public Viewer() {
 // Read configuration file
 ConfigFileReader cfReader = new ConfigFileReader("config");
 
 currentMessagesCount = cfReader.getCurrentMessagesCount();
 saveDir = new File(cfReader.getSaveDir());
 currentDir = new File(saveDir, "current");
 archiveDir = new File(saveDir, "archive");
 lastMessage = new File("no last message");
 // create save directories if don't exist
 if (!saveDir.exists()) {
 saveDir.mkdir();
 }
 if (!currentDir.exists()) {
 currentDir.mkdir();
 }
 if (!archiveDir.exists()) {
 archiveDir.mkdir();
 }
 File[] messages = currentDir.listFiles();
 Arrays.sort(messages);
 }
 public void view() {
 printAllMessages();
 while (true) {
 try {
 Thread.sleep(500);
 } catch (InterruptedException e) {
 e.printStackTrace();
 }
 printNewMessage();
 checkLimit();
 }
 }
 // prints all messages immediately
 private void printAllMessages() {
 File[] messages = currentDir.listFiles();
 if (messages.length == 0) {
 return;
 }
 Arrays.sort(messages);
 for (int i = 0; i < messages.length; i++) {
 ChatMessage message = ChatMessage.fromFile(messages[i]);
 printMessage(message);
 }
 lastMessage = messages[messages.length - 1];
 }
 // prints newest if there is a more recent than last read message
 private void printNewMessage() {
 File[] messages = currentDir.listFiles();
 Arrays.sort(messages);
 if (messages.length == 0) {
 return;
 }
 // add all new messages to a list and print it out reversively
 List<File> newMessages = new ArrayList<>();
 for (int i = messages.length - 1; i >= 0; i--) {
 if (!messages[i].toString().equals(lastMessage.toString())) {
 newMessages.add(messages[i]);
 } else {
 break;
 }
 }
 while (newMessages.size() > 0) {
 File currentMessage = newMessages.get(newMessages.size() - 1);
 ChatMessage message = ChatMessage.fromFile(currentMessage);
 printMessage(message);
 newMessages.remove(currentMessage);
 lastMessage = currentMessage;
 }
 }
 private void printMessage(ChatMessage message) {
 System.out.println(message.getDate() + " (" + message.getSender() + ")");
 System.out.println(message.getContent());
 System.out.println();
 }
 // checks if there are too much messages in currentDir
 private void checkLimit() {
 File[] messages = currentDir.listFiles();
 Arrays.sort(messages);
 int numOfSuperfluousMessages = messages.length - currentMessagesCount;
 if (numOfSuperfluousMessages > 0) {
 for (int i = 0; i < numOfSuperfluousMessages; i++) {
 messages[i].renameTo(new File(archiveDir, messages[i].getName()));
 }
 }
 }
}
answered Jul 16, 2020 at 6:55
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Thank you for your great work! \$\endgroup\$ Commented Jul 17, 2020 at 7:34
  • 1
    \$\begingroup\$ Small note: I would avoid ever calling LocalDateTime.now() as that class cannot represent a moment. That class lacks the context of a time zone or offset-from-UTC, holding only a date with time-of-day. A date with time is inherently ambiguous (Noon in Tokyo? Noon In Toulouse? Noon in Toledo Ohio?). Instead use ZonedDateTime.now( ZoneId.systemDefault() ) if you want to capture the current moment in the JVM’s current default time zone. \$\endgroup\$ Commented Mar 30, 2024 at 20:25

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.