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

1. I guess you can get rid of getContentWithoutUnicode and make a method where the content will be cleared of Unicode and call it from getContent if necessary
2. for (int i = 0; i < content.length(); i += 1) {
   o.write(content.charAt(i));
   } - you don't need a for-loop here. you can replace with only o.write(content);
   3)"InputStream i" could be announced once in the beginning.

From up-to-down:

• Parser with saveContent behaviour ??!!!
• good Parser have not to look for real data source, as new FileInputStream(file);
  one function parameter "InputStream" for getContent* - it's good.
  in this case: clean/close resources - it is not the job/preoccupation for this Parser
• does not clean/close Stream resource :need three times try-finally
• does not synchronized access to private "file" for three operations
• o.write(content.getBytes()) - replace the loop statement
• where is try-catch for FileNotFoundException for streaming operation?
• StringBuilder in the place of "String output" for use of getContent* loop operations

Problems observed with the code

1. The constructor should be with a File parameter instead of the default constructor

---

public Parser(File file){

if(file!=null){
   this.file = file
}
else{
   // Throw an exception
}

}

---

This is to ensure that the file instance variable is set before performing the file operations (getContent, getContentWithoutUnicode, saveContent)

In the current Parser.java file, if we create a parser object and then directly call the getContent method on it, it will throw
an NullPointerException.

2. The InputStream is used for reading the file content. This means that every call to InputStream.read() will perform a syscall (expensive), whereas most calls to BufferedInputStream.read() will return data from the buffer. In short, if you are doing "small" reads, putting a BufferedInputStream into your stream stack will improve performance.

3. There is a performance hit by synchronization on the Parser object

---

public synchronized void setFile(File f) {
file = f;
}

public synchronized File getFile() {
return file;
}

---

There is no need of these if the constructor mentioned in Step 1) is present. Also there is no need to synchronize these methods
Moreover locking the entire Parser object is not good and it is a performance hit.

4. This logic is not thread safe.

I see that the getContent and saveContent are not synchronized. This is dangerous and might lead to improper results.

This logic should be synchronized by getting a lock on an object. Here I also mean that the lock should not be on the Parser object as it will
lock any other operations which can be performed parallely.

---

private final Object lock = new Object();

public String getContent() throws IOException {
synchronized(lock){
InputStream i = new FileInputStream(file);
String output = "";
int data;
while ((data = i.read()) > 0) {
output += (char) data;
}
return output;
}
}

public String getContentWithoutUnicode() throws IOException {
synchronized(lock){
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 {
synchronized(lock){
OutputStream o = new FileOutputStream(file);
for (int i = 0; i < content.length(); i += 1) {
o.write(content.charAt(i));
}
}
}

---

5. Streams are not closed. This is dangerous and can lead to improper results.

6. StringBuffer for performance improvement as Strings are immutable and hence each time a new String object is created and added to the string pool. StringBuffer is non-synchronized compared to StringBuilder but it is fine as we don't need synchronization at that level.

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