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
            }
    }
}