Java method to load random paragraph from text file

Question

Looking for some critique for this code. but ultimately I would like to be directed to resources/books that address my code writing weaknesses, in terms of style structure an convention :

The task is to create code that would read a file that has several questions and answers, each question starts with the word "Question", and out put the question and a number of lines from the answer that is not greater than 40 lines.

my code method below is meant to:

1. read all lines into an List.

2. select a random start line in the list.

3. check id the line starting with the string "Question".

4. check if the remaining list elements are more than 40 (I've appended 40 empty lines to the end of the file, to make sure to include the last lines of the file).

5. create a string buffer to include maximum the 39 lines which are present after the start line, the one that contains "Question" , if the the word "Question" appears before the 40th line, then we we end the text captured at the preceding line (creating a text block).

6. return that (text block).

---

public static String readJavaInterview() throws IOException{
 List<String> list = Files.readAllLines(new File("src/lists/JavaCodingInterview.txt").toPath(), Charset.defaultCharset() );
 List<String> subList = new ArrayList<>();
 int start=0 , end= 0;
 Random rand = new Random();
 int n = rand.nextInt(list.size());
 StringBuffer textBlock= new StringBuffer();
 Pattern p = Pattern.compile("Question");
 Matcher m;
 textBlock.append("Some Java Interview Qs :\n");
 if (n + 40 < list.size()){
 subList = list.subList(n, n + 40);
 for (int i = 0 ; i < subList.size() - 1 ; i++){
 m = p.matcher(subList.get(i).toString());
 boolean b = m.find();
 if (b)
 {
 start= i + 1;
 textBlock.append(subList.get(i) + "\n");
 break;
 }else{
 continue;
 }
 }
 for(int j = start ; j < start + 39; j++){
 m = p.matcher(subList.get(j).toString());
 boolean b = m.find();
 if (!b){
 textBlock.append(subList.get(j) + "\n");
 }else{
 break;
 }
 }
 }else{
 return "";
 }
 return textBlock.toString();
 }

Would this be how you would do it?

• Imus: Was this for an interview? (Or other assignment?) If so, what was the actual question? What you explained is how you solved the problem, but without the problem itself it's a bit difficult to provide good feedback.

Answer 1

Some assumptions based on the limited explanation of the question you posted:

• A new question starts with the word "Question" at the beginning of the line
• The first line in the file starts with "Question". (not really required but otherwise we have to add a check).
• A question can have any number of lines. It ends either at the end of the file, or right before a line that starts with the word "Question" (at which point the next question starts).
• If a question contains more than 40 lines, we are only interested in the first 40. The rest of the question is ignored.
• You want to randomly select a single question from the file as a result of the program

---

What I would do is split this up into 2 main parts.

1) read the file and split it up into separate "Questions"
2) select a random question

To me this looks a lot like I want to have List<Question> questions somewhere. Where Question is a new class to hold a single question.

Once we have that step 2 becomes almost trivial. (You've shown in your code you know how to do this).

Let's split step 1 up into specific parts as well.

1) read a file line by line
2) for each line:
2.a) Check if it is the start of a new Question.
2.b) add it to the current question if it's not 40 lines yet, otherwise ignore

Let's skip step 1 for a moment and look at how to process each line. (We'll come back to reading the file later).

I suggest creating a sort of "factory" class. This class will store that List<Question> that I wanted, and to make it easy for ourselves it will also store the Question currentQuestion. That way we can do step 2.b by just adding the line to the currentQuestion.

But first let's look at the Question class:

class Question {
 private List<String> lines = new ArrayList<>();
 public void addLine(String line){
 if(lines.size()>=40){
 return;
 }
 lines.add(line);
 }
 public List<String> getLines(){
 //we might want to make a new list here to prevent other classes from modifying it.
 return lines;
 }
}

Notice how I also made sure to only keep track of the first 40 lines. By doing this here, we can safely keep calling the add method while parsing all the lines without worying about it anymore.

Now for that factory class. This turns out to be rather easy as well:

public class InterviewQuestionFactory {
 private List<Question> questions = new ArrayList<>();
 private Question currentQuestion = null;
 public InterviewQuestionFactory(){
 //TODO read lines from file
 // for each ... {
 String line = "";
 process(line);
 //}
 }
 private void process(String line){
 if (line.startsWith("Question")) {
 currentQuestion = new Question();
 }
 currentQuestion.addLine(line);
 }
}

That was everything for step 2. All that's left is step 1 and we're done.

For step 1 here it's best to take a look at this post. Your current solution to read all lines at once might not work for large files. Since java 8 you can easily do something like this instead:

try (Stream<String> lines = Files.lines(Paths.get(filename), Charset.defaultCharset())) {
 lines.forEachOrdered(line -> process(line));
}

And that's how I would have solved the problem. I left some parts up for you to actually complete but it should be almost trivial at this point.

---

To complete this review let's point out some things about your current piece of art:

doesn't work as intended

If your random start happens to be the line just after the start of a really long question you don't return anything.

unnecessary whitespace

this:

for (int i = 0 ; i < subList.size() - 1 ; i++){
 m = p.matcher(subList.get(i).toString());
 boolean b = m.find();
 if (b)
 {
 start= i + 1;
 textBlock.append(subList.get(i) + "\n");
 break;
 }else{

should look like this instead:

for (int i = 0; i < subList.size() - 1; i++) {
 m = p.matcher(subList.get(i).toString());
 boolean b = m.find();
 if (b) {
 start = i + 1;
 textBlock.append(subList.get(i) + "\n");
 break;
 } else {

variable names

In that last bit of code, what is the meaning of the variables m p b and start? This reads like:

You match the string from the sublist to something. If it matches you update start to the next index in the sublist and append the current String to the textblock.

Most of it can be solved by using sublist(i).startsWith("Question") instead. If we then also inline the b to get rid of it entirely and rename start to nextLineIndex it turns into this:

for (int i = 0; i < subList.size() - 1; i++) {
 if (sublist.get(i).startsWith("Question") {
 nextLineIndex = i + 1;
 textBlock.append(subList.get(i) + "\n");
 break;
 } else {

actually the start name was fine if you had set it to start = i instead. And start the following for loop at for(int j = start + 1; ... instead. At least then we would know you start looping at the second line of the question.

string.toString()

sublist.get(i) is already a String. No need to call toString() on it.

• Tlink: Excellent, will read more about Java 8 streams and Factory pattern. Other than bug that you have pointed out (nothing displayed when there's a really long question), I've also noticed an out of array boundary error. I have some resources on factory pattern, but if you have recommended resources with real life examples, please let me know. my up vote will show up once I'm over 15 reputations.
• Imus: The "factory" idea used here probably doesn't completely follow the GoF factory pattern though. If you're interested in learning how to use the more common patterns I suggest reading "Head first design patters" book. Quick note on streams though: they're definitely worth studying and are awesome when you want to do more complex filtering etc. (especially parallellised) but they do have a performance cost. Sometimes a clever for loop works far better.
• Tlink: Great, I've ordered 2 books, the one you suggested and another on Lambda and Streams. I hope I'm not wearing you out with questions, but from time to time I keep getting this error Exception in thread "main" java.lang.IndexOutOfBoundsException: Index: 40, Size: 40 at java.util.ArrayList$SubList.rangeCheck(ArrayList.java:1217) at java.util.ArrayList$SubList.get(ArrayList.java:1034) at RandomatorWeekly.readSimpleJava(MyClassName.java:506)
• Tlink: where line 506 is m = p.matcher(subList.get(j).toString()); in my original code, the line is inside the last for-loop in my code. tried to change the 39 to 40 then 38 , but still getting the same error from time to time. Any suggestions?
• Imus: It's probably in the error case I mentioned. Your first while loop doesn't find a line with Question (or finds it really late). Then your start = 39 when you first get into the second for loop. This one tries to run untill 78 but already gives an error at j=40. This because the sublist is only 40 elements long (index starts at 0, so the last possible index is 39). That's why you get an index out of bounds.