Code Review Asked by Emmanuel Okafor on January 4, 2022
I recently created a program that could get information from a client, a name and some text, and then put that into a text file. From there the user can get open the text file straight from the program. I made it in two days. Please give me some tips in ways I can improve it, thank you!
The Server:
import java.io.*;
import java.net.*;
import java.nio.*;
import java.nio.channels.FileChannel;
import java.nio.file.*;
public class Server {
static boolean isAvailable = false;
public static void main(String[] args) throws IOException {
// Get both connections to the client
ServerSocket serverSocket =
new ServerSocket(4999);
ServerSocket serverSocket2 =
new ServerSocket(4998);
Socket socket =
serverSocket.accept();
Socket socket2 =
serverSocket2.accept();
// Confirm client connection
System.out.println("The client has been connected to the server");
// Get the streams from the client containing the string
InputStreamReader fileNameIn =
new InputStreamReader(socket.getInputStream());
BufferedReader br =
new BufferedReader(fileNameIn);
InputStreamReader informationIn =
new InputStreamReader(socket2.getInputStream());
BufferedReader br2 =
new BufferedReader(informationIn);
// Read and assign the strings
String fileName =
br.readLine();
String fileInformation =
br2.readLine();
// Get char array to write to the created file
char[] fileInformationC =
fileInformation.toCharArray();
// Create file and write to it
try (FileChannel fileChannel =
(FileChannel) Files.newByteChannel(
Path.of("C:\Users\Emman\" + fileName + ".txt"),
StandardOpenOption.CREATE,
StandardOpenOption.WRITE
)) {
int allocatedVal = 1000;
ByteBuffer byteBuffer =
ByteBuffer.allocate(allocatedVal);
for (char c : fileInformationC){
byteBuffer.put((byte) c);
// Error handling: it is now impossible for the Buffer to run out of space
// However a BufferOverflowException is caught, in case of a problem
if (!(byteBuffer.position() == allocatedVal)){
ByteBuffer.allocate(allocatedVal * 2);
}
}
byteBuffer.rewind();
fileChannel.write(byteBuffer);
System.out.println("File has been created and written to, n" +
"Press Enter to open file");
// The input does not matter
//noinspection ResultOfMethodCallIgnored
System.in.read();
ProcessBuilder processBuilder =
new ProcessBuilder("notepad.exe", "C:\Users\Emman\" + fileName + ".txt");
processBuilder.start();
} catch (InvalidPathException e) {
isAvailable = true;
PrintWriter printToClient =
new PrintWriter(socket.getOutputStream());
printToClient.write(""Error in creating/writing to file \n" +n" +
"Due to incorrect/invalid path" +n" +
"e");
printToClient.close();
} catch (IOException e) {
isAvailable = true;
PrintWriter printToClient =
new PrintWriter(socket.getOutputStream());
printToClient.write("Error in creating/writing to file n" +
"Due to error in IO" +
e);
printToClient.close();
} catch (BufferOverflowException e) {
isAvailable = true;
PrintWriter printToClient =
new PrintWriter(socket.getOutputStream());
printToClient.write("Error in creating/writing to file n" +
"Due to a buffer overflow" +
e);
printToClient.close();
}
}
}
The Client:
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.Socket;
public class Client {
public static void main(String[] args) throws IOException{
// Create connections to server
Socket socket =
new Socket("localhost", 4999);
Socket socket2 =
new Socket("localhost", 4998);
// Create print writer output stream to give to server
PrintWriter pr =
new PrintWriter(socket.getOutputStream());
PrintWriter pr2 =
new PrintWriter(socket2.getOutputStream());
// Create a buffered reader for reading the file name and information
BufferedReader br =
new BufferedReader(
new InputStreamReader(
System.in
)
);
// Get the file names and information
String fileName = br.readLine();
String information = br.readLine();
// Put in the file Name and the file information
pr.write(fileName);
pr.close();
pr2.write(information);
pr2.close();
// Checking whether the error message is available,
// if not only one line of code is read, wasting barely any time
if(Server.isAvailable){
InputStreamReader errorInfoIn =
new InputStreamReader(socket.getInputStream());
BufferedReader readErrorInfo =
new BufferedReader(errorInfoIn);
String errorInfo =
readErrorInfo.readLine();
System.out.println(errorInfo);
errorInfoIn.close();
}
}
}
Don't put everything in a single function. That makes your code difficult to maintain and reuse, even by yourself. The ideal function size is so small that one can no longer reasonably extract further functions from it. In your code, each program is just one function, quite the opposite of the ideal.
All resources should be handled with try
-with-resources constructs, not only the fileChannel
.
It is not possible for a reviewer to run your program without changing it first, because you use an absolute, operating-system-specific and user-specific path in your program: Path.of("C:\Users\Emman\" + fileName + ".txt")
.
If you're serious about learning network programming, you may want to do some sanitation to fileName
. What if it contains ..
? What if somebody sends ....WINNTexplorer.exe
or something like that as filename?
The way how your program is written, isAvailable
can and should be a local variable. In general, having non-final
static
variables is considered bad design. It prevents multiple instances of your class to function independently in parallel.
Answered by Christian Hujer on January 4, 2022
Get help from others!
Recent Answers
Recent Questions
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP