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();
}
}
}
}
1 Answer 1
Very good implementation, commented and easy to understand. There is no much to improve but I am providing you my suggestions.
Configuration
Viewer
andWriter
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
andInstant
instead ofDate
(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()));
}
}
}
}
-
2\$\begingroup\$ Thank you for your great work! \$\endgroup\$Dexter Thorn– Dexter Thorn2020年07月17日 07:34:33 +00:00Commented 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 useZonedDateTime.now( ZoneId.systemDefault() )
if you want to capture the current moment in the JVM’s current default time zone. \$\endgroup\$Basil Bourque– Basil Bourque2024年03月30日 20:25:12 +00:00Commented Mar 30, 2024 at 20:25