Refactor#1
Open
byronclark wants to merge 3 commits into
Open
Conversation
Allow the user to pass a directory on the command line. If not passed, use the provided src/main/resources. Now that we accept input, we can no longer assume that the files can be opened from resources. Stop passing around an InputStream that will just be converted to a scala.io.Source and use an absolute path instead. Just for kicks, get rid of the additional list conversion done when getting the file list.
Creating an entire ActorSystem for each file is overkill. Use a single actor system and watch for all the files requested to finish or timeout before shutting down.
Make it clearly obvious which messages belong to each actor.
| val system = ActorSystem() | ||
| implicit val ec = system.dispatcher | ||
| val results = directory.listFiles.map(_.getAbsolutePath).map(processFile(system, _)) | ||
| Future.sequence(results.toIterable).onComplete { _ => |
Owner
|
@byronclark Thank you for doing this! This is my first time writing and sharing something while I am learning and it feels good to interact with the community :) I left a few comments in the your pull request. I learned a few things and I will integrate some of it. Yes, I did a lot of refactoring to have only one ActorSystem and not waited for the Future to return to shutdown. I used a separate Actor to handle it. I don't fully understand the concept of
Thank you!!!! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for putting together a good example project. I've been learning Akka myself and saw some things I wanted to try here.
It looks like you've already gone a slightly different way on your refactor, but I still wanted to push some of these changes. I'm not expecting this to be merged, just providing some thoughts.
It feels a little strained to use Akka for running a set of operations and then waiting for it to shut down. I've followed your original pattern, but used a single actor system and the coalesced all the Futures to determine when to shut down. I also created a
propsmethod for each companion class to avoid possibly closing on state.