I am studying Java. Can you criticize my code and tell me what I need to do to make it better?

I have code that reads a txt file(product base). It contains tabular data where lines are formatted as:

id productName price quantity

We launch the code with:

-u id productName price quantity - update line;

or

-d id - delete line.

So the logic is to find this line, create new File with updated line or without it, delete base file and rename new file.

Last important thing: every element on textbase has own weight. It's int argsLength. If the element size less that its weight, we fill empty space with whitespaces.

simple for test(indents is correct):

1 Recorder 100.00 12
212 Rocket 182.00 400
99333 Hat 4500.00 5
1984711 Crocodile 2.5 4339
13247983Pistol 53500.903

https://ru.files.fm/u/5q3zb94a

main:

public class CRUD 
 public static void main(String args) 
 try 
 BufferedReader reader = new BufferedReader(new InputStreamReader(System.in));
 String filename = reader.readLine();
 BufferedReader fileReader = new BufferedReader(new FileReader(filename));

 int argsLength = new int0, 8, 30, 8, 4;

 FileCreator creator = new FileCreatorFactory().getFileCreator(args[0], args, argsLength, filename);
 String line;

 if (creator == null) 
 System.out.println("Unknow command");
 return;
 
 if ((line = creator.isLineIsset()) == null) 
 System.out.println("Unknow ID");
 return;
 

 File resultFile = creator.createNewFile(line);

 if (resultFile.length() == 0) 
 System.out.println("Error");
 return;
 

 reader.close();
 fileReader.close();
 Files.delete(new File(filename).toPath());
 System.out.println("Result:" + (resultFile.renameTo(new File(filename))));
 catch (IOException e) 
 e.printStackTrace();
 
 

FileCreatorFactory:

package ru.kirstentasks.filecreator;

public class FileCreatorFactory 
 public FileCreator getFileCreator(String arg,String args, int argsMaxLength,String filename) 
 switch (arg) 
 case "-u":
 return new idUpdater(filename,args,argsMaxLength);
 case "-d":
 return new idDeleter(filename,args,argsMaxLength);
 default:
 return null;
 
 

FileCreator:

public abstract class FileCreator 
 protected String args;
 protected int argsMaxLength;
 protected String fileName;

 FileCreator(String fileName, String args, int argsMaxLength) 
 this.args = args;
 this.argsMaxLength = argsMaxLength;
 this.fileName = fileName;
 

 public String isLineIsset() throws IOException 
 String result;
 BufferedReader reader = new BufferedReader(new FileReader(fileName));
 while (!((result = reader.readLine()) == null)) 
 if (args[1].trim().equals(result.substring(0, argsMaxLength[1]).trim())) 
 reader.close();
 return result;
 
 
 reader.close();
 return null;
 

 public abstract File createNewFile(String line);

idDeleter:

package ru.kirstentasks.filecreator;

import java.io.*;

public class idDeleter extends FileCreator 
 private String fileName;

 idDeleter(String fileName, String args, int argsMaxLength) 
 super(fileName, args, argsMaxLength);
 this.fileName = fileName;
 


 @Override
 public File createNewFile(String line) 
 BufferedReader fileReader;
 BufferedWriter fileWriter;

 File tempFile = new File(fileName + ".temp");

 try 
 fileReader = new BufferedReader(new FileReader(fileName));
 fileWriter = new BufferedWriter(new FileWriter(tempFile));
 String temp;
 while (!((temp = fileReader.readLine()) == null)) 
 if (temp.equals(line)) 
 continue;
 
 fileWriter.write(temp);
 fileWriter.newLine();
 
 fileReader.close();
 fileWriter.close();
 catch (IOException e) 
 e.printStackTrace();
 
 return tempFile;
 

idUpdater:

package ru.kirstentasks.filecreator;

import java.io.*;

public class idUpdater extends FileCreator 
 private String filename;

 idUpdater(String filename, String args, int argsMaxLength) 
 super(filename, args, argsMaxLength);
 this.filename = filename;
 

 @Override
 public File createNewFile(String line) 
 BufferedReader fileReader;
 BufferedWriter fileWriter;

 File tempFile = new File(filename + ".temp");

 try 
 fileReader = new BufferedReader(new FileReader(filename));
 fileWriter = new BufferedWriter(new FileWriter(tempFile));
 String temp;
 while (!((temp = fileReader.readLine()) == null)) 
 if (temp.equals(line)) 
 temp = createLine(args, argsMaxLength);
 
 fileWriter.write(temp);
 fileWriter.newLine();
 
 fileReader.close();
 fileWriter.close();
 catch (IOException e) 
 e.printStackTrace();
 
 return tempFile;
 

 private String createLine(String args, int argsLength) 
 if (args.length != argsLength.length) 
 return null;
 
 StringBuilder sb = new StringBuilder();
 for (int i = 1; i < args.length; i++) 
 sb.append(String.format("%-" + argsLength[i] + "s", args[i]));
 
 return sb.toString();
 

• Classes in Java begin with a capital letter. The use of multi-caps acronyms is discouraged as they are difficult to read. Prefer Crud to CRUD and IdUpdater to idUpdater.




• You must always close readers that you open. The preferred way to do this is with a try-with-resources block. Old-school is to use a try-finally construct.




• A product seems like a top-level thing that should possibly be represented by an object rather than a String. This problem is small enough that it might be overkill to create an object, but it does make things easier.




• The idea of having separate actions that encapsulate deleting and updating is sound, but the implementation could use some work, since the parent class has multiple concepts that only apply to one or the other. You can go a long way with a simple one-method interface, since most of the code is really shared between the two operations. The only difference is what you do when you find a matching line. Everything else is duplicated.




• Don't use a public class with a package-private constructor unless you need to. You can make the class package-private also.




• The factory class is probably overkill for just two arguments.




• You're not really handling IOException at all, so you may as well let it percolate out. The final output will be the same either way.

I took a quick stab at applying my suggestions to your existing code. The end result (untested) looks like:

Crud

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.File;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.nio.file.Files;
import java.util.Scanner;

public final class Crud 

 public static void main(final String args)
 throws IOException 

 final ProductHandler productHandler;
 switch(args[0]) 
 case "-u":
 productHandler = new IdUpdater(new Product(new String args[1], args[2], args[3], args[4]));
 break;
 case "-d":
 productHandler = new IdDeleter(args[1]);
 break;
 default:
 System.out.println("Unknown command: " + args[0]);
 return;
 

 final File file = new File(filename());
 final File tempFile = File.createTempFile("", "");

 try (final Scanner console = new Scanner(System.in);
 final FileReader fileReader = new FileReader(file);
 final BufferedReader bufferedReader = new BufferedReader(fileReader);
 final FileWriter fileWriter = new FileWriter(tempFile);
 final BufferedWriter bufferedWriter = new BufferedWriter(fileWriter)) 

 boolean found = false;
 String line;
 while ((line = bufferedReader.readLine()) != null) = productHandler.handle(product, bufferedWriter);
 

 if (!found) 
 System.out.println("No id matching '" + args[1] + "' found");
 

 Files.delete(file.toPath());
 System.out.println("Result:" + (tempFile.renameTo(file)));
 
 

 private static String filename() 
 try (final Scanner scanner = new Scanner(System.in)) 
 return scanner.nextLine();
 
 

IdDeleter

import java.io.IOException;
import java.io.Writer;

class IdDeleter implements ProductHandler 

 private final String id;

 IdDeleter(final String id) 
 this.id = id;
 


 @Override
 public boolean handle(final Product product, final Writer writer)
 throws IOException 

 if (product.hasId(this.id)) 
 return true;
 

 writer.write(product.asString());
 return false;
 

IdUpdater

import java.io.IOException;
import java.io.Writer;

final class IdUpdater implements ProductHandler 

 private final Product updatedProduct;

 public IdUpdater(final Product updatedProduct) 
 this.updatedProduct = updatedProduct;
 

 @Override
 public boolean handle(final Product product, final Writer writer)
 throws IOException 

 if (product.sharesIdWith(this.updatedProduct)) 
 writer.write(this.updatedProduct.asString());
 return true;
 

 writer.write(product.asString());
 return false;
 

Product

final class Product 

 private static final int COLUMN_SIZES = 8, 30, 8, 4 ;
 private final String id;
 private final String product;

 public Product(final String product) 
 this.product = product;
 this.id = this.product.substring(0, COLUMN_SIZES[0] - 1).trim();
 

 public Product(final String product) 
 final StringBuilder stringBuilder = new StringBuilder();
 for (int i = 0; i < product.length; i++) 
 stringBuilder.append(String.format("%-" + COLUMN_SIZES[i] + "s", product[i]));
 

 this.product = stringBuilder.toString();
 this.id = this.product.substring(0, COLUMN_SIZES[0] - 1).trim();
 

 public boolean hasId(final String id) 
 return this.id.equals(id);
 

 public boolean sharesIdWith(final Product product) 
 return this.id.equals(product.id);
 

 public String asString() 
 return this.product;
 

ProductHandler

import java.io.IOException;
import java.io.Writer;

interface ProductHandler 

 boolean handle(final Product product, final Writer writer)
 throws IOException;

file format definition

You have some definition of the file format, like

It contains tabular data where lines are formatted as:

id productName price quantity

and

Last important thing: every element on textbase has own weight. It's int argsLength. If the element size less that its weight, we fill empty space with whitespaces.

Why do you have to explain that?
Why is this not obvious from your code?

Without your hints on the structure of the format, I'd be lost.

The information about the format is not represented in your code in any high level way.

It's a good starting point to be able to slice the string the right way to get the desired information out of it. But the following line

 if (args[1].trim().equals(result.substring(0, argsMaxLength[1]).trim())) 

is not very readable or easy to maintain.

names

There are too many nondescriptive abbreviated variable/parameter names that are too similar. Look at this parameter list for example and tell me what it means after two months of time passed:

(String arg,String args, int argsMaxLength,String filename)

levels of abstraction

What's strange about the code is that it incorporates very abstract concepts like patterns (in the form of the factory pattern, FileCreatorFactory) while operating mostly on "low" level types like strings and arrays. There's no middle ground.

While reading your question, the first thing I would expect to see is a class Product, but it doesn't exist.
I'd start with that and take a more object oriented aproach. This can be the place to define how to convert from a string and back into one. When two Product objects are considered to be equal. etc.

This let's you think about the problem not "reading line by line" but "reading product by product". In a top-down manner.