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.
Propertiesto store this data? The class has a load- and store-method, and their key and values can be splittet by a"=". \$\endgroup\$