Reading and writing to file

Question

Is there anything I can improve in my UserManager class which saves and loads info about users connected to my server?

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
import java.net.Socket;
import java.util.ArrayList;
import server.engine.CustomLog;
public class UserManager {
public static synchronized User loadUser(String id, Socket s) {
 User user = null;
 if (id == null) {
 return user;
 }
 File f = new File("users.dat");
 if (f.isFile() && f.canRead()) {
 try (BufferedReader br = new BufferedReader(new FileReader(f))) {
 String line;
 while ((line = br.readLine()) != null) {
 if (line.equals("[" + id + "]")) {
 user = new User(s);
 user.id = id;
 user.password = br.readLine().split("=")[1];
 user.username = br.readLine().split("=")[1];
 break;
 }
 }
 } catch (IOException ex) {
 CustomLog.error(ex.getMessage());
 }
 }
 return user;
}
public static synchronized ArrayList<User> loadAllUsers()
{
 File f = new File("users.dat");
 ArrayList<User> users = new ArrayList();
 if (f.isFile() && f.canRead()) {
 try (BufferedReader br = new BufferedReader(new FileReader(f))) {
 String line;
 while ((line = br.readLine()) != null) {
 if (line.matches("^(\\[[0-9]*\\])$")) {
 User user = new User(null);
 user.id = line.replace("[", "").replace("]", "");
 user.password = br.readLine().split("=")[1];
 user.username = br.readLine().split("=")[1];
 users.add(user);
 }
 }
 } catch (IOException ex) {
 CustomLog.error(ex.getMessage());
 }
 }
 return users;
}
public static synchronized void saveUser(User user) {
 File f = new File("users.dat");
 String content = "";
 String newLine = System.getProperty("line.separator");
 boolean found = false;
 if (f.isFile() && f.canRead()) {
 try (BufferedReader br = new BufferedReader(new FileReader(f))) {
 String line;
 while ((line = br.readLine()) != null) {
 if (line.equals("[" + user.id + "]") && br.readLine().equals(user.password)) {
 found = true;
 content += "[" + user.id + "]" + newLine;
 content += "password=" + user.password + newLine;
 content += "username=" + user.username + newLine;
 br.readLine();
 } else {
 content += line + newLine;
 }
 }
 } catch (IOException ex) {
 CustomLog.error(ex.getMessage());
 }
 }
 if (!found) {
 content += "[" + user.id + "]" + newLine;
 content += "password=" + user.password + newLine;
 content += "username=" + user.username + newLine;
 }
 try (BufferedWriter writer = new BufferedWriter(new FileWriter(f))) {
 writer.write(content);
 writer.close();
 } catch (FileNotFoundException | UnsupportedEncodingException ex) {
 CustomLog.error(ex.getMessage());
 } catch (IOException ex) {
 CustomLog.error(ex.getMessage());
 }
}
}

EDIT: I forgot to mention that my client and server uses auto-register. Basically id and the password are randomly generated when the client requests registration and server saves them as a new user and client saves them too and keeps using them to log in. So user is only prompted to enter his username. Thats why I was not bothered about privacy of the passwords because they are just random numbers and letters.

• IEatBagels: Are your password stored in clear text in a text file?
• Ladas125: Yeah, they are.
• Marco Acierno: Don't store password in plain text
• bobbel: What about using Properties to store this data? The class has a load- and store-method, and their key and values can be splittet by a "=".
• Marco Acierno: The fact that you decide to implement a login/register feature in your site gives you the responsibility to keep user private data.. private. You will not have any benefit. Just more code to write.

Answer 1

First, the obligatory don't store passwords in plain text! If this is any type of production app, you should (and I believe must) follow security best practices for both your sake and the sake of your users.

Now, there is a lot of duplication in this code, and very little code reuse. For example, you have both loadUser and loadAllUsers methods that both load Users from a file. Duplicated parts include getting the file handle with the file name, reading over the file in a loop, parsing the lines for user information, and putting that all into a User object. (saveUser duplicates some of these functionalities as well.) By following the Single Responsibility Principle, we can see that we should extract out some of these responsibilities into individual methods.

User user = new User(null);
user.id = line.replace("[", "").replace("]", "");
user.password = br.readLine().split("=")[1];
user.username = br.readLine().split("=")[1];

This looks like a good candidate for extraction. It's not exactly the same as the code in loadUser, but it's close. The biggest difference is that instead of passing a Socket into the user, we pass a null. The second biggest difference is that the id is parsed from the line rather than passed in directly. The latter can be addressed by just ignoring it. We can extract the id back out even if we are supplied it, and it would make virtually no difference. The former can be addressed by allowing a parameter to be supplied, and supplying null where we expect null. So we can extract the following method:

private static User getUserFromLines(String[] lines, Socket s) {
 User user = new User(s);
 user.id = lines[0].replace("[", "").replace("]", "");
 user.password = lines[1].split("=")[1];
 user.username = lines[2].split("=")[1];
}

It's a little messy in the middle, but it has cleaner edges. We can now use it like so:

public static synchronized User loadUser(String id, Socket s) {
 User user = null;
 if (id == null) {
 return user;
 }
 File f = new File("users.dat");
 if (f.isFile() && f.canRead()) {
 try (BufferedReader br = new BufferedReader(new FileReader(f))) {
 String line;
 while ((line = br.readLine()) != null) {
 if (line.equals("[" + id + "]")) {
 user = getUserFromLines(new String[] {
 line, br.readLine(), br.readLine()
 }, s);
 break;
 }
 }
 } catch (IOException ex) {
 CustomLog.error(ex.getMessage());
 }
 }
 return user;
}
public static synchronized ArrayList<User> loadAllUsers()
{
 File f = new File("users.dat");
 ArrayList<User> users = new ArrayList();
 if (f.isFile() && f.canRead()) {
 try (BufferedReader br = new BufferedReader(new FileReader(f))) {
 String line;
 while ((line = br.readLine()) != null) {
 if (line.matches("^(\\[[0-9]*\\])$")) {
 users.add(getUserFromLines(new String[] {
 line, br.readLine(), br.readLine()
 }, null));
 }
 }
 } catch (IOException ex) {
 CustomLog.error(ex.getMessage());
 }
 }
 return users;
}

This does expose something that is odd about your code: Why does a User take a Socket? I can't speak too much to that because we don't have the code behind User, but it is a rather large code smell indicating a potential design problem.

Now we have that BufferedReader we keep using. Let's encapsulate that behind a method so we don't have to worry so much about how we get it:

private static BufferedReader getUsersFileReader() throws IOException {
 File f = new File("users.dat");
 if(!f.isFile() || !f.canRead()) {
 throw new IOException("Can't find users file!");
 }
 return new BufferedReader(new FileReader(f));
}

Some things to note quickly: If the file does not exist or cannot be read, it now throws an IOException (which gets caught by your try-with-resources block). This is an exceptional case that at least should be logged, and possibly should even be bubbled up. Ignoring it could mask errors, leading to headaches and debugging.

We can use our new method as follows:

public static synchronized User loadUser(String id, Socket s) {
 User user = null;
 if (id == null) {
 return user;
 }
 try (BufferedReader br = getUsersFileReader()) {
 String line;
 while ((line = br.readLine()) != null) {
 if (line.equals("[" + id + "]")) {
 user = getUserFromLines(new String[] {
 line, br.readLine(), br.readLine()
 }, s);
 break;
 }
 }
 } catch (IOException ex) {
 CustomLog.error(ex.getMessage());
 }
 return user;
}
public static synchronized ArrayList<User> loadAllUsers()
{
 ArrayList<User> users = new ArrayList();
 try (BufferedReader br = getUsersFileReader()) {
 String line;
 while ((line = br.readLine()) != null) {
 if (line.matches("^(\\[[0-9]*\\])$")) {
 users.add(getUserFromLines(new String[] {
 line, br.readLine(), br.readLine()
 }, null));
 }
 }
 } catch (IOException ex) {
 CustomLog.error(ex.getMessage());
 }
 return users;
}

