Skip to main content
deleted 482 characters in body
Source Link
infinitezero
  • 1.1k
  • 5
  • 14

Disclaimer: I'm not a Java programmer (last time was ~14 years ago)

Buffer lines
FileIO is slow. While you mention you want to reduce memory, you also mention you want to improve performance ™. In order to speed things up and assumming one line doesn't not occupy several hundred megabytes of memory, you should consider reading multiple lines at once to reduce the bottleneck of accessing the file. I suggest addingIt's a parametergood choice to the constructor which specifies how many lines (or bytes) should be read so the user can fine-tune it. I don't know too much about the specifics of BufferedReader though, maybe it already takes care apropriatelyuse a Buffered Reader here as FileIO is generally a bottleneck.

Don't terminate on non-critical errors
I would never expect that a function that's called hasNext has the ability to nuke the running application (System.exit(-1)). That's an absolute no-go.
Definitely prefer exceptions over program termination.

Don't loop if you don't want a loop It looks like this is just copy/pasted because why is there a while loop here? Either you terminate the program or you return from the function.

while ((messageRowJson = bufferedReader.readLine()) != null) {
  next = parserService.getMessageFromRowJson(messageRowJson);
  if (next == null) {
    logger.log(Level.SEVERE, "Unable to parse messaeg row");
    System.exit(-1);
  } else {
    return true;
  }
}

If you decide to implement the buffer system, use a for loop instead (for reading lines) or a while loop (for reading byte sizes).

User expectation
Typically the hasNext implementation should be very light. In your case it contains FileIO. Initially I found this weird but decided not to raise a comment about it, since you can't know if there is a nextValue unless when you read the file. Except that you can. The only way that there is no further data is when the file reader has encountered EOF. Otherwise there is another line to be read. If the line read is invalid, you can still throw an exception. That is, move the file reading code into the next function and just check if another read will succeed. Potentially couple this with the buffering (check if there are still lines on the buffer first).

Typos
"Unable to parse messaeg row" contains a typo.

Disclaimer: I'm not a Java programmer (last time was ~14 years ago)

Buffer lines
FileIO is slow. While you mention you want to reduce memory, you also mention you want to improve performance ™. In order to speed things up and assumming one line doesn't not occupy several hundred megabytes of memory, you should consider reading multiple lines at once to reduce the bottleneck of accessing the file. I suggest adding a parameter to the constructor which specifies how many lines (or bytes) should be read so the user can fine-tune it. I don't know too much about the specifics of BufferedReader though, maybe it already takes care apropriately.

Don't terminate on non-critical errors
I would never expect that a function that's called hasNext has the ability to nuke the running application (System.exit(-1)). That's an absolute no-go.
Definitely prefer exceptions over program termination.

Don't loop if you don't want a loop It looks like this is just copy/pasted because why is there a while loop here? Either you terminate the program or you return from the function.

while ((messageRowJson = bufferedReader.readLine()) != null) {
  next = parserService.getMessageFromRowJson(messageRowJson);
  if (next == null) {
    logger.log(Level.SEVERE, "Unable to parse messaeg row");
    System.exit(-1);
  } else {
    return true;
  }
}

If you decide to implement the buffer system, use a for loop instead (for reading lines) or a while loop (for reading byte sizes).

User expectation
Typically the hasNext implementation should be very light. In your case it contains FileIO. Initially I found this weird but decided not to raise a comment about it, since you can't know if there is a nextValue unless when you read the file. Except that you can. The only way that there is no further data is when the file reader has encountered EOF. Otherwise there is another line to be read. If the line read is invalid, you can still throw an exception. That is, move the file reading code into the next function and just check if another read will succeed. Potentially couple this with the buffering (check if there are still lines on the buffer first).

Typos
"Unable to parse messaeg row" contains a typo.

Disclaimer: I'm not a Java programmer (last time was ~14 years ago)

