-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[W3][T12-2] #3
base: master
Are you sure you want to change the base?
[W3][T12-2] #3
Conversation
@@ -763,7 +763,7 @@ private static void createFileIfMissing(String filePath) { | |||
private static void savePersonsToFile(ArrayList<String[]> persons, String filePath) { | |||
final ArrayList<String> linesToWrite = encodePersonsToStrings(persons); | |||
try { | |||
Files.write(Paths.get(storageFilePath), linesToWrite); | |||
Files.write(Paths.get(filePath), linesToWrite); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job catching this 👍 another fix could be omitting filePath
parameter entirely from the method signature since it is not used in the method anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one 👍
Hi @sergiovieri, your pull request title is invalid. For PR sent to satisfy a learning outcome, the PR name should be in the format of For team PR, the PR name should be in the format of Please follow the above format strictly and edit your title for reprocessing. Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at nus-se-pr-bot and add a link to this PR. |
Method
savePersonsToFile
writes tostorageFilePath
instead of the passed parameterfilePath
, which could cause incorrect behaviour in future uses. @bqnguyen94