patternjavaMinor
Reading and writing to file
Viewed 0 times
andfilereadingwriting
Problem
Is there anything I can improve in my
```
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 loadAllUsers()
{
File f = new File("users.dat");
ArrayList 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
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 loadAllUsers()
{
File f = new File("users.dat");
ArrayList 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
Solution
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
This looks like a good candidate for extraction. It's not exactly the same as the code in
It's a little messy in the middle, but it has cleaner edges. We can now use it like so:
This does expose something that is odd about your code: Why does a
Now we have that
Some things to note quickly: If the file does not exist or cannot be read, it now throws an
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 loadAllUsers()
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 loadAllUsers()
{
File f = new File("users.dat");
ArrayList 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 loadAllUsers()
Code Snippets
User user = new User(null);
user.id = line.replace("[", "").replace("]", "");
user.password = br.readLine().split("=")[1];
user.username = br.readLine().split("=")[1];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];
}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;
}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));
}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;
}Context
StackExchange Code Review Q#97497, answer score: 9
Revisions (0)
No revisions yet.