tP comments dashboard

Sorted based on the number of comments given to others' PRs, but also showing comments on own PRs and other comments given. Updated weekly.

[This page was last updated on Apr 08 2021]

Would it better to refactor all the key functions out of the 'main' method?

Would it be better to use a switch statement here? This can help accommodate future addition of commands.

I like that you researched this! (:

i think we can put this an invalid input, and print an invalid input message

can we refactor this switch statement out of parse because this technically isnt parsing? so it should not be in parse, probably a get command or smth

initialise the error message as a private final here

I am not sure if we require comments for these two methods, the details of the test is already in the method name. It might make the comments unnecessary.

Should we remove this as well to adhere to coding standards?

Method names should be an action. I suggest "getMessagePrintedToUser"

Class name should be a noun not a verb, can we name this Storage instead?

Remove extra empty line

Variable name should be 'file' not 'f'

Perhaps we can refactor this part into a separate function?

I think this should be split into 2 lines

Would it be better to use the Ui class to print the error message.

Additionally, this magic string should be a variable message.

Perhaps we can also change the message to "There was an error loading the data for FridgeFriend! ):"

All the methods in Fridge are static, there is no need to declare a fridge variable here

Method can return void, and access the Fridge statically.

not todo

i think we can use partial food names too, no need to be exact

i might be wrong

Can we leave this as a 1 liner list, instead of a long vertical list?

Should there not be space here?

I think you can just return FoodCategory.contains(description) here

Same here, just return FoodStorageLocation.contains(descrption)

i think they trim by default, best to just remove the extra spaces

there are extra spaces here on line 111 and 112

extra spaces here on line 75-77

extra spaces here on line 134-135

extra spaces here on line 157-158

extra space here

This line should have extra spacing

lines 75-77 has weird spacing

This line should have extra spacing

this line should have extra space

this line should have extra spaces

technically its a help command object

technically its an bye command object

i think can push this into a if-elseif instead of a nested if-else

shall we shift this to utilities?

inheritance should be block arrow

Can we define our app as such:

Food
Command
Exception
Utilities
FridgeFriend --> entry point

I think you can just briefly mention that it contains key driver classes. the other details will be in the utilities section

actually the user interacts with utilities not fridgefriend

Make the last sentence in third person

Make this sentence in third person

There is a*

Can we list the features by the command instead? Like without the description at the front, else our contents page would look kinda message

Can we just name this "Contents" cos technically theres no table HAHAHAA

Also can we put a ## heading for this?

can we put as much into 1 line? i dont think we need a new line for each sentence

Cann we put all this into 1 line

can we remove this bullet point? then we can shift everything below one tab to the left

maybe change this into a numbered list instead?

change all to singular verbs?

tbh i thought this should be things like "utilities component" or smth

can we name this "originalQuantity" instead

can we change this and the one below to like "sorry my kawan"

can we change this to "sorry my kawan"

remove

do you want to put brackets around 11?

can we change this HAHA we have a lot of "Sorry my friend"s

maybe like Sorry amigo or smth

line 365 should return HistoryCommand instead

Updated

Will refactor this method

It is a bit difficult to refactor this because most of the method calls are interdependent

so we can statically access this without having to pass a Fridge object through methods

will add javadoc comments

changes made

@Leeyp default notation requires a language to be defined for each code block, here declares that there is no language to highlight

oh yes, thanks, good catch

will edit based on prof's suggestions

modified

rejected, error message on your side will change

minquantity will be done by @SimJJ96

edited for for location

Wrong issue, ignore

Shouldn't we be using packages and separate classes for parsing commands already?

I think we can use packages, but not classes for parsing as of now. We will handle parsing in another commit.

LGTM

Good job. But why are there so many parallel PRs? Might start to merge one by one.

because breadth first approach (:

Does anyone know why there are no tests here?

LGTM. Also reminds me of the need to modify docs. 💯

no worries, i can do this for the UG @Vinci-Hu

I will edit it on the UG nonetheless

Will reopen as a suggestion to improve our UG

Steph Comments Summary

extract quantity stuff into another class
check if the exception throwing is consistent across all sequence diagrams, add a footnote/info that for simplicity we will express throwing of exceptions in comments of diagrams
combine all common details

??) future work —> modify to explain future ideas without mention of food cat

??) hide food category classes in sequence diagram

??) add warning in UG and DG for history data

Might want to call it MESSAGE_ADDED_LESSON instead, so all messages will have the same prefix.

These can be shifted to a Constants class in the common package when we merge.

I have this part in the storage code too. Maybe we can store this method in a class in the common package.

Might need to modify this to meet the Java Coding Standards.

Would be good if you can extract this into a class in the commands package.

Will the validity of arguements be checked here? Or will that be done in the parser?

Maybe can shift this into the Module class since they don't really deal with the UI. Then there would not be a need to pass in the task list.

Might want to change the name to printTaskInstructions.

Is there a check for validity of the token? (E.g. non-int input, negative value or out of bounds)

Perhaps gradedStatus might be a better name here to avoid confusion with isDone.

Actually is this needed, since the variable command is only used in the try block?

Think we can put all of these in a Constant class in common package since many of them are used in other parts as well.

This part can be made shorter by returning the command directly from each case. Same for the switch statements for inside the module below.

For the delete module command, are we deleting by index or by name?

What happens after the exception is thrown?

How will the exception be handled here?

Pattern can be declared as a string constant.

The storage part uses a few of them.

Perhaps these two methods can combine since the method isn't long, as "getFilteredTasks" can easily be confused with "getChosenTasks" below.

I think we are standardizing the printing of message to use ui.printMessage.

Actually will CommandException be thrown in this method?

Should we do it this way for the rest of the code?

For UnknownCommandException, will we have other messages?

I think we should follow the notes and not put an empty line here.

Might want to remove these blank lines too while you are at it.

Can put together with the lesson messages.

I notice that this is the only file modified push request. Is this message in use?

Can consider the use of .isEmpty() for arraylist.

Oh didn't realise that Crtl-D would cause it to crash. Maybe we can do something to fix this.

Is there a reason why the word "icon" is used here instead of something like "string"?

Might want to change 'e' to "event".

Is there a reason why this is separated from line 33? (Same for the other files)

Perhaps a more specific method name here would make it easier to understand? (Same for the edit command)

Random ';' here.

Is there a reason why this command needs to extend the delete command?

Can help me remove the duplicate on line 80? Thanks.

Hmm. But some will overlap, like the index of the fields and the date format. We should try to merge them.

Sure

Ok removed.

Ok replaced.

Ok. Will replace all of it.

Oops forgot about this

Can't add since method not in code yet,

Closed to use another pull request with strings shifted to Message class for all components.

Add author tags to fix tracking issue instead.

Closing because we will need to change diagrams if iit is mplement.

Number of parameters is valid for this.

add lesson >lesson type> ;; >day & time> ;; >link> ;; >teaching staff name> ;; >email>

Closing because might cause other bugs.

Fixed

Not merging to avoid introduction of new bugs.

catch this exception in ItemsParser

catch this exception at OrdersParser

no comment for now since need see the output together with item stock

No comments for Ui related stuff for now

Do together with item stock? If not for now since quite trivial can put inside the command as well

same comment as below, need check the print format when item stock is added

ItemsPromptItemInfoCommand instead?

Still have to do together with add item stock

update this later on

To be confirmed

Just do item.setItemStock() since you iterating the items in the order already

Change to int itemStockIndex

Can say that if they want to add on to an order that is not done, consider deleting the order first then create.

Also can say that if the order is done, and that the particular has a new order, delete also. Just add this for now, I'll decide whether to delete a done order after generating a receipt

undo this part, hyperlink is not working anymore

not sure if this part is required, might delete later

Change this to itemSales instead of itemsSales? Because price and stock are itemPrice and itemStock

think this part is irrelevant, but we can leave it for now

numberformat exception can be caught below in the processPriceInput

think there is no need to trim the customer name here because all inputs should be trimmed at the start alrd, can just go back to normal and just check for .equals(""). I think what needs to be trimmed is when asking for the item index and item quantity to be inputted into the order thats need to be trimmed

refer to comment above as well

refer to first comment as well

refer to first comment

no need for item description to be trimmed because inputs are alrd trimmed in the Ui askForUserInput() method.

same as first comment, user input is always automatically trimmed before processing the input

same as first and 2nd comment

Maybe consider adding also if you forget some of the available commands or need to check their format they can use the help function as well?

same as above, but i feel this one is more relevant towards just looking at items related features if the user forgets

think this is wrong because we can't add as 1 line

maybe do step by step instead of talking in story form, not too sure, we can discuss again

same as above comments regarding help and items

hi can fix grammar error here? When you are*?

consider adding a limit to the item names as I received a feedback saying the orders name is unlimited and causes a bug when storing the long name into save file. Limit that i set for customer name for orders add was 30 characters including spaces

Maybe just say whenever you would like to check the items and their stock in your inventory is enough already?

Reopening and updating because yiwen didnt complete the feature/ do the feature properly

Reopening because delete feature not deleting the correct index

Closing PR because of trivial changes

Approved for now since items add needs to take in stock of the item as well, so will review when both is added together

Because the price required was in integer, will change all to BigDecimal in v2.1

bug has been fixed or undergoing fix since the release of v2.0

Thank you for your report, i believe this has been fixed @zikunz @Cocokkkk ? If fixed @zikunz @Cocokkkk kindly close this comment.

Appreciate the report, @zikunz @Cocokkkk if its fixed do close the this issue thanks.

Appreciate your suggestion, we will work on updating the user guide to better inform users what not to type for item name

Appreciate the comment, we will change the argument to all show >customer_name> instead of >order_name>, thank you

Appreciate your comment, this typo has been fixed.

Hi, UG has since been update to reflect the details of the items stats feature as well, thank you.

Appreciate your comment, tagging the PR to fix this typo now.

Appreciate the feedback, we will consider implementing the ability to see the new stock. @Cocokkkk mind updating this, because you did the stock features, thank you.

Appreciate your comment on this, we would do a screenshot of the inputs and expected output of all features at the end for better clarity once all the Ui messages have been confirmed.

Appreciate your comment on this, @951553394 mind updating your part in the UG and and also exception messages to say that input field should be case sensitive? Thanks

Appreciate your report, @Cocokkkk do you mind checking your items add same item name part again, to make sure that this error does not occur again or we can decide during the group meeting if we are gonna keep this feature again.

Appreciate your finding on this, however, i do not believe that there is a bug regarding this as the stock of B should be 30 initially. When you added the orders for GZY and GEE, the 10 and 20 stock are then removed respectively from the inventory. Therefore, after you typed 'items list' after adding the orders, the inventory shows stock of 0 for B. In conclusion, both orders can still be fulfilled. Maybe we can improve by better indicating that the inventory count is excluding the stock assigned to the orders

Appreciate your report, this issues has since been fixed by @Cocokkkk , thank you.

I'll fix this since its under orders add feature

Appreciate your comment, we would consider limiting the length of the customer's name.

Appreciate your comment, we have since trimmed whitespace to prevent this problem.

Appreciate your comment, UG has since been updated to reflect all functionalities

Appreciate your comment, @Cocokkkk @zikunz has this been fixed since?

Appreciate your comment, will add that the invalid file input is line >number> of the save file

Appreciate your comment, but I am unable to replicate the bug that you have shown, perharps u may have entered an item before that with the name " ". We have since fixed that empty spaces will be trimmed from the user's input

We have since fix that user's input has been trimmed and empty items cannot be added

Appreciate your comment, receipt file can only be accessed when you exit the app. Maybe we can add that message after we say receipt generated to let the user know that he/she needs to exit the app first. Will also note that in the UG as well

Appreciate your comment, this issue has since been fixed.

Appreciate your comment on this. We will update our UG accordingly to show full screenshots.

@e00426142 @Cocokkkk do update the expected output accordingly

Appreciate your comment, we will delete the feature whereby you can delete by the item name. @951553394 do make the necessary updates.

Appreciate your comment, but typing items list is enough to see the full list of inventory. Any additional stuff being typed after items list is ignored. Maybe we can add that any additional inputs added at the back will be ignored. @e00426142 kindly make the necessary changes for items list and orders list parts in the UG

Appreciate your comment, we will delete the feature whereby you can delete an item by its name to resolve such confusions.

@951553394 do make the necessary changes.

Appreciate your comment, @zikunz maybe we can consider removing the clear functions to prevent such problems from existing as we are running out of time to fix this issue.

Appreciate your comment, but we stated in the UG that marking using orders done feature would automatically generate a receipt for that particular order and remove it from the system.

Appreciate your comment, we have since updated our UG to correctly reflect the items stats feature as well.

Appreciate your comment, the intention was to keep the orders list as minimal as possible, thus we removed orders that are marked as done and convert them into receipts and if the user wants to view the completed orders, they can just view the receipts instead. Maybe we can consider your suggestion for future versions

Appreciate your comment, but there are no restrictions as to what the user wants to input for the item name

Appreciate your comment, this issue has since been fixed.

Appreciate your comment, it would definitely make more sense, but for now due to speed, we decided not to add this function as it will greatly slow down the process if the order is big. Perhaps, we can state in the UG about this limitation instead.

Appreciate your comment, we will standardize by just making sure that the items delete feature would only delete by item index instead of by item name as well. @951553394 do make the necessary adjustments.

Would it be better to parse this as part of the "add" method, in case in the future it is not only the foodDescription (e.g. add expiry date, add category) as part of the input?

Would it be better to make this line consistent with the exceptions below? (Minor Code Quality)

Would it be better to explain what this "split" statement does in the comments above?

Code looks good and clean. Well done!

Only request is to explain a little bit slightly more about what this Pattern Compile does, since it is a "rare" and technical usage. As long as it is explained once, no need to explain again in future, should be good to go.

Would it be better to SLAP this method (i.e. make them all methods?)

Would it be better to further SLAP this method (such that they are all calling methods only?)

Can I understand what is the rationale for the change?

Would be nice if we could have sample outputs for these usages

Might be good to clarify what this split command does in a javadoc comment or similar.

Would be good to extract this out as a method for Code Quality purposes

What's lang-none?

Here too

Might want to clarify that is NOT on isExit then the loop continues, because it will actually quit on isExit, which might be misleading

Could it be possible that this is not an 'alt' box?

FridgeFriend runs the command regardless whether is an exception or not (unless invalid input, which can be mentioned, or perhaps simply put this as 'valid command' instead of 'successful execution')

So, I think if the 'alt's command is a successful execution, the logical opposite is an unsuccessful execution, in which case it affects the output but not the initial execution of the command.

"No more than one alternative partitions be executed in an alt frame. That is, it is acceptable for none of the alternative partitions to be executed but it is not acceptable for multiple partitions to be executed." (https://nus-cs2113-ay2021s2.github.io/website/schedule/week10/topics.html#w10-1-sequence-diagrams-basics)

Therefore I recommend either this condition be changed to "valid command" instead of "successful execution", or the alt box moved downward to capture execute() of Command rather than getCommand() of Parser.

Am I seeing things, or is the self-invocation here not spawning a new activation bar? Is that supposed to be correct?

I think this might be incorrect? Where did "totalQuantity" come from? I believe the Fridge needs to read the value from somewhere. Should there be a clarification?

Typo in "getMessagePrinterToUser" -> Should be getMessagePrintedToUser.

Would it be better if you refactored/SLAPPed the Search function more? Right now, "getMessagePrintedToUser" looks quite messy. It might be a good idea to extract out into several methods for SLAP 😃

I think this implementation requires more clarification.

My apologies but, as I have just added a new History Command, I think you might have to add that into the Diagram.

There is a typo in the actual file name. I think there might be problems generating the file image.

might need another 'alt' where the exception is thrown. I think can write a note, but need to draw the alt box at least.

Perhaps rephrase this into something that sounds nicer?

It's not very clear what this means to 'consist'.

Shifting the order of the foodToAdd would result in the history log data adding the post-edited quantity instead of the original quantity. Has this been addressed?

We might want to set the "MAX Value" not to integer max value, but rather a smaller value (perhaps 10k? 100k?) which would definitely prevent the case of Integer Overflows.

We are missing InvalidFoodStorageLocation Exception and the newest Minimum Quantity exception

Do be slightly careful that order matters in the History command, but seems like the checks pass, so looks good!

should be 'or' instead of 'and'

Displays a list of food items that are expiring within a week or have already expired.

Alright

Resolved!

resolved!

This is unfortunately not the current implementation

Good eye, resolved

It looks nice on my compiler, can double check?

Good catch, resolved

Sharp eye! Resolved.

Resolved

This one abit long so i think cannot tho unless i cut

alright

I condensed a bit but into 1 single line would violate code quality ("Line too long!")

Sure, implemented.

This is true, but would it be clearer for the reader to leave this expected output here, or to leave it blank for accuracy?

After some thought, I think it would be clearer to leave this as it is, because I need at least one path that tells us that the list is being output, which was not indicated anywhere else

I have thought of a solution that would solve both issues in my latest commit.

Resolved comments in second commit.

No longer relevant since all PRs include implicit Code Quality & Standard reviews.

Need to add a space before "in food object"

I help you edit in my commit

LGTM

Added Javadoc comments to some exceptions and fixed minor cosmetic issues

Also, refactored List method

Made changes, requesting approval from @Vinci-Hu and @SimJJ96 and @joohwan58

Fix broken multiquote code tags under Appendix: Instructions for Manual Testing

Dear @stephlewyh ,

Thank you for your comments!

Regarding point 11, I intentionally made the "Saving data" portion under "Manual testing" verbose, to insure against cheeky testers who attempt to break our code by testing all sorts of invalid corrupted test files, then reporting them as bugs. Since data files have the widest margin for mischief, I believed that having all possibilities spelled out would prevent them from reporting known issues as "bugs".

Is my assumption wrong? Is there a better way to write the section while keeping in mind adversarial readers/testers?

Thank you very much!

Marked as invalid due to user clearing the fridge before leaving the application, and misunderstanding the UG features.

Invalid bug but will update the UI status message to be even more clear.

Invalid because this is the intended output.

Invalid because this is the intended output. However, will update in UG to be even more clear.

Marked as invalid due to "FridgeFriend_v2.0.jar" being copyable in the java command as suggested. However, will modify in UG for further clarification.

Marked as invalid due the fact that it is technically correct.

Marked as invalid due to the fact that it is subjective opinion.

Marked as invalid due to the fact that project constraints require us to add all in one command. This is the intended function.

Marked as invalid due to subjective opinion.

Marked as invalid due to this being a suggestion rather than an actual bug. However, the suggestion will be noted.

Same concern for 7), even though the section is lengthy, it is clear and comprehensive, while not compromising on readability as some people who may be more interested in the Exceptions might find it useful (even though most might skim it). Clarity seems to be important here as there might be some weaker/confused readers who might find fault with sections that are not as clear/fleshed out.

