Java, delete / update the line from the file by id

Question

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 int[]{0, 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();
    }
}

Answer 1

• 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) {
                final Product product = new Product(line);
                found |= 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;
}

Answer 2

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.