[CS2113-T13-3] LeBook #43
Conversation
TVageesan
left a comment
TVageesan
left a comment
There was a problem hiding this comment.
Overall looks good, don't see any major design issues here. Main way you could (optionally) improve is by utilizing custom exceptions to streamline your error handling.
| // Assert that we're returning a non-null list | ||
| assert books != null : "Book list should never be null"; |
There was a problem hiding this comment.
Comments like these violate the principle of 'don't repeat the obvious'. The assert is already quite clear on what it does just by reading the code: if the books is null, flag a assert that "Book List should never be null". The comment here actually harms the readability as the reader/developer reads twice the amount of words to get the same amount of understanding of what this line does.
| public String toSaveAsString(Book book) { | ||
| assert (filePath != null) && !filePath.trim().isEmpty() : "File path cannot be null or empty"; | ||
| return book.toFileFormat(); | ||
| } |
There was a problem hiding this comment.
Does it make sense to verify that filePath is valid here, inside toSaveAsString? You could either 1) check once at initilaization 2) assert that filePath is valid only in functions that are relevant/require the filepath to be valid. Secondly, if this function is just a wrapper around book.toFileFormat() do we need the function in the first place?
| return bookList; | ||
| } | ||
|
|
||
| Scanner s = new Scanner(f); |
There was a problem hiding this comment.
avoid single letter variables outside of temporary variables (i.e. i,j,k in loops). I promise the readability gain is worth typing the extra seven characters.
| directory.mkdirs(); | ||
| } | ||
|
|
||
| try (BufferedWriter writer = new BufferedWriter(new FileWriter(filePath))) { // Overwrites file |
| public void printError(String message) { | ||
| printSeparator(); | ||
| System.out.println("[ERROR] " + message); | ||
| printSeparator(); | ||
| } | ||
|
|
||
| public void printWithSeparator(String message) { | ||
| printSeparator(); | ||
| System.out.println(message); | ||
| printSeparator(); |
There was a problem hiding this comment.
If you already have a printWithSeperator, why not utilize that to implement your other prints? (i.e. printError(String message) { printWithSeperator("[ERROR]" + message) } )
| String result = bookManager.deleteBook("2"); // Index out of bounds | ||
|
|
||
| assertEquals(1, bookManager.getBooks().size()); | ||
| assertEquals("There is no such book in the library!", result); |
There was a problem hiding this comment.
not essential/necessary, but considering using actual exceptions for situations like this to make your testing less brittle. When you output directly a error message (and check for it) it means the test is very dependent on the message you write. If you change the text a bit later on, all the tests related to it will fail even though it's still functioning correctly.
What you can do to make this less brittle is use custom exceptions. IF you use a InvalidBookException for example, jest can just check if the correct type of exception is thrown (which wont change) instead of checking if the error message itself matches (which changes much more often)
| public String deleteBook(String index) { | ||
| assert index != null : "Book Index cannot be null"; | ||
|
|
||
| try { | ||
| int bookIndex = Integer.parseInt(index) - 1; |
There was a problem hiding this comment.
With SLAP principle, we want to focus responsibilities specifically to manging books to the Book Manager class. It's a bit strange then that deleteBook receives a string index and then processes it into a integer index. This should be the Parser level responsibility to process string data into the data the internal classes want to receive.
Edited searchbyid and fixed bug for UI
Added in ppp and edited ug
added junit tests for some command files
Add more parser tests and implement minor change in parser
added listoverdue and undo function to UG
Update help command to include listing of users
minor bug fix for undo function
improve the help command. Also improved formatting when adding a book.
added Ethan ppp
Fix test bugs.
Add Jermaine PPP
Minor fix to commands
Update deanson-choo.md
remove the pagebreak
Update README
Update README
Fix formatting
Delete old README.md
update DG
update and remove extraneous
fixes bug on listing shelf
bug fixing in shelf list numbering
bug fix in list overdue users
14 participants
Book Management System that eases the workload of librarians. It is optimized for librarians so that tasks can be done faster.