Is there any way to improve the section while not compromising on detail/clarity for more nitpicky readers/might raise (invalid) "bugs" over content that is summarized instead of detailed?

Remove magic number.

Since you are using "\n" frequently, you can store it as a constant NEW_LINE

Need to implement setter and getter for selectedModule if we change it to private.

For the module code name, you can try changing them to constants so it will be easier to modify them for debugging

Don't forget to remove it later

Probably better to change the magic number to a constant.e.g NUMBER_OF_ARGUMENTS_IN_COMMAND.

Again magic number to a constant.

Preferable to change the magic number to a constant

Change 0(magic number) to a constant e.g NUMBER_OF_ARGUMENTS_FOR_LIST_COMMAND

Can change the magic string to constant EMPTY_MESSAGE

Same as the magic string above

Magic number to constant EMPTY_TASK_LIST

You can use a constant if you want

Method name: saveTextToFile() ;

Refactor magic number

Refactor magic string

Refactor magic number

Magic number present

Can refactor magic number

Nice thinking!

Was planning on doing so after I'm done implementing all commands. Thank you for the suggestion anw.

I'll have a look. Sure idm

This is not the final comment. I wrote this for Ivan. Will change it after he's done implementing the parser. No worries

Parser

I wanted to wait for the merging to discuss the names and refactor the code. I believe every lesson commands should be kept in the command package. Just didn't to create new classes in the skeleton rn.

Will change it the constant name

I'll have a look after your merge and make the appropriate changes

No, but the abstract class throws an exception. So I had to handle it

idm

Ivan told me to add this to the messages class. He's gonna use it in the parser class

sure

I chose this function name because it is overridden in EditCheatSheet as well. Is executeCommand ok?

fixed thanks

Edit uses most methods from delete. Edit contains an overridden method and an additional one to perform its function.

Also, I notice you are missing the remove function for lessons.

This branch contains only the methods needed to add a lesson. The methods for other commands will be added from their respective branches.

Thank you Wen Hao!!

Good job! LGTM! Nevertheless, I believe that we should wait for version 2.1 after we are done implementing all our features. Then we can do it for UI as well and it will be consistent.

You need to add weight as a param and initialize it as well.

please add Javadoc comment for all public methods

same as bodyweight class, please initialize totalCalories

Please complete the method

please complete the method

Please add a constructor to initialize all private variables

this is a class member, so should be static

enum members should be in UPPERCASE

the constructor should be:


public BodyWeight(RecordType type, LocalDate date, double weight) {

    super(type, date);

    this.weight = weight;

}

Please refer to the constructor code I gave for BodyWeight

I'll take care of this part after merging

please use the constructor to initialize the private attribute duration

Please add Javadoc comment for all public methods

You should use the constructor to initialize totalCalories

please complete the method

Please add Javadoc comments for all public methods

please complete the method

please complete the constructor

Javadoc comments do not follow the standard.

Parameters and returns must be specified, please add them accordingly

I'll merge first. will open a pr for bug fix later.

Please fix the CI error

The text ui test error can be ignored. The test is to be updated after this PR is merged.

This does not seem to be a bug. The body weight record can be added via add command, which is clearly specified in the UG.

@PingruiLi can you take a look at this issue plz. Thanks!

This is actually designed in this way. Once a goal is set, it will check the progress via existing records immediately. That's why the newly added daily goal will be seen as achieved since there are records for today already. Therefore, this is not considered as a bug.

@PingruiLi can you take a look at this issue plz? Thanks. Adding a lower bound for bodyweight when checking the input(e.g. at least 20kg) will do.

@PingruiLi

@PingruiLi can you take a look at this issue plz? Thanks. Adding a lower bound for bodyweight when checking the input(e.g. at least 20kg) will do.

@PingruiLi

This is a design choice. More workout categories can be added indeed, but this is not considered a bug, hence won't fix.

@PingruiLi

This limit is specified in UG clearly.

However, we can consider using float number for duration.

@PingruiLi

A good advice, but this is not considered a bug, hence closed.

@PingruiLi

As stated in the user guide, the application limits the number of food categories. Rice and bread can be seen as grain. Hence not considered a bug.

Already fixed in pr#129

Already fixed in #129

unable to reproduce the issue

Maybe these constants can be combined and their name can be more descriptive? e.g. MESSAGE_DELETE_PROMPT

I think the method names could be modified to be more self-explanatory e.g. starting with verbs

Line 30 too

Maybe this could be moved to a method in UI

Thank you for removing these duplicated methods!!

Can add a newline at the end for this file

If this is not used anywhere can just delete

I think lines 36 and 44 could be replaced with ui.readCommand()?

Scanner scanner = new Scanner(System.in);

...

String input = scanner.nextLine();

(some parts greyed out so I can only comment nearest to the lines)

Actually should this class be called ListModulesCommand? To standardise with ListLessonsCommand and ListTasksCommand and also to be clearer

I think we just missed the s when typing

Maybe this line and line 37 could also make use of ui.readCommand()? And generally replace any other places where a new Scanner is created?

Maybe can consider:

MESSAGE_TASK_CHECK_GRADED

MESSAGE_TASK_CHECK_GRADED_INFO

MESSAGE_SELECT_TASK_INFO

Could you add ability to print messages for NumberFormatException and ArrayIndexOutOfBounds?

Right, thanks!

Maybe can improve readability by ending each string at NEWLINE? But optional I guess because it's just test code

I think you can use FORMAT_INDEX_ITEM for line

I think printed messages will look less cluttered now, nice!

maybe can use the happy path method instead (return if lessonToEdit == null)

Maybe the method name can be redefined to show the negative outcome it is expecting? e.g. checkForInvalidCharacters()

Thank you, edited!

thank you, done!

thanks, done!

okay, done

Most output looks standardised now to me except for DeleteLessonCommand. I think can standardise it/edit/update UG

*Handle invalid non-integer input

Add ability to edit fields for task command (description, deadline, remarks)

Should not have split lines in markdown, 1 paragraph 1 line better according to se guide

Do let me know if you disagree or have other suggestions for the command names. I will update in the UG tomorrow and push to this same PR. Thanks guys!

Looks really good, nice! Only one thing, maybe can use "tsk" for the task commands? Not so much to save letters but more to standardise so that all the commands are easier to remember for the user.

Hi, maybe instead of return to the main command, you can redirect them back to the option select instead for user friendliness.

Labels: severity.VeryLow type.FeatureFlaw

original: lamzf1998/ped#1

We avoided using a while loop because we anticipated that if the user changed his/her mind halfway and no longer wanted to edit the lesson, they would not be stuck endlessly being prompted for a valid input. However, we have taken note of this and will mention it in the design considerations for the edit commands.

Consider using a more descriptive variable name other than 'c'

Consider refactoring this as a method so that u can run this recursively.

Consider adding javadoc to describe the use of this method.

Consider assigning a variable to constant values.

ie: private static final String FILTER = "filter";

Command command = parser.parse...

Because single character names usually used for counter / index.

Yup, perhaps init()?

Consider adding super(errorMessage).

To follow OOP, you should call on the exception's getMessage to print here.

showInvalidParameter(InvalidParameterException e) {system.out.println(e.getMessage()))}

FIND command is missing here.

Consider using fixed constraints, since there are only 2 possible valid cases (sort asc , sort desc). This will capture all valid cases.

To adhere to other codes, use this.value = null.

consider refactoring this to private methods to make code readable

Consider parsing the key to EmptyParameterException so that calling "System.out.println(e.getMessage())" is enough. Adheres to OOP

Completed. Can be tested with fetchByLocation and fetchByType.

Also implement JUnit test for this feature.

Make use of ApiRepository.fetchUnits(HashMap>QueryKey,String> map) to test.

Take note of exceptions, when user input does not make query key.

Handle the 'output' part of the app. Work with Angel so she can add to UG.

Pang How is doing the 'input' of the app, Jin Wei is doing the 'output' of the app.

Work with Angel, since this is the input part of the app. She will add details into UG.

This is not a useful feature, since user has to 'guess' the price at which the units are available.

Once Pang How is done with parser you may start on this feature

May need asynchronous programming

Need to add to Parser

sort, save and delete command need to trim values

All integer values as input must be checked.

TYPE should have harder restrictions.

Error message for TYPE, should include the info reported above.

Already transferred your changes to UG and TextUi. Thanks.

Duplicate.

Function name should be a verb, like printByeMessage

Same as above

If you show this message before remove action take place, this output would be weird. (There might be excpetions when you actually run remove method)

Good to have.

This comment is vague

Why is this class level?

Should explain what Logger class does here?

You changed this to /cat right?

I have wrote it in my pr for this.

Do you plan to add a pattern for every child class of QUANTITY? But current implementation can work.

I suggest You are running low on CATEGORY

Me and JJ are using a more pink color for the Food box.

Fixed

Yes thank you.

Will solve this in next pr!

LGTM

This is for the running low warning.

modify remove commands: change syntax into remove foodname quantity
update junit test and text-ui-test for remove commands

We are not supposed to list OTHER locations in the first place??

@Leeyp @kwokyto @joohwan58 @SimJJ96