Buffer lines
It's a good choice to use a Buffered Reader here as FileIO is generally a bottleneck.

Don't terminate on non-critical errors
I would never expect that a function that's called hasNext has the ability to nuke the running application (System.exit(-1)). That's an absolute no-go.
Definitely prefer exceptions over program termination.

Don't loop if you don't want a loop It looks like this is just copy/pasted because why is there a while loop here? Either you terminate the program or you return from the function.

while ((messageRowJson = bufferedReader.readLine()) != null) {
  next = parserService.getMessageFromRowJson(messageRowJson);
  if (next == null) {
    logger.log(Level.SEVERE, "Unable to parse messaeg row");
    System.exit(-1);
  } else {
    return true;
  }
}

If you decide to implement the buffer system, use a for loop instead (for reading lines) or a while loop (for reading byte sizes).

User expectation
Typically the hasNext implementation should be very light. In your case it contains FileIO. Initially I found this weird but decided not to raise a comment about it, since you can't know if there is a nextValue unless when you read the file. Except that you can. The only way that there is no further data is when the file reader has encountered EOF. Otherwise there is another line to be read. If the line read is invalid, you can still throw an exception. That is, move the file reading code into the next function and just check if another read will succeed. Potentially couple this with the buffering (check if there are still lines on the buffer first).

Typos
"Unable to parse messaeg row" contains a typo.

added 688 characters in body
Source Link
infinitezero
  • 1.1k
  • 5
  • 14

Disclaimer: I'm not a Java programmer (last time was ~14 years ago)

Buffer lines
FileIO is slow. While you mention you want to reduce memory, you also mention you want to improve performance ™. In order to speed things up and assumming one line doesn't not occupy several hundred megabytes of memory, you should consider reading multiple lines at once to reduce the bottleneck of accessing the file. I suggest adding a parameter to the constructor which specifies how many lines (or bytes) should be read so the user can fine-tune it. I don't know too much about the specifics of BufferedReader though, maybe it already takes care apropriately.

Don't terminate on non-critical errors
I would never expect that a function that's called hasNext has the ability to nuke the running application (System.exit(-1)). That's an absolute no-go.
Definitely prefer exceptions over program termination.

Don't loop if you don't want a loop It looks like this is just copy/pasted because why is there a while loop here? Either you terminate the program or you return from the function.

while ((messageRowJson = bufferedReader.readLine()) != null) {
  next = parserService.getMessageFromRowJson(messageRowJson);
  if (next == null) {
    logger.log(Level.SEVERE, "Unable to parse messaeg row");
    System.exit(-1);
  } else {
    return true;
  }
}

If you decide to implement the buffer system, use a for loop instead (for reading lines) or a while loop (for reading byte sizes).

User expectation
Typically the hasNext implementation should be very light. In your case it contains FileIO. Initially I found this weird but decided not to raise a comment about it, since you can't know if there is a nextValue unless when you read the file. Except that you can. The only way that there is no further data is when the file reader has encountered EOF. Otherwise there is another line to be read. If the line read is invalid, you can still throw an exception. That is, move the file reading code into the next function and just check if another read will succeed. Potentially couple this with the buffering (check if there are still lines on the buffer first).

Typos
"Unable to parse messaeg row" contains a typo.

Disclaimer: I'm not a Java programmer (last time was ~14 years ago)

Buffer lines
FileIO is slow. While you mention you want to reduce memory, you also mention you want to improve performance ™. In order to speed things up and assumming one line doesn't not occupy several hundred megabytes of memory, you should consider reading multiple lines at once to reduce the bottleneck of accessing the file. I suggest adding a parameter to the constructor which specifies how many lines (or bytes) should be read so the user can fine-tune it. I don't know too much about the specifics of BufferedReader though, maybe it already takes care apropriately.

Don't terminate on non-critical errors
I would never expect that a function that's called hasNext has the ability to nuke the running application (System.exit(-1)). That's an absolute no-go.
Definitely prefer exceptions over program termination.