Now a couple of points about other aspects of your code. All of your methods are synchronized, but you don't appear to have anything that requires synchronizing. This creates overhead without any benefit. What you may actually be looking for is file locking. The file should not be read while it is being written, or it will give corrupt results (reads, on the other hand, can happen in parallel). There are ways to accomplish file locking in Java, but it might be better to look at using a database as a persistence back-end. A database will handle all of the concurrency concerns for you, as well as providing other benefits. Right now, you find entries by iterating through the file. A database could optimize such a look-up with an index. Lightweight, file-based databases, like SQLite, are available if you don't want to run a heavy database server on your machine.

Your loadAllUsers method explicitly returns an ArrayList<User>. While on the surface, this doesn't look like anything to be concerned about, let's say we discover down the line that ArrayList is a performance bottleneck, and we'd rather use LinkedList. Because the method signature is so specific, we can't change the underlying implementation transparently. Instead, we should return a higher-level abstraction like a List<User>. Now we can internally switch the type of list without any external effects.

From the code provided, we can see that your User class exposes public properties for setting state. Doing this means that the User objects cannot have full control over their internals. They cannot guarantee consistency or correctness. It would be better to hide those properties, and to allow setting them through setters or through the constructor.

You will also see that I've added some vertical whitespace, some newlines between chunks of code. Vertical space is free, and it helps to break up the code into readable chunks.

In each of your original methods, you had a line (File f = new File("users.dat")) that contains both a dependency and configuration. This is a sign of two things: the repeated creation of the same state signals that this should probably be an instance object rather than a set of static methods; the configuration data tells us that the dependency should probably be injected (facilitated by making this an instance object, BTW).

So we start with a basic constructor that encapsulates our state:

private File usersFile;
public UserManager() {
 usersFile = new File("users.dat");
}

And we change our code to match. All of the statics go away, and we adjust the getUsersFileReader method to use our new property.

private BufferedReader getUsersFileReader() throws IOException {
 if(!usersFile.isFile() || !usersFile.canRead()) {
 throw new IOException("Can't find users file!");
 }
 return new BufferedReader(new FileReader(usersFile));
}

We can now write a constructor that accepts a File, so we can inject our dependency instead of just accepting the default:

public UserManager(File usersFile) {
 this.usersFile = usersFile;
}

Now that we have pulled this dependency up, we can start to see something interesting about our class. Going back to the Single Responsibility Principle, we can more clearly see that our class has more than one responsibility: it is doing file I/O, and it is doing string parsing to build Users (and the reverse). We should extract out one of those responsibilities into a new class. Because of the name of this class, building Users seems like the right responsibility to keep here. (FYI, this class is a User repository, a factory for building and saving objects. It is a common pattern in the wild.)

So we should extract out the file I/O. So if we extract that, what is the interface we want to deal with? We want something that provides BufferedReaders and BufferedWriters. We don't care where they come from. Let's write that interface:

public interface UserData {
 public BufferedReader getReader();
 public BufferedWriter getWriter();
}

Now let's write the class that fulfills that interface:

public class UserDataFile implements UserData {
 private File usersFile;
 public UserDataFile(File usersFile) {
 this.usersFile = usersFile;
 }
 public BufferedReader getReader() throws IOException {
 if(!usersFile.isFile() || !usersFile.canRead()) {
 throw new IOException("Can't find users file!");
 }
 return new BufferedReader(new FileReader(usersFile));
 }
 public BufferedWriter getWriter() throws IOException {
 if(!usersFile.isFile() || !usersFile.canWrite()) {
 throw new IOException("Can't write to users file!");
 }
 return new BufferedWriter(new FileWriter(usersFile));
 }
}

This is a very simple class, but it hides anything having to do with files from our other class. The extracted interface allows us to switch out back-ends at runtime. We could write different implementations for testing, or for getting our data over the network, or for many other scenarios. It doesn't matter to UserManager. It would look like this now:

public class UserManager {
 private UserData userData;
 public UserManager(UserData userData) {
 this.userData = userData;
 }
 public User loadUser(String id, Socket s) {
 User user = null;
 if (id == null) {
 return user;
 }
 try (BufferedReader br = userData.getReader()) {
 String line;
 while ((line = br.readLine()) != null) {
 if (line.equals("[" + id + "]")) {
 user = getUserFromLines(new String[] {
 line, br.readLine(), br.readLine()
 }, s);
 break;
 }
 }
 } catch (IOException ex) {
 CustomLog.error(ex.getMessage());
 }
 return user;
 }
 public List<User> loadAllUsers() {
 ArrayList<User> users = new ArrayList();
 try (BufferedReader br = userData.getReader()) {
 String line;
 while ((line = br.readLine()) != null) {
 if (line.matches("^(\\[[0-9]*\\])$")) {
 users.add(getUserFromLines(new String[] {
 line, br.readLine(), br.readLine()
 }, null));
 }
 }
 } catch (IOException ex) {
 CustomLog.error(ex.getMessage());
 }
 return users;
 }
 public void saveUser(User user) {
 String content = "";
 String newLine = System.getProperty("line.separator");
 boolean found = false;
 try (BufferedReader br = userData.getReader()) {
 String line;
 while ((line = br.readLine()) != null) {
 if (line.equals("[" + user.id + "]") && br.readLine().equals(user.password)) {
 found = true;
 content += "[" + user.id + "]" + newLine;
 content += "password=" + user.password + newLine;
 content += "username=" + user.username + newLine;
 br.readLine();
 } else {
 content += line + newLine;
 }
 }
 } catch (IOException ex) {
 CustomLog.error(ex.getMessage());
 }
 if (!found) {
 content += "[" + user.id + "]" + newLine;
 content += "password=" + user.password + newLine;
 content += "username=" + user.username + newLine;
 }
 try (BufferedWriter writer = userData.getWriter()) {
 writer.write(content);
 writer.close();
 } catch (FileNotFoundException | UnsupportedEncodingException ex) {
 CustomLog.error(ex.getMessage());
 } catch (IOException ex) {
 CustomLog.error(ex.getMessage());
 }
 }
 private User getUserFromLines(String[] lines, Socket s) {
 User user = new User(s);
 user.id = lines[0].replace("[", "").replace("]", "");
 user.password = lines[1].split("=")[1];
 user.username = lines[2].split("=")[1];
 }
}

There's a lot more that could still be done (see the other answers for other pointers), but this begins to move the code forward. It is focused on having a more singular responsibility. It is more modular, and depends more upon abstractions than concretions.

• Ladas125: Thank you a lot for your very detailed answer, I really appreciate it. I might switch to database anyways because of faster and better working with users data. About storing the passwords check the bottom of the question, it explains why the passwords are stored as plain text. To explain the socket in the User constructor, it's because of each listening Thread has assigned a User to itself so when a PacketHandler in my server handles a packet it is used to simply send server's packets to the user who sent packets to server.

Answer 2

Is there any reason why all your methods are sychronized? There are no shared variables, so the methods could be stateless. The passed Socket to loadUser could be a reason why synchronized is needed. It is unusual to for the class User, to have a constructor which accepts a Socket. Data from the Socket should probably be read elsewhere and after that the constructor could be called.

Instead of the File class with its isFile() and canRead() methods, the more modern Path-based API could be used. see https://stackoverflow.com/questions/6903335/java-7-path-vs-file

edit: the properties of the User class seem to be public or protected. This should be replaced by setters or a constructor which makes sure your object is in a consistent state. like

public User(String id, String userName, String password)
{
this.id=id;
// etc.
}

Answer 3

Some tips

• Use StringBuffer (thread safe) and StringBuilder (non-thread-safe) when you do many appends as you are doing.
• Use String.format instead of + operator to build strings if performance is important.
• I don't know how big are your lines when you use readline() in your code, but if you have big strings, it may be beneficial to use regex instead of split.

• bobbel: Regular expressions are usual very slow. Furthermore String.split uses also a regular expression to split the string. So, the fastest way would probably be a combination of String.indexOf("=") and a String.substring.
• h.j.k.: @bobbel on that note JDK 7 introduced a 'fast-path' for single-character split() calls... see this Stackoverflow answer for more info.