3
\$\begingroup\$

I am writing a textual chat app for exercise purposes. All Chatters using my program have access to a network directory, and there my program is stored. This chat app consists of three programs. The user management program allows you to create and delete users. The messaging program allows you to write messages. The viewer program shows all messages.

Now I am about to find a strategy that is good for my viewer program. The code is not ordered yet, I just want to ask if the logic that is behind my code is suitable for a chat viewing program, or if there is a more sophisticated way to create such programs.

import java.io.FileInputStream;
import java.io.ObjectInputStream;
import java.io.IOException;
import java.io.File;

public class MessageViewerProgram {
    public static void main(String[] args) throws Exception {
        String saveDir = ".save/messages/";
        int fileName = 1;
        File file = new File(saveDir + fileName);
        while (true) {
            Thread.sleep(50);
            if (file.exists()) {
                Thread.sleep(20);
                try (FileInputStream fis = new FileInputStream(file);
                     ObjectInputStream ois = new ObjectInputStream(fis)) {
                    Message message = (Message) ois.readObject();
                    System.out.println(message.getSenderName() + ": " + message.getMessage());
                    fileName++;
                    file = new File(saveDir + fileName);
                } catch (IOException e) {
                    e.printStackTrace();
                } catch (ClassNotFoundException e) {
                    e.printStackTrace();
                }
            }
        }
    }
}

And this here is my "MessageWriterProgram". It is not finished yet, but it works already. I just post it because maybe it is needed to understand the problem, but you dont have to review the following class:

import java.util.Scanner;
import java.io.File;
import java.io.FileOutputStream;
import java.io.ObjectOutputStream;
import java.io.IOException;

public class MessageWriterProgram {
    public static void main(String[] args) {
        MessageWriterProgram mwp = new MessageWriterProgram();
        mwp.mainLoop();
    }

    private static final String SAVE_DIR_NAME = ".save/messages/";

    private Scanner scanner = new Scanner(System.in);
    private UserManager userManager;
    private User identity;

    public MessageWriterProgram() {
        scanner = new Scanner(System.in);
        userManager = new UserManager();
    }

    public void mainLoop() {
        if (!login()) {
            System.out.println("Login was not successful.");
            return;
        }
        System.out.println(identity.getName());

        while (true) {
            System.out.print("> ");
            String input = scanner.nextLine();
            if (!input.isEmpty()) {
                Message message = new Message(identity.getName(), input);
                saveMessage(message);
            }
        }
    }

    public void saveMessage(Message message) {
        // create file name that isn't used
        int filename = 1;
        while (true) {
            File file = new File(SAVE_DIR_NAME + filename);
            if (file.exists()) {
                filename++;
            } else {
                break;
            }
        }
        File file = new File(SAVE_DIR_NAME + filename);

        // store message
        try (FileOutputStream fos = new FileOutputStream(file);
             ObjectOutputStream oos = new ObjectOutputStream(fos)) {
            oos.writeObject(message);
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    // returns if login was successful
    public boolean login() {
        System.out.print("Name: ");
        String name = scanner.nextLine();
        System.out.print("Password: ");
        String password = scanner.nextLine();
        identity = userManager.retainUserIdentity(name, password);
        return identity != null;
    }
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

I have some suggestions.

MessageViewerProgram

  1. Instead of brute forcing all the messages in the directory, I suggest that you list them and iterate on them instead. You can use the method java.io.File#listFiles() for this.
File file = new File(saveDir);
File[] files = file.listFiles();

This will give you an array of the files in the message folder, and you can iterate.

File saveDir = new File(".save/messages/");

for (File currentMessageFile : saveDir.listFiles()) {
   try (FileInputStream fis = new FileInputStream(currentMessageFile);
            ObjectInputStream ois = new ObjectInputStream(fis)) {

      Message message = (Message) ois.readObject();
      System.out.println(message.getSenderName() + ": " + message.getMessage());
   } catch (IOException e) {
      e.printStackTrace();
   } catch (ClassNotFoundException e) {
      e.printStackTrace();
   }
}

MessageWriterProgram

  1. The initialization of the MessageWriterProgram#scanner variable outside the constructor is useless, it's created inside the constructor already.
private Scanner scanner;

public MessageWriterProgram() {
   scanner = new Scanner(System.in);
}
  1. I suggest that you rename the method mainLoop in start or run or even execute.

MessageWriterProgram#saveMessage method

  1. Rename the variable filename into fileName

  2. I suggest that you extract the logic of counting the next file name in a method.

public void saveMessage(Message message) {
   // create file name that isn't used
   int fileName = getNextFilename();
   //[...]
}

private int getNextFilename() {
   int fileName = getNextFilename();

   while (true) {
      File file = new File(SAVE_DIR_NAME + fileName);
      if (file.exists()) {
         fileName++;
      } else {
         break;
      }
   }

   return fileName;
}
  1. Instead of counting the messages in the loop, you can use the java.io.File#listFiles() and check the number of files in the folder.
private int getNextFilename() {
   File messageDirectory = new File(SAVE_DIR_NAME);
   File[] files = messageDirectory.listFiles();

   if (files != null) {
      return files.length + 1;
   } else {
      return 1;
   }
}

MessageWriterProgram#login method

In my opinion, this method is a bit useless and add confusion, since it does more than one thing.

  • Check if the User is present.
  • Update the identity variable.

If you inline it, it will be more readable and you can convert the identity variable to a local variable.

System.out.print("Name: ");
String name = scanner.nextLine();
System.out.print("Password: ");
String password = scanner.nextLine();

User identity = userManager.retainUserIdentity(name, password);

if (identity == null) {
   System.out.println("Login was not successful.");
   return;
}

Refactored MessageWriterProgram class

public class MessageWriterProgram {
   public static void main(String[] args) {
      MessageWriterProgram mwp = new MessageWriterProgram();
      mwp.mainLoop();
   }

   private static final String SAVE_DIR_NAME = ".save/messages/";

   private Scanner scanner;
   private UserManager userManager;

   public MessageWriterProgram() {
      scanner = new Scanner(System.in);
      userManager = new UserManager();
   }

   public void mainLoop() {
      System.out.print("Name: ");
      String name = scanner.nextLine();
      System.out.print("Password: ");
      String password = scanner.nextLine();

      User identity = userManager.retainUserIdentity(name, password);

      if (identity == null) {
         System.out.println("Login was not successful.");
         return;
      }

      System.out.println(identity.getName());

      while (true) {
         System.out.print("> ");
         String input = scanner.nextLine();

         if (!input.isEmpty()) {
            Message message = new Message(identity.getName(), input);
            saveMessage(message);
         }
      }
   }

   public void saveMessage(Message message) {
      // create file name that isn't used
      int fileName = getNextFilename();

      File file = new File(SAVE_DIR_NAME + fileName);

      // store message
      try (FileOutputStream fos = new FileOutputStream(file);
               ObjectOutputStream oos = new ObjectOutputStream(fos)) {
         oos.writeObject(message);
      } catch (IOException e) {
         e.printStackTrace();
      }
   }

   private int getNextFilename() {
      File messageDirectory = new File(SAVE_DIR_NAME);
      File[] files = messageDirectory.listFiles();

      if (files != null) {
         return files.length + 1;
      } else {
         return 1;
      }
   }
}
\$\endgroup\$
1
  • \$\begingroup\$ That's an excellent code review, just wow! \$\endgroup\$ Commented Jan 28, 2020 at 17:38

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.