Don't loop if you don't want a loop It looks like this is just copy/pasted because why is there a while loop here? Either you terminate the program or you return from the function.

while ((messageRowJson = bufferedReader.readLine()) != null) {
  next = parserService.getMessageFromRowJson(messageRowJson);
  if (next == null) {
    logger.log(Level.SEVERE, "Unable to parse messaeg row");
    System.exit(-1);
  } else {
    return true;
  }
}

If you decide to implement the buffer system, use a for loop instead (for reading lines) or a while loop (for reading byte sizes).

Typos
"Unable to parse messaeg row" contains a typo.

Disclaimer: I'm not a Java programmer (last time was ~14 years ago)

Buffer lines
FileIO is slow. While you mention you want to reduce memory, you also mention you want to improve performance ™. In order to speed things up and assumming one line doesn't not occupy several hundred megabytes of memory, you should consider reading multiple lines at once to reduce the bottleneck of accessing the file. I suggest adding a parameter to the constructor which specifies how many lines (or bytes) should be read so the user can fine-tune it. I don't know too much about the specifics of BufferedReader though, maybe it already takes care apropriately.

Don't terminate on non-critical errors
I would never expect that a function that's called hasNext has the ability to nuke the running application (System.exit(-1)). That's an absolute no-go.
Definitely prefer exceptions over program termination.

Don't loop if you don't want a loop It looks like this is just copy/pasted because why is there a while loop here? Either you terminate the program or you return from the function.

while ((messageRowJson = bufferedReader.readLine()) != null) {
  next = parserService.getMessageFromRowJson(messageRowJson);
  if (next == null) {
    logger.log(Level.SEVERE, "Unable to parse messaeg row");
    System.exit(-1);
  } else {
    return true;
  }
}

If you decide to implement the buffer system, use a for loop instead (for reading lines) or a while loop (for reading byte sizes).

User expectation
Typically the hasNext implementation should be very light. In your case it contains FileIO. Initially I found this weird but decided not to raise a comment about it, since you can't know if there is a nextValue unless when you read the file. Except that you can. The only way that there is no further data is when the file reader has encountered EOF. Otherwise there is another line to be read. If the line read is invalid, you can still throw an exception. That is, move the file reading code into the next function and just check if another read will succeed. Potentially couple this with the buffering (check if there are still lines on the buffer first).

Typos
"Unable to parse messaeg row" contains a typo.

Source Link
infinitezero
  • 1.1k
  • 5
  • 14

Disclaimer: I'm not a Java programmer (last time was ~14 years ago)

Buffer lines
FileIO is slow. While you mention you want to reduce memory, you also mention you want to improve performance ™. In order to speed things up and assumming one line doesn't not occupy several hundred megabytes of memory, you should consider reading multiple lines at once to reduce the bottleneck of accessing the file. I suggest adding a parameter to the constructor which specifies how many lines (or bytes) should be read so the user can fine-tune it. I don't know too much about the specifics of BufferedReader though, maybe it already takes care apropriately.

Don't terminate on non-critical errors
I would never expect that a function that's called hasNext has the ability to nuke the running application (System.exit(-1)). That's an absolute no-go.
Definitely prefer exceptions over program termination.

Don't loop if you don't want a loop It looks like this is just copy/pasted because why is there a while loop here? Either you terminate the program or you return from the function.

while ((messageRowJson = bufferedReader.readLine()) != null) {
  next = parserService.getMessageFromRowJson(messageRowJson);
  if (next == null) {
    logger.log(Level.SEVERE, "Unable to parse messaeg row");
    System.exit(-1);
  } else {
    return true;
  }
}

If you decide to implement the buffer system, use a for loop instead (for reading lines) or a while loop (for reading byte sizes).

Typos
"Unable to parse messaeg row" contains a typo.