We are not supposed to list OTHER locations in the first place??

@Leeyp @kwokyto @joohwan58 @SimJJ96

Because this is only a list by category. We might need to emphasize this CATEGORY in the display message

I don't think we should use str since it is an abbreviation, the argument could just be called string.

Alternatively, you could also rename the method to printMessage and the argument to message.

Should also add a check to ensure that a patient has been loaded, otherwise print an error message to inform the user that the user has not been found.

The null value there was a placeholder since the Ui class wasn't done yet, sorry if my tests were misleading!

Future JUnit tests shouldn't use null (I guess you could create a new Ui object and pass that as a parameter?)

You could change "not be empty" to "not be null" to reduce confusion between list being null or an empty string "".

Would it be better to extract this as a global constant to avoid using magic values?

The order of the arguments for assertEquals is (expected, actual), so it would be good to swap them around for consistency.

Would it be better to extract this out into the Constants class?

You could make this into a conditional, in case the file already exists.

I think everything from this part until fileWriter.close() doesn't need to be in the try catch block, could you double-check that?

No worries, I just checked and I think that writing to file could also throw an IOException, so it's fine to leave it in the try-catch.

Could expand enum out into enumerate

unexpecred -> unexpected

Should I number the records, e.g.

coughing
fever

etc?

Sounds good, will use this format.

I was considering that, but it seems that github flavoured markdown doesn't support resizing images from within the tag (see https://gist.github.com/uupaa/f77d2bcf4dc7a294d109). It seems like I have to use <img> tags to control the image size.

Done!

LGTM!

DG draft done

LGTM!

I think it should be fine to allow future dates, in case the user's computer time isn't set properly.

In general, for feature flaws, as long as we can justify our decisions, we can always reject the bug report in the actual PE.

Tag #87

Thanks for the report, bug has been fixed!

Closed for now, will come back to it after the bugfixes.

Fixed in #83

Change all tests to use assertDoesNotThrow instead of a try catch block.

Additional minor changes:

Remove hardcoded whitespace from error messages
Change BaseException.toString() to be compatible with the new error format
Add MethodDoesNotExist exception to be caught in the Parser.parse() method, for *Command classes that do not have the correct constructor
Small changes to error messages

Consider creating a default constructor

Remove final

To remove from object class

addModule creates new file after loading?

Consider creating a default constructor

Can probably use the UI class to read input.

Might need to standardize naming of constants.

Duplicate statements with the one after?

Would probably be good to generalize both the exceptions to something like a ParserException instead of re-using a CommandException since it is thrown by Parser class.

See above comment.

In my view, the isValidModuleCode method should be in the Module class as a class-level method since it could be a general method that other components might need to use. i.e. loading of module lists.

Gained approval from @8kdesign to re-assign.

Failing tests, will look into it.

Require further look into codebase.

Would it be better to consider the input command as a lower case to handle the upper case command of bye?

I think you have missed out on replacing the word todo with list

yes actually any name will do as long as it is not empty

Is extra space allow here?

Should the message here be quantity 5?

I think you can just return without a text since the listing is actually a void method where all their getters are actually passed to the UI.

Would it be possible to change the limit to 0?

Should we inform the user that if all of the minimum quantity if set to 0 would disable the running low command ?

Would you help me change to the newer version of setlimit vegetable /qty 0 ?

Similarly here would be "Okie dokie! The new minimum quantity for category 'VEGETABLE' is 0"

No. I was thinking that these two would be sufficient as we would have the user enter the amount either by /qty or /wgt only

From the website is actually just a term for us to have a common understanding of certain key words ?

Glossary: A glossary serves to ensure that all stakeholders have a common understanding of the noteworthy terms, abbreviations, acronyms etc.

What do you mean by singular verbs? Do you mean to add the user to everything?

i.e The user has a tendency to forget expiry date and location of the food stored?

We will consider as a feature improvement.

We will consider this as a future implementation.

Repeated #187

We will consider this for future implementation.

Do you think it's better to put these constants in a more specific class, eg: StorageConstants, since there will be many different constants for different parts of the program

Consider renaming FILE_FORMAT to something more descriptive of what format you're looking for?

Suggestion: TXT_FILE_EXTENSION, TXT_FILE_FORMAT, TXT_FILE?

Consider replacing this line with:

import static seedu.duke.common.Constants.(constant name)

so you don't have to repeatedly use Constants.(constant name)? I think if you are using most of the constants in that class, it is acceptable to use *.

Same as Loader class, consider import static?

Good job moving validation to the respective class. Makes it more organized and logical. Same for email validation (isValidEmail).

Nice, refactored this to a new class makes Parser cleaner.

Nice refactoring of the getDashboardCommandFromString method.

Nice, will keep in mind

I see, moved this to new class. Got it!

Possible typing error here!

I did think of putting it into a separate Constant class, but I was not sure where else they would be used other than the parser. Perhaps I can separate them into a class called ParserConstants?

Okay! I will make the change

For now we are coding for the happy case so it's irrelevant. I wrote that comment to remind us later on to catch that exception when user enters an invalid lesson type

Oops, missed that one. Thanks!

I was thinking to let the respective command objects catch the exception? What do you all think?

For now, I will add a comment to remind myself to throw those exceptions later on in our 2nd week

Just checked with Isa, no parameter needed as he will be getting user input for the index.

WIll do

We could move it there, but it has to be public maybe so everyone can use it?

Will remember to update in next PR, thanks!

Magic strings are as placeholders for now.

Okay, will keep in mind!

Sorry for the overlook! I was following the module website instructions which told us to run the jar using "java -jar". Managed to run app after running the command as mentioned in UG:

@okkhoy

@xhxh96

Shall I reopen the issue prof @okkhoy ?

After merging, can @H-horizon and @aliciatay-zls check your respective parts (Hemrish -> add lesson, Alicia -> add task) in the parser.

There are magic string warnings which I have put there for now as placeholders. (look for the comment "//PLACEHOLDER")

Can you replace them with your own proper warnings by declaring them as constants?

Resolved by handling non-integer input in checkIndices method.

Resolved by handling non-integer inputs in checkIndices method.

Change of mind, link validation will be done in parser. Validity of provided link will be handled by OpenLink command.

Resolved in PR #131

Do let me know if you disagree or have other suggestions for the command names. I will update in the UG tomorrow and push to this same PR. Thanks guys!

Looks really good, nice! Only one thing, maybe can use "tsk" for the task commands? Not so much to save letters but more to standardise so that all the commands are easier to remember for the user.

Hm you have a point there about consistency, we did consider it at first but then we realized that we have quite a few other 4 letter commands (exit, list, close - 5 letter) which did not need to be shortened as they were already short enough, plus keeping the full word where possible might improve the understandability?

Check is already being done, just that it is not indicated on the UI.

Can print a date and time to differentiate.

Capitalize the lesson type, eg. tutorial -> Tutorial

Resolved in v2.1

Handled in another issue.

Use proper regex string

No.

Hi, ok.

That's new to us, will change.

Be thankful we are user friendly

[After discussion with the team] No.

[Related to previous issue] Thanks

Awaiting response from Zikun

Good suggestion but nah

Tthank you. Wwil fix.

Considering. Might implement for v3.0

Screenshots would have been nice.

No.

It's a feature, not a bug

Link can be added, but cannot be opened. It's the responsibility of the user to enter correct links.

For add lesson, there is no integer involved in the command. We are handling inputs as strings.

For other parts of our program:

"The int type in Java can be used to represent any whole number from -2147483648 to 2147483647"

Hence, any values beyond this range can be considered an invalid integer.

This is getting out of hand.

This is a CS1231 argument. Vacuous truth.

Remove font functionality for v2.1

This one probably using assertDoesNotThrow will be better, or just put without the try/catch?

Probably a renumbering is required (1. 2. 3. 4. ...)?

Oh. I notice that. I will merge it then. Thanks!

I thought the records are only stored as a single string? If that's the case, I think simply printing out should be sufficient.

Probably you would like to use Singapore time format instead of the default ISO one for both printing and parsing.


protected static String datePattern = "dd/MM/yyyy";

...

LocalDate date = LocalDate.parse(dateString, DateTimeFormatter.ofPattern(datePattern));

