quiz.java

quiz.java

/**
 * Please review the class below and suggest improvements. How would
 * you refactor this class if it would be in a real-life project?
 * There are many problems here, both high-level design mistakes,
 * and low-level implementation bugs. We're interested to see high-level
 * problems first, since they are most critical. The more mistakes
 * you can spot, the better programmer you are.
 */
 
/**
 * This class is thread safe.
 */
public class Parser {
  private File file;
  public synchronized void setFile(File f) {
    file = f;
  }
  public synchronized File getFile() {
    return file;
  }
  public String getContent() throws IOException {
    InputStream i = new FileInputStream(file);
    String output = "";
    int data;
    while ((data = i.read()) > 0) {
      output += (char) data;
    }
    return output;
  }
  public String getContentWithoutUnicode() throws IOException {
    InputStream i = new FileInputStream(file);
    String output = "";
    int data;
    while ((data = i.read()) > 0) {
      if (data < 0x80) {
        output += (char) data;
      }
    }
    return output;
  }
  public void saveContent(String content) throws IOException {
    OutputStream o = new FileOutputStream(file);
    for (int i = 0; i < content.length(); i += 1) {
      o.write(content.charAt(i));
    }
  }
}

High-level design issues:

1. Class functionality is covered by standard java.nio.file.Files class with more flexibility and many other features.

Read from the file:
String content = new String(Files.readAllBytes(Paths.get(fileName)));

Write to new file (note, this functionality should not be in parser entity):
Files.write(Paths.get(fileName), content.getBytes(), StandardOpenOption.CREATE);

If the Files interface is too complicated for client class a new helping util class with simplificated interface can be implemented to hide Files usage:

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.nio.file.StandardOpenOption;

public class FilesUtil {
  public static String readTextFile(String fileName) throws IOException {
    String content = new String(Files.readAllBytes(Paths.get(fileName)));
    return content;
  }

  public static void writeToTextFile(String fileName, String content) throws IOException {
    Files.write(Paths.get(fileName), content.getBytes(), StandardOpenOption.CREATE);
  } 
} 

2. The class design does not ensure proper synchronization:

Parser p = new Parser

1 thread: 
   p.setFile(File1);
   String res1 = p.getContent();     <  not ensured here that File1 will be parsed

2 thread: 
   p.setFile(File2);
   String res2 = p.getContent();   <  not ensured here that File2 will be parsed

I prefer to define such a member instead of current setFile(File) and subsequent call of getContent():
public synchronized String getContent(java.net.URI uri)

Also this is better to use singleton pattern to not create many instances for such a utility class.

4. The method getContent handles only ASCII symbols.

5. The method getContentWithoutUnicode() is written invalid if it is intended to exclude unicode symbols. InputStream.read() returns only 1 next byte from the stream, so for typical unicode symbol we will exclude only first byte of symbol value. So only unicode symbols from range 0x80 – 0xFF will be excluded correctly.

Also there are can be issues with characters encoding (UTF8 , UTF16)

5. Performance issues without buffered reading/writing

---

Low-level issues

• streams are never closed, there are should be proper try-catch- finally blocks
• use string concatenations in cycles instead of StringBuilder
• no imports
• no detailed javadocs
• no junits
• class name is too generic, better name – SimpleTextFileParser
• big files should not be parsed

Я бы использовал библиотеку apache commons, кода станет в разы меньше:

public String getContent() throws IOException, FileNotFoundException {
        if (file.exists()) {
            List<String> lines = FileUtils.readLines(file, "UTF-8");
            return lines.toString();
        } else {
            throw new FileNotFoundException("File not found...");
        }
    }

/**
 * Made this class fully public for next extend cases
 * For now it's unclear is this need to split FileContent class to different ones
 *
 * F.e. ParsedFile could have two methods returning SimpleContent and UnicodeFreeContent 
 * And each *Content class could have unique logic in it
 * 
 * ParsedFile is thread safe because of immutability 
 */
class FileContent {
    private final String raw;

    public FileContent(String rawContent) {
        this.raw = rawContent;
    }

    public String text() {
        return this.raw;
    }
}

class ParsedFile {
    private final InputStream fileInputStream;

    public ParsedFile(InputStream fileInputStream) {
        this.fileInputStream = fileInputStream;
    }

    public FileContent content() {
        String content = "bytes converted to string";
        return new FileContent(content);
    }

    public FileContent contentUnicodeFree() {
        String content = "bytes converted to string without unicode";
        return new FileContent(content);
    }
}

class DefaultParsedFileUsage {
    public static void main(String[] args) {
            File file = new File("path");
            try(InputStream is = new FileInputStream(file)) {
                ParsedFile parsedFile = new ParsedFile(is);
                FileContent fileContent = parsedFile.content();
                String fileText = fileContent.text();
                // next code
            } catch (IOException ioException) {
                // logging exception
            }
    }
}

I prefer to redesign this peace of code just like that

import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.function.UnaryOperator;

interface Content {
    String asString() throws IOException;
    void replaceWith(final String s) throws IOException;
}

final class ContentOf implements Content {
    private final File file;

    public ContentOf(final File f) {
        this.file = f;
    }

    @Override
    public synchronized String asString() throws IOException {
        try (final var f = new FileInputStream(this.file)) {
            return new String(f.readAllBytes(), StandardCharsets.UTF_8);
        }
    }

    @Override
    public synchronized void replaceWith(final String s) throws IOException {
        try (final var f = new FileOutputStream(this.file)) {
            f.write(s.getBytes(StandardCharsets.UTF_8));
        }
    }
}

final class AsciiOnly implements Content {
    private final Content origin;
    private final UnaryOperator<String> filter;

    public AsciiOnly(final Content c) {
        this(
            c,
            s -> s
                .codePoints()
                .filter(el -> el < 0x80)
                .collect(
                    StringBuilder::new,
                    StringBuilder::appendCodePoint,
                    StringBuilder::append
                )
                .toString()
        );
    }

    public AsciiOnly(final Content c, final UnaryOperator<String> f) {
        this.origin = c;
        this.filter = f;
    }

    @Override
    public synchronized String asString() throws IOException {
        return this.filter.apply(this.origin.asString());
    }

    @Override
    public synchronized void replaceWith(final String s) throws IOException {
        this.origin.replaceWith(this.filter.apply(s));
    }
}