System.out.println(date.format(DateTimeFormatter.ofPattern(datePattern));

As per SE-EDU's markdown guide, using all 1. might be better.

Would it be better if to use markdown's inbuilt tag instead?

Like this one for our user guide!

I would suggest use another delimiter as "\t" is a white space character. I am not sure whether it will cause unexpected behavior since I need to filter all these delimiter in Parser as well.

Delete

Add if message is invalid command, print out one more line notice.

Thank you! Corrections has been pushed.

Thanks! I have added a few more lines to this.

Thank you. It was fixed along with other occurrences of typos.

Thanks! It has been updated.

Issue reference: #22 #24 #25

The documentation bug is confirmed. It should be S7654321F. Others give an invalid message as they are in deed invalid - we use NRIC checksum to verify the validity.

Problem confirmed. /dd is ignored but we allowed an empty record command. This will be fixed in v2.1

Usage is record [DATE] [/s SYMPTOM] [/d DIAGNOSIS] [/p PRESCRIPTION] (specified in UG)

If date is omitted, it will be today's date by default. If you want records to be updated to the 3/3/2021 one, the date must be specified everytime record is executed.

For T0012345, the checksum is A. So T0012345A is valid, but T0012345C is not. It says invalid because IT IS INVALID. That is basically a non-existing ID number.

For our current design, only the "meaningful" part of the command is used. For current command, since it does not require a payload/any arguments, everything behind the command itself will be ignored.

We still value this suggestion but it should be a won't fix (at least in v2.1) and it is not a bug.

This is a valid suggestion on documentation but it is a duplicate of #108. Please refer to it.

Probably we would need to implement a check here to ensure dates are today and/or the past.

Probably we can add a check to ensure an input entry is not exactly the same as an existing one.

This has been mentioned in the UG that you have to load a patient before you can record things to it. It is also shown in your screenshot.

This command requires that a patient has been loaded with the load command.

If no patient has been loaded, Patient Manager will print an error message.

Duplicate of #115. This is a valid suggestion it will be fixed for v2.1.

This is a valid concern and should be fixed in v2.1.

We use the last digit ("checksum digit") to verify the validity of the IC numbers. For T1234567, it should be T1234567J. Similarly, there is no IC numbers starting with e, so it is an invalid number.

This was fixed in #83.

Fixed.

We no longer allow a record without any content to be added.

This has been fixed.

@brandonfoong Yes. I understand that. However, I think the suggestion not allowing future dates sounds reasonable that's why I added it. That should largely be a typo if it occurs. For the computer clock problem, I think it is minor and users probably will want to ensure their computer clock being accurate before use our program.

It will be problem if there is a long list of patients with similar ID (e.g. they were born in the same year). It will also not be computationally efficient to do a O(n) search for all items for each and every load executed. We understand this is a good suggestion but it would be implemented.

Good use of simpleton pattern!

Great job adhering to OOP by using less static!

it's better to not leave commented code if you are unlikely to use it again

Is it possible to separate the save all persons and load all persons test to 2 separate tests?

since you use this a lot, perhaps you can abstract out to a method and use if condition in that method for readability.

v2 👏👏👏

maybe a short comment to let the reader know 76 is the length of the divider or using a static final int

Good use of oop for command

maybe can say:

with the use of commands using just the keyboard.

I think you can add that you implemented the clear command too

This is harder than expected

test comment please ignore

If i say so myself 👌

Will add UG entry to clarify that it only moves the storage, not loads it.

the code fails stylechecks on line 69-83, could you check and edit? thanks!

I think there may not be a need to add the before tag if you are calling the methods in each test method since it might be executed twice?

This seems to be a little too nested

what does edit do if program is not in review mode? Should we add a warning since this command does not work in reco mode?

you might wanna remove the extra lines haha

try to use the existing ui instance

In this case anything other than 'n' will abandon the recommendation. Shall we have a loop to allow multiple tries for inputting y/n?

duplicate review says review list but duplicate recommendation only says list. Shall we standardize it?

Instead of this is it possible to define a new method in UI printWhiteSpacePrice that uses MAX_WHITE_SPACE_PRICE = 17 instead?

Need to leave a empty line between each method

Need to remove the space after InvalidCommandException e

Change to single import

Cannot have empty catch statement

I don't think you need the else part. You can just setCheckIn(false) outside the if without the else statement to make the happy path prominent.

Yes I agree too. Likewise for line 70 and 99 in Duke class

Good use of Singleton design pattern

If ignore, then we just close this PR?

We have generalized it to anywhere instead of just malls.

Looks good to merge 😃

Looks good to merge 👍🏼

Will be continued in PR #73

Looks good to merge

Looks good to merge!

Looks good to merge but @hussain1998 remember to address these problem in the next PR.

The tests should fail. Work in progress.

Looks good to merge

Alright looks good to merge!

Looks good to merge!

The check problem will be solved in the next PR.

Looks good to merge!

looks good to merge

Looks good to be merged

Looks good to merge

ok looks good to me!

LGTM!

LGTM

Looks good to merge!

Test PR

Looks good to merge! Well done!

Looks good to merge 😂

CYC understood n/Ian /p13572468 as a person name since the user did not start the phone number with /p.

This is intended to check for whoever has checked out.

PersonLog.txt cannot be touched. Mentioned in UG.

list becomes listcheckedin

Change error message

Ask the person editmax

We did not do that as we wanted to order the details according to priority level.

#195

Remove location name from args

Editing particulars will be implemented in v2.2 and above.

Add extra line

Venue name will be removed

Thank u

Add into QnA

Location name will be removed

Looks good to merge

The clear command only clears the entries in the tracking list. It does not clear the person log file. That is why it is not possible to check in another person with the same ID.

Do not approve.

This issue can be closed since it has been done in #250

This PR can be approved if its okay 😃

The changes in the DG after table of contents to around line 400 was due to handling of merge conflict when attempting to pull #260 . I should have accidently missed some lines there.

missing break;

Duke? not FridgeFriend?

outdated? we dont have todos

the 'save' folder

pull request #67 has the instructions to save

That conflicts with Food Storage Location, and save can be a noun anyway

The switch statement is missing the break; for the cases

LGTM. Though can we change all instances of Duke to FridgeFriend?

LGTM. Are we good to press Merge button?

LGTM

Ah the checks are failing because the IO tests have not been updated

Right how can you account for the saving function in the ui test

LGTM

LGTM, thanks for the help!

Maybe you can consider repositioning the class diagram as current it is quite hard to read

Perhaps some attributes or methods can be omitted as it may not do much in explaining the UI Component

Why don't you consider maintaining a unified style for class diagram? Perhaps different color for each component will allow the reader to better understand the class diagrams

Consider hiding the boxes at the bottom using "hide footer"

We will mention this in the UG

Since the saving has to be done regardless of whether the file exists (if it does, overwrite the existing file, if it doesn't create the file) would it be fine without the conditional?

Not much of an issue, but the markdown coding standard seems to prefer * over - for bullet points

The phrasing here can be a little confusing for readers. Perhaps "Words or characters starting with / marks the beginning of an argument" would sound a little clearer.

Initially I had them labelled 1. 2. 3. but according to the markdown coding standard, using generic numbers (1.) makes it easier to insert more items in the future.

... so that the records can be stored for safe-keeping

WAIT WHY ARE MY COMMITS ALL MADE UNDER A DIFFERENT ACCOUNT

History has been rewritten, shall redo the whole PR.

Marked invalid as it is stated in the user guide that the record command requires a patient to be loaded.

don't have to do it now, but by right unit tests should test for negative examples too

should the > and > brackets be together with the input?

Most likely will be delayed

How is this severity.High? Will fix in the later release.

This is documentation bug. Will fix accordingly along with other documentation issues.

How is this severity.Medium? Aren't all testers supposed to run the JAR files using java -jar?

How is this severity.Medium as well?

I think we should change them to:

System.out.print("Thank you for using TraceYourProj!" + "\n");

System.out.print("Hope you have a wonderful day." + "\n");

It is because the text-ui-test may fail due to the line separator not matching.

I suggest adding list-all again to see if the resource is really deleted.

We think that this functionality is no longer useful. Therefore, the development of it stopped. (Close issue)

The behaviour of add functionality has been modified. No overwrite will be performed now.

I think the circles should be removed according to prof's post: https://github.com/nus-cs2113-AY2021S2/forum/issues/54

Good spot! Don't know where this was used in.

thanks for editing this to make it look nicer.

Failed.

Fixes #18

Hi Jason, your user story is nicely crafted with the user, needs, and reason of needs.

Fixes #33

Reopen - close only after V1.0 is working.

Hi Gerard, thank you for the work, the code looks good and organized.

Closed due to failing tests. @gerardtwk can you kindly recheck on the runtest.bat. Thank you

Update title typo from validateDate to validateAmount.

Good job Gerard. Thank you for your hard work. The code looks good.

Thank you for your hard work @marklowsk. I'll test out the program with mine.

Thank you for working till so late. Appreciate it.

Hi @tzexern, thank you for finishing up the remaining methods.

Hi @tzexern, this issue has been completed. Can you kindly close this issue for both of us?

Thank Jason!

Thank you for the hard work @tzexern.

Thanks, @jonahtwl. This portion was difficult to handle, thank you for your effort and hard work!

Thank you for the update and constant improvement! @marklowsk

The program now shows up to 2 decimal places with proper amount validation and error messages.

Looks good.

Thank you for testing!

Thanks @tzexern

Thanks for fixing the bug!

Thanks for the fix and sorry due to the bug in github.

Github bug: #85 passed but afterward show failure.

Hi, @tzexern can you kindly please close this issue for us? Our team have completed v1.0.

Thanks @marklowsk.

Update: closed due to keeping -i feature.

Fixes #102

@marklowsk Thank you for your hardwork!

Thanks @marklowsk for setting up the skeleton.

@tzexern parser side for credit score done (see PR #103).

Good work @jonahtwl 😃

Looks good!!

Nice Architecture.

Nice update on DG

Nicely done

Complete in PR #108

thank for working on the update!

Good job @jonahtwl

Agree with mark. Thanks @gerardtwk

@tzexern thanks for the improvement!

Thanks for the update @marklowsk.

Thanks for updating the UG screenshot and DG updates.

Fixed in PR #196

Fix in PR #197

Fix in PR #196

Hi, thank you for your feedback. I think you might have missed out on some portions of the UG due to time constraints but we have stated that >> represent an area where data is required. And under the 3.8 of the UG, we also stated that the features are named and highlighted in mark up. Below are some screenshots for your reference.

Thanks for the suggestion, we will update the user guide on this.

Hi thank you for suggesting but the argument that was read in was recognized as "2.9 -d20032021". There are no other valid syntax options after it.

Updated in PR #191.

Fixes #197 to address amount validation.

Fixes #198 to address help output

Thanks for the fixes.

Thanks for spotting this.

sounds good

do we use reviewS list?

Try changing \n to System.lineSeparator()

Repeated issue #85

Implement as 1 step

Repeated #94

Maybe either just take from the list of all emails or explain in the user guide

add read to storage

repeated #102

repeated #95

Fix #102, #109, #110

I have two emailManager objects, one for sender, one for recipient. Using static means that the emailList is shared between all the objects. So when I update the recipient's emailList, the sender's list gets updated too

I am thinking that the tags is up to the recipient to set themselves and the tag is not linked to the emails. An example is like teachers may tag an email as work and send it to students but for the students they will tag it as school.

I think should be "list-all" here instead of "list all".

Thanks Sam for checking. My bad, I will make the changes asap.

Yes sure, I have updated on your behalf.

Can consider overriding equals() method for Project instead

Duplicate of #135

Duplicate of #133

It includes information about the person details, time and movement whether its in or out

LGTM

Current UI

LGTM

Ui need some touch up

LGTM

Fixes #155

Fixes #154

LGTM

shouldnt have indentation here

yes

Invalid usage of the command.

It is a feature of our application to ensure good data validation.

I think @EmilyTJX did changes for this alr, will there be merge conflicts?

should we decide what features we want to implement for the reco mode? like for example can the user edit in reco mode?

edited accordingly!

ok i have resolved this alr!

standardised!

I see that this implementation only sets the answers, if assignment.getAnswers() == null. Maybe you could consider changing it so that we set the answers every time the answers are loaded from text file? This way the answers will update in case the text file was changed.

LGTM

Thank you for the feedback. This is a user guide problem that we will fix

Thank you for your feedback, we have added the command to our user guide!

Thank you for the feedback, we have updated the user guide.

Thank you for the feedback, I have updated our user guide

LGTM

Please update test-ui-test 😃

I need to modify this when composing email too. I thought about this when i saw the error and I was thinking that we might need to check '.' too

I will add a parser method to check if email is valid

@jalvinchan you changed this right? Do you want to change to "list all" instead? Sounds better. Otherwise can just close this issue!

@jalvinchan, send email by checking if email is in the system. If yes, send, if not, print error message. Thank you!!

Noted with 1, but actually why so?

ya my rationale behind it is to simulate real email system where you can edit to anything you like, but just cant send unless you change to a correct email address

yeah good point!

actually, i believe user doesnt necessary have to list allemails, they can list draft, archive etc.

@jalvinchan

Sorry, do you have any suggestions? haha because i just followed the example they gave on the week 7 project

okay will change accordingly. thank you

are u referring to the whole chunk of code inside the while loop? what would you recommend I call it?

okay thank you! done!

okay done!

good point! done!

CS-add name

admin can now verify themselves

admin can now add stores to canteen

done!

KIV

done

Fix !

fixed !

Can you provide SS. Thanks.

fixed

I think you might have misunderstood the UG.

You should select canteen and store first.

exit is suppose to end the application and take you out of the file. Explained in UG.

You type only when it shows that you can type.

We will leave it for future development

merge approved

Decide not to do due to potential bugs

Hi, in the DG, it is stated that :

dd must be an integer between 1-31

mm must be an integer between 1-12

yyyy must be an integer between 2021-2099

Hi, it was mentioned in the DG that:

dd must be an integer between 1-31

mm must be an integer between 1-12

yyyy must be an integer between 2021-2099

Thank you.

This is already fixed for the new tp.jar

move to local fork

duplicate

tutorial task

these text files are put inside jar.

Did this occur when you logged in as admin, or did this occur in public user? Did this occur when selecting stores?

Unable to reproduce issue

Unable to reproduce error

What is the issue?

Is there any screenshots so we can reproduce this issue? Did you also download our pre-made text file before testing (as per user guide)?

Thanks for the constructive feedback.

You must have selected a canteen before you can add a store. May I know the steps to reproduce this issue?

Did you download our data folder as stated in the user guide?

I understand your concerns. In production purposes, we will remove the admin password from the user guide. However, as we are only developing this app currently, we have to provide the password for you to test. Without it, you will only have access to half of our app.

Refer to issue #124

I appreciate your concerns for the inappropriateness of deleting reviews. However, it is necessary to control the input of reviews, so that we can prevent potential issues such as unrealistic and inappropriate reviews.

Noted on improving our user guide. We will try to continuously improve on it!

Thanks for the comment! However, we might leave this to a future iteration.

These are for our logging purposes, and will not be shown to the user. Thanks for correcting our English though. We will look into ways to prevent logging from being shown to the user.

Refer to issue #110

Proper application structure has been established

LGTM

Add 'do not edit .txt files' to UG

Handle exception

parser handle exception

Handle exception

Add all relevant rectified issues

LGTM

Previously rectified

There is a mistake in addToSent method

this feature is not that important to have

Everyone can supplement their own parts

examples in the feature part almost all need to change later

Looks good.

thank you!!!

done

bcz of typo. Done

solved.

Solved! Thanks a lot 😃

Hi, thanks for the feedback. We were considering it in intellij environment. The change in tone was not intended actually, I wanted it to be friendly throughout, but will review it as per your comments.

Thanks for the feedback. I think you did not download the latest jar file and data folder.

Hi, thanks for the valuable feedback. Will fix it!

Duplicated issue

Not necessary to have anymore

Jar file not updated for testers

current versions has solve this issue

@yanli1215 @jalvinchan @FarmZH98 please help close issue if no problem hehe

Team to discuss if users should be informed that the user would have to first create a type before the list shows/displays any information

the jar file has not been updated during the testing.

team to help close this issue if appropriate

jar file has not been updated during testing

Team to decide if this error message is good enough

jar file has not been updated during testing.

error handled in the latest version

Team to close this issue if appropriate

jar file not updated for testing

regardless, updated code to make it more bug free

jar file not updated for dry run

current version is up to date

Issue solved in current version

Error message corrected in earlier version and not updated in jar

Latest version check that is the same for your machine bah

To decide if we want to implement this feature!

@FarmZH98 Yeah you r right let me change and update here

Team to decide what to do with this

Change user guide

this is resolved, the jar file is not updated during test

@Chihui8199 to check for "." too

Junk and deleted emails are different

https://ay2021s2-cs2113t-f08-2.github.io/tp/UserGuide.html#7-command-summary

Fail. Rename DeliveriTest.java and Deliviri.java to DukeTest.java and Duke.java respectively

Fail. Missing package.

Add bye command to UG

Unable to replicate issue

Looking great! We can move on to the next phase now.

Thanks for the changes! Look good to merge now 😃

Thanks for finding it!

Note to merge PR #59 before merging this.

Looking great, thanks for the good work!

Looks good, now I have a clearer idea of what to do for Credit Score, thanks!

Fixes #98 and #95

The UserGuide looks better now! Thanks for the screenshot!

Thanks for being sharp! Didn't notice about this small issue as I refactor the code

Looks much more readable now. Thanks!

Looks great! Thanks for the work!

Looks great, thanks for the hardwork!

Hi Jonah,

Please indicate in your section that users are not allowed to modify finux.txt at all.

Thank you

Closed due to the branch not being up to date before sending the PR.

Good work! Thank you

Thanks for the edit! It looks much better now

Looks good to merge! Great work!

Thanks for handling these issues, the UG looks much better now!

Looks good, thanks for the hard work!

Fixes #9

Duplicate of #10

List package is not complete yet.

Thank you for your suggestion. We have taken note of this issue and decided to allow showing the progress of MCs more than the course requirements (eg. 160 MCs) as it is stated here that MCs more than 160 will still be computed towards your CAP.

Thank you for your comment. The CAP should be calculated even when the total MCs exceeded 160 MCs based on the NUS CAP calculation method here.

Thank you for raising this issue. My team and I think that it is better to implement a new "info" feature for viewing the details of a module (including prerequisites) rather than listing all under the "list" feature which could be too cramped.

fixed bugs and add delete for moduleplanner

great job!

fixed with post PED commit

Great use of logging to keep track of program control flow and exceptions. Also noticed additions to JUnit tests which will be good for code coverage. Overall very well done!

Multiple schedules allowed for same patient ID as long as different date

I wanted to ask you guys first before I deleted the line. On whether I should keep the previous one or current one.

If if looks ok I will delete the comments in the a new PR

I tried to do it but had synchronisation errors, I will try again later on!

I think this one was solved by @JonathanKhooTY

I have done the change to ""

I have extracted all the magic values and used global constants instead

so that the records can be stored for safe-keeping

I can record the patient symptoms

The records can be stored for safe-keeping

There are some checkstyle errors causing the checks to fail.

/home/runner/work/tp/tp/src/main/java/seedu/duke/Storage.java:95: At-clause should have a non-empty description. [NonEmptyAtclauseDescription]

/home/runner/work/tp/tp/src/main/java/seedu/duke/Storage.java:121: First sentence of Javadoc is missing an ending period. [SummaryJavadoc]

/home/runner/work/tp/tp/src/main/java/seedu/duke/Storage.java:154:56: '(' is preceded with whitespace. [MethodParamPad]

SORRY! I accidentally opened a pull request in the wrong group

Doesn't work

Checkstyle Errors

Failed check for expected output

PR #23 completed this.

Thanks @marklowsk for the abstract command class, will continue working on the other subclasses.

Looks fine, thanks for refactoring.

Thanks for refactoring the code! Looks cleaner now

Great catch on the bug!

Will change the Double variable to BigDecimal and PR again!

We are removing the index (-i) option right?

Do let me know if anyone has a question with regards to this.

Looks fine, thanks for the quick work

Looks fine

Thanks for getting this up quickly

looks fine, thanks for the quick update

Looks great! Thanks Mark!

Nice enhancement! Thanks for the quick update

isReturn NOT removed
returnDate is added
returnDate is saved
returnDate can be loaded

Looks good! Thanks for updating.

Thanks for refactoring and editing the images!

Nice job!

Great work.

Please do let me know if there are changes to be made. For minor changes (spelling/full stop), I believe we can edit directly on Git

Okay nice!

Hi! Thank you for raising this up! We will make it more explicit in the UG in the explanation of the credit score feature!

May we have some extra information on this bug?

Thank you for pointing this out. We will edit our UG to make this clearer!

Okay! Thank you for your review!

@nus-pe-bot @ongweisheng Yes! We've already fixed it.

Thanks:)

@nus-pe-bot @ongweisheng Yes, we've fixed it already!

Thanks! 😃

@nus-pe-bot Thank you for your report! We've fixed it already.

@nus-pe-bot Thank you for your report. We've fixed it already.

@nus-pe-bot Thanks for your report. We've fixed it already.

@nus-pe-bot Thank you for your report. We've fixed it already.

@ongweisheng Yes, we've fixed it already. Thanks:)

Looks good

Implemented in #25 PR.

Looks awesome

Looks good to merge

Looks good

Looks good to merge

Cancel PR.

CI Error detected.

Thanks for the useful help commands!

Sure, no problem buddy! @LeeHanYongAndy

Implemented in #63 and #65

CI Error.

Thanks for the bug fixes @marklowsk !

Looks good, thanks!

Yep, no problem buddy.

The test run looked fine.

@gerardtwk You may want to change the type of the parameter days in the getCreditScore method from int to long.

Good work on DG!

Thanks for the changes!

Thanks for the edits!

Thanks for the updates!

great work!

Thanks for the improvement Andy!

Thanks for the refinement!

Great job on the diagrams!

Thanks for the UG updates!

Thanks for the changes!

Thanks for the updates!

Thanks for the updates on view andy!

Non-issue, hovering over the blocks mentioned, E3 and LT5 shows that they are in fact connected

@Rye98 kindly take note and close when added

Non-issue. Following the map attached the shortest route is correctly determined. Cutting across places with no pathways are not considered.

Non-issue. The deck is neither an engineering canteen nor a computing canteen. It is an arts canteen.

Non-issue. Central Square is not part of engineering neither is it part of computing. It is located at YIH which is not covered under NUSMaze.

Seems to look fine on my end. Will check around.

It is the intended functionality. Edit will be considered.

Will make documentation clearer.

Will change 2 and add in picture for 3. Will try to simplify 1 if possible.

Will be considered as improvement

Non-issue. This is an intended feature. Users are allowed to call blocks multiple names if they desire.

Intended behavior. However warning as an improvement will be considered.

Will consider. This was a conscious decision as multiple errors may clog up the CLI with invalid inputs.

It is the current intended functionality. Will discuss with @wjchoi0712 and decide if changing it is better.

similar to #150

similar to #153

Non-issue. There is no direct path from E3A to T-lab according to the map below.

Non-issue. According to the attached map E3 lies in between the mentioned path.

Users are not meant to touch the data files.

Non-Issue. Users are not meant to touch the data files.

Non-issue. Users are not meant to edit the data files.

Non-issue. Users are not supposed to edit the data files.

similar to #162

linked to #159

Fixed by #116

fixed by #116

fixed by #170

Error message will be changed

similar to #164

similar to #157

fixes #145 and #167

fixed by #176

fixed by #173

Will be considered for future implementation

Looks good

I will work on the next phase of the commands. Thanks!

Thanks for fixing the parser!

Looking good, nice use of JavaDocs

Thanks for adding in the common.Validators class for code reuse!

Thanks for adding the exception.

Very fast bug fix!

Implemented in #45,

changes made in #49,

and Record toString() method finalized in #71.

Good work!

Validation finalized in #68.

Thanks for reusing the validation method!

Discontinued due to prioritization of other features.

Can also update it with Intro, Help and View feature if you have time. 👍

Yes, e.g. from remove -i 7 to remove 7.

They will try to implement it as planned.

Sorry, forgot to mention to add in the person attribute too.

Done in #122

Thanks for pointing this out. This is due to the default ResolverStyle of the DateTimeFormatter.

Year era AD only, starting from year 0001.

We only do not allow empty strings for description.

Hi, it is stated at section 3.1, the top section of add feature.

Thanks!

Thanks for pointing this out. This is due to the default ResolverStyle of the DateTimeFormatter.

Fixed in #193.

Looks good to merge!

Fixed in #193.

The credit score of 'alex' is the same as the issueDate of alex and returnDate is on the same day.

So there is no deduction of credit score.

Yes, we do not have unique IDs for each record, relying solely on the list entry index.

We plan to keep it this way as this is still a viable for this iteration.

Deleting an above entry will reduce the index of all entries below naturally, so we will not identify it as a bug.

Since the fileWriter uses inFile, there will be an error if I put it outside the try catch block. I'll try to see if there's anything I can do about this. Thanks!

hmm I think can merge emily's pr first? then i try to pull and fix merge conflicts. or the person merging can fix

can avoid merging first - empty class

Sorry, I will make all hyperlinks following the convention we did for UG.

We could keep for now. If not required, we can delete later 😃

Fixed! Sorry for the typo!

I will check all exceptions and make sure they are caught at right place and print correct error message when adding an item later today 😃

I see, will make changes to it 😃

Kexuan has fully implemented the feature relating to the user story (as a user, I can see orders-related commands).

Wei Sheng has fully implemented the feature relating to the user story (as a user, I can add new orders).

Zikun has fully implemented the feature relating to the user story (as a user, I can clear all orders).

[Zikun] Wrong messages are printed. Please fix the bug.

Logging with storage functionality is to do done later.

Our user guide still needs to be refined and all relevant pull requests should be linked to this issue.

Consider changing item tips to item stats(statistics) instead

That's a better suggestion, thank you!

Mutiple items can be deleted using item update command. This user story might not be relevant as of now.

It might complicate things and increase learning cost for the user. We will stick to delete index for items.

It might complicate things and increase learning cost for the user. We will stick to delete index for orders.

Because the price required was in integer, will change all to BigDecimal in v2.1

I see. Let me change it to BigDecimal 😃

Not recommended to add more features after v2.0.

The items add command of this version of easyLog is consistent with its orders add command. Specifically, the user needs to input item price and item command in 1 line. Also, the user will be prompted again and again if a pair of valid item price and stock is not given. The coding style is also improved.

The Ui class and Message class are not done yet. It is intentionally left for another member to do.

Could you please elaborate why item description/name cannot be a pure number?

Thank you for your feedback, can @Cocokkkk kindly check if the bug still exists in the current version of easyLog?

Thank you for your feedback. We have recified them 😃

Thank you for your feedback. The current version of easyLog shows the correct message for invalid item price 😃

v1.0-trial-MingShun Version

Thank you for the issue, bug fixed

bug fixed

Yes it is allowed, will reflect clearer in UG

bug fixed

Give Zhi Fah more lines please he has 1 line

Oopsies

Oopsiez

The situation described here is very unlikely to occur to the target audience for our application. In fact, there are little to no situations where this scenario is likely to occur at all.

Please read the user guide in detail, the error message is valid and as intended.

As good as the suggestion is, the error message is detailed enough and is sufficient for the target audience, a NUS student, to see the error in his input.

Duplicate of #193, will be resolved under there.

Not an intended feature for our app, users can refer to NUS website for more information.

update v1.0 version

#resolve 101

Issue has since been rectified with by altering storage method

Command shows intended behaviour, refer to UG for proper usage of update command.

Duplicate under issue #212. Will be classified as issue #212

Duplicate issue with #201. Will be fixed under #201

User guide specifically mentioned not to mess with the configuration file. The intended behaviour under a modified/corrupted file is to avoid program crash.

Duplicate issue under #196. Will be tracked under #196.

Duplicate issue under #209. Will be tracked under #209.

Duplicate under #187. Will be tracked under #187.

Duplicate under #198. Will be tracked under #198.

The command is designed to fit all flags in a single command specifically for fast typists, who can quickly add a new module in a single line. The suggestion has been extensively discussed previously and the conclusion was that having multiple prompts would not benefit fast typists and other users, as they need to constantly pause and await system response.

The update command does not work on its own, without the need for done command. Duplicated under #209. Will be tracked under #209.

LGTM

cool use of AssertThrows

LGTM

As a TA, I can autograde MCQ assignments which have set values and short answer assignments, which can be short sentences

also output list of students who did not submit work for autograding

LGTM

Great job in improving the parser class to separate out all the parameters from the user input. I like how you declared the length of certain separators and commands as final variables.

However, a concern I have with this implementation is that the module code has to be a parameter for every single command. Would it be easier to select a module we are working in so all the actions performed are done in the context of that module? It would also reduce the number of unnecessary parameters for each command!

LGTM

Logo is correct

issue done

Looks good for merging

Looks ready to merge

LGTM

Looks good for merging

Looks good for merging!

LGTM

Looks good to merge

tP comments dashboard

1. RYAN KWOK @kwokyto (73 comments)

2. Chong Wen Hao @8kdesign (36 comments)

3. ONG WEI SHENG @ongweisheng (35 comments)

4. LEE YANG PENG @Leeyp (31 comments)

5. BUNDHOO HEMRISH @H-horizon (22 comments)

6. Song Yu @Song0180 (19 comments)

7. ALICIA TAY @aliciatay-zls (17 comments)

8. WONG JUN HONG @jadenwjh (13 comments)

9. Hu Wenqi @Vinci-Hu (12 comments)

10. FOONG YAN KAI, BRANDON @brandonfoong (12 comments)

11. ISA BIN HARON @isaharon (11 comments)

12. SIM JING JIE @SimJJ96 (10 comments)

13. IVAN CHONG ZHI EN @ivanchongzhien (10 comments)

14. LIU ZHUOHAO @fsgmhoward (10 comments)

15. IVAN TAN JIA AN @sarzorwyn (9 comments)

16. WANG HUA CHEN @huachen24 (9 comments)

17. A AKIL AHAMED @iamakilahamed (7 comments)

18. KIM JOOHWAN @joohwan58 (6 comments)

19. CHOI WONJAE @wjchoi0712 (4 comments)

20. MABEL KOH JIAXIN @Emkay16 (3 comments)

21. Chew Jing Wei @tehtea (2 comments)

22. Ng Man Sing @NgManSing (2 comments)

23. LEE HAN YONG ANDY @LeeHanYongAndy (2 comments)

24. EMILY TOCK JIAXIN @EmilyTJX (2 comments)

25. JALVIN CHAN JIA XIANG @jalvinchan (1 comments)

26. Jovan Huang Tian Chun @jovanhuang (1 comments)

27. Goh Chen Kang Sean @seangoats (1 comments)

28. LIM WEN FENG @limwenfeng (1 comments)

29. OWEN YIP JING EN @blank-bank (1 comments)

30. KRITHIGHA PANNEER @Krithigha24 (1 comments)

31. VIKAS HARLANI @vvvvh123 (1 comments)

32. FARM ZHENG HAO @FarmZH98 (0 comments)

33. CHUAH JIN WEI @heyjinwei (0 comments)

34. YEUNG CHI SENG @JoviYeung92 (0 comments)

35. Li Haoyang @lihaoyangML (0 comments)

36. Violin Yapputri @violinyap (0 comments)

37. Zhang Yongzhe @zhangyongzhe20 (0 comments)

38. Ng Wai Kit Kevin @KevinNgWK (0 comments)

39. Kalana Manika Hennedige @ManikaHennedige (0 comments)

40. Liu Yanli @yanli1215 (0 comments)

41. YAN YI XUE @yyixue (0 comments)

42. Cao Shuwen @cswbibibi (0 comments)

43. Wang Kangxin @kangxinwang (0 comments)

44. Ng Chi Hui @Chihui8199 (0 comments)

45. Low Soo Yee, Calvin @e0699194 (0 comments)

46. GERARD TAN WEI KIAT @gerardtwk (0 comments)

47. LOK KE WEN @kewenlok (0 comments)

48. FANG XINJIA @fangxinjia0203 (0 comments)

49. ALEXANDER TAN JUN AN @AlexanderTanJunAn (0 comments)

50. H MOHAMED HUSSAIN @hussain1998 (0 comments)

51. HIONG KAI HAN @hiongkaihan (0 comments)

52. JONAH THAM WEN LONG @jonahtwl (0 comments)

53. LI KEXUAN @Cocokkkk (0 comments)

54. TAN TZE XERN @tzexern (0 comments)

55. SIM BOWEN @SimBowen (0 comments)

56. MARK LOW SENG KEONG @marklowsk (0 comments)

57. LEOW XIAO XUAN @leowxx (0 comments)

58. ANG JIA HUI @jhjhajh (0 comments)

59. TAY JING YING KIMBERLEY @KimIdeas8 (0 comments)

60. ZHU ZIKUN @zikunz (0 comments)

61. KWEK MING SHUN @MingShun98 (0 comments)

62. XIA FUXI @fupernova (0 comments)

63. CHIAM JIA-EN @Chiamjiaen (0 comments)

64. SEH XIN RU @xseh (0 comments)

65. Wang Zihan @zihan9485 (0 comments)

66. Zhuang Jianning @jianningzhuang (0 comments)

67. Bryan Wong Hong Liang @bryanwhl (0 comments)

1. RYAN KWOK `@kwokyto` (73 comments)

2. Chong Wen Hao `@8kdesign` (36 comments)

3. ONG WEI SHENG `@ongweisheng` (35 comments)

4. LEE YANG PENG `@Leeyp` (31 comments)

5. BUNDHOO HEMRISH `@H-horizon` (22 comments)

6. Song Yu `@Song0180` (19 comments)

7. ALICIA TAY `@aliciatay-zls` (17 comments)

8. WONG JUN HONG `@jadenwjh` (13 comments)

9. Hu Wenqi `@Vinci-Hu` (12 comments)

10. FOONG YAN KAI, BRANDON `@brandonfoong` (12 comments)

11. ISA BIN HARON `@isaharon` (11 comments)

12. SIM JING JIE `@SimJJ96` (10 comments)

13. IVAN CHONG ZHI EN `@ivanchongzhien` (10 comments)

14. LIU ZHUOHAO `@fsgmhoward` (10 comments)

15. IVAN TAN JIA AN `@sarzorwyn` (9 comments)

16. WANG HUA CHEN `@huachen24` (9 comments)

17. A AKIL AHAMED `@iamakilahamed` (7 comments)

18. KIM JOOHWAN `@joohwan58` (6 comments)

19. CHOI WONJAE `@wjchoi0712` (4 comments)

20. MABEL KOH JIAXIN `@Emkay16` (3 comments)

21. Chew Jing Wei `@tehtea` (2 comments)

22. Ng Man Sing `@NgManSing` (2 comments)

23. LEE HAN YONG ANDY `@LeeHanYongAndy` (2 comments)

24. EMILY TOCK JIAXIN `@EmilyTJX` (2 comments)

25. JALVIN CHAN JIA XIANG `@jalvinchan` (1 comments)

26. Jovan Huang Tian Chun `@jovanhuang` (1 comments)

27. Goh Chen Kang Sean `@seangoats` (1 comments)

28. LIM WEN FENG `@limwenfeng` (1 comments)

29. OWEN YIP JING EN `@blank-bank` (1 comments)

30. KRITHIGHA PANNEER `@Krithigha24` (1 comments)

31. VIKAS HARLANI `@vvvvh123` (1 comments)

32. FARM ZHENG HAO `@FarmZH98` (0 comments)

33. CHUAH JIN WEI `@heyjinwei` (0 comments)

34. YEUNG CHI SENG `@JoviYeung92` (0 comments)

35. Li Haoyang `@lihaoyangML` (0 comments)

36. Violin Yapputri `@violinyap` (0 comments)

37. Zhang Yongzhe `@zhangyongzhe20` (0 comments)

38. Ng Wai Kit Kevin `@KevinNgWK` (0 comments)

39. Kalana Manika Hennedige `@ManikaHennedige` (0 comments)

40. Liu Yanli `@yanli1215` (0 comments)

41. YAN YI XUE `@yyixue` (0 comments)

42. Cao Shuwen `@cswbibibi` (0 comments)

43. Wang Kangxin `@kangxinwang` (0 comments)

44. Ng Chi Hui `@Chihui8199` (0 comments)

45. Low Soo Yee, Calvin `@e0699194` (0 comments)

46. GERARD TAN WEI KIAT `@gerardtwk` (0 comments)

47. LOK KE WEN `@kewenlok` (0 comments)

48. FANG XINJIA `@fangxinjia0203` (0 comments)

49. ALEXANDER TAN JUN AN `@AlexanderTanJunAn` (0 comments)

50. H MOHAMED HUSSAIN `@hussain1998` (0 comments)

51. HIONG KAI HAN `@hiongkaihan` (0 comments)

52. JONAH THAM WEN LONG `@jonahtwl` (0 comments)

53. LI KEXUAN `@Cocokkkk` (0 comments)

54. TAN TZE XERN `@tzexern` (0 comments)

55. SIM BOWEN `@SimBowen` (0 comments)

56. MARK LOW SENG KEONG `@marklowsk` (0 comments)

57. LEOW XIAO XUAN `@leowxx` (0 comments)

58. ANG JIA HUI `@jhjhajh` (0 comments)

59. TAY JING YING KIMBERLEY `@KimIdeas8` (0 comments)

60. ZHU ZIKUN `@zikunz` (0 comments)

61. KWEK MING SHUN `@MingShun98` (0 comments)

62. XIA FUXI `@fupernova` (0 comments)

63. CHIAM JIA-EN `@Chiamjiaen` (0 comments)

64. SEH XIN RU `@xseh` (0 comments)

65. Wang Zihan `@zihan9485` (0 comments)

66. Zhuang Jianning `@jianningzhuang` (0 comments)

67. Bryan Wong Hong Liang `@bryanwhl` (0 comments)