Skip to content
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

Add noorul room storage #64

Merged

Conversation

itssodium
Copy link
Collaborator

@itssodium itssodium commented Oct 6, 2020

Please review my PR where I changed files from .txt to JSON and removed arrays and used ObservableList.

Noorul Azlina added 9 commits September 29, 2020 21:05
* 'master' of https://github.com/AY2021S1-CS2103T-W12-1/tp:
  Update table of content for UG
  Update index.md
  Remove all traces of Addressbook
  Update AboutUs.md
  Update AboutUs.md
  Update _config.yml
  Add files via upload
  Update README.md
  Create README.md
  Update AboutUs.md
  Update AboutUs.md
  Update AboutUs
  Add picture
  Change file name
  Add image for AboutUs
  Update AboutUs.md
* 'master' of https://github.com/AY2021S1-CS2103T-W12-1/tp: (33 commits)
  Update UserGuide.md
  Update DeveloperGuide.md
  add docs for AboutUS
  Update UserGuide.md
  Update UserGuide.md
  Update DeveloperGuide.md
  Update NFR and Glossary
  Make changes to pass checkstyle.
  Make changes according to review given by peers
  Update AboutUs.md
  Update AboutUs.md
  Add profile photo
  Make changes to pass checkstyle and test cases
  Updated requirements section
  Update UserGuide.md
  Make changes to pass checkstyle and test cases
  Make changes to pass checkstyle
  Make changes to pass checkstyle
  Made changes according to comments given reviewers
  Made changes according to comments given reviewers
  ...
* 'master' of https://github.com/AY2021S1-CS2103T-W12-1/tp:
  Update JSON data to reflect valid and invalid Patient
  Package commands, parsers, and unit tests
  Transfer SearchPatient*.java to Patient package
  Replace all occurrences of Person with Patient
  update UG with searchpatient command
* 'master' of https://github.com/AY2021S1-CS2103T-W12-1/tp: (22 commits)
  Update RoomListPanel.fxml
  Fix checkstyle
  Add help command in UG
  Add UI tab for task
  Update help command
  Remove tag from codebase
  Add tests for new patient attributes in ParserUtilTest
  Remove email of person from the codebase
  Fix checkstyle error
  Fix bugs for UI
  Fix checkstyle
  Fix bugs from refactoring
  Adjust logo and remove white background
  Update RoomListPanel.java
  Update UI to display room details
  Allow UI to read and display rooms
  Fix UI for patientListPanelPlaceholder
  Fix styling issues
  Add RoomDetails to UI
  Change layout of javafx elements
  ...
Make changes from array to Observable list and change all storage files to jason
Make changes to pass checkstyle.
Make changes to pass checkstyle.
Make changes to command to allow easy understanding of commands by user.
Make changes to pass checkstyle.
@codecov-commenter
Copy link

Codecov Report

Merging #64 into master will decrease coverage by 0.82%.
The diff coverage is 59.34%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #64      +/-   ##
============================================
- Coverage     66.95%   66.12%   -0.83%     
- Complexity      480      482       +2     
============================================
  Files            84       86       +2     
  Lines          1646     1662      +16     
  Branches        194      193       -1     
============================================
- Hits           1102     1099       -3     
- Misses          488      505      +17     
- Partials         56       58       +2     
Impacted Files Coverage Δ Complexity Δ
src/main/java/seedu/address/MainApp.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...main/java/seedu/address/commons/util/FileUtil.java 81.81% <ø> (ø) 10.00 <0.00> (ø)
.../seedu/address/logic/parser/AddressBookParser.java 75.00% <0.00%> (ø) 11.00 <0.00> (ø)
src/main/java/seedu/address/model/room/Room.java 71.79% <0.00%> (-2.40%) 15.00 <0.00> (+1.00) ⬇️
.../java/seedu/address/model/util/SampleDataUtil.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...va/seedu/address/storage/RoomOccupancyStorage.java 68.75% <63.63%> (-18.75%) 4.00 <3.00> (-6.00)
...c/main/java/seedu/address/model/room/RoomList.java 79.16% <64.28%> (-7.94%) 25.00 <7.00> (+1.00) ⬇️
.../address/logic/commands/room/InitRoomsCommand.java 62.50% <100.00%> (ø) 5.00 <1.00> (?)
...edu/address/logic/parser/room/InitRoomsParser.java 100.00% <100.00%> (ø) 3.00 <1.00> (?)
...rc/main/java/seedu/address/model/ModelManager.java 98.14% <100.00%> (ø) 27.00 <0.00> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91fb05a...5ce9f07. Read the comment docs.

Copy link
Collaborator

@LeeMingDe LeeMingDe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, would really appreciate it if you can remove the convertPriorityQueue method for me

Noorul Azlina added 2 commits October 6, 2020 14:01
Add test cases, to increase testability of code.
Make changes to pass checkstyle, to improve code readability.
Copy link
Collaborator

@w-yeehong w-yeehong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your hard work! This is a major change, since the entity (Room) we are adding is so complex. I think there are some code quality nits we can discuss and address before we merge? Really good work though. You even went as far as to integrate Jackson for Room even though it's not needed for this part yet.

docs/UserGuide.md Show resolved Hide resolved
src/main/java/seedu/address/MainApp.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/MainApp.java Show resolved Hide resolved
src/main/java/seedu/address/commons/util/FileUtil.java Outdated Show resolved Hide resolved
public static final int DEFAULT_ROOM_NUMBER = 10;
public static final boolean DEFAULT_IS_OCCUPIED = true;
public static final Patient DEFAULT_PATIENT = TypicalPatients.ALICE;
public static final Task DEFAULT_TASK = null;
Copy link
Collaborator

@w-yeehong w-yeehong Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add this when I pull your changes 👍

src/test/java/seedu/address/testutil/RoomUtil.java Outdated Show resolved Hide resolved
Noorul Azlina and others added 8 commits October 6, 2020 15:46
Make changes as given by reviewers.
Make changes as given by reviewers.
Make changes as given by reviewers.
Make changes as given by reviewers.
…into add-noorul-roomStorage

* 'master' of https://github.com/AY2021S1-CS2103T-W12-1/tp:
  no message
  update Unit test: add success test for temperatureRange
  edit function name:searchPatientInvalidSearchCriteria
  update User Guide
  fix checkStyle
  fix search patient

# Conflicts:
#	docs/UserGuide.md
Make changes as suggested by reviewers.
@itssodium itssodium added this to the v1.2 milestone Oct 6, 2020
@itssodium itssodium linked an issue Oct 6, 2020 that may be closed by this pull request
Copy link

@chiamyunqing chiamyunqing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job and effort! Just some nitty gritty details. :)

@@ -158,6 +159,7 @@ Action | Format, Examples
--------|------------------
**Add Patient** | `addpatient n/NAME t/TEMPERATURE d/PERIOD_OF_STAY p/PHONE_NUMBER a/AGE [c/COMMENT]` <br> e.g.,`addpatient n/Betsy Crowe t/36.5 d/20201001-20201014 p/91234567 a/19 c/Is asthmatic`
**Edit Patient** | `editpatient NAME [n/NAME] [t/TEMPERATURE] [d/PERIOD_OF_STAY] [p/PHONE_NUMBER] [a/AGE] [c/COMMENT]`<br> e.g.,`editpatient James Lee t/36.5`
**Initialize Rooms** | `initRooms NUMBER_OF_ROOMS` <br> e.g., `addRooms 123`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be after search patient command but it's okay for now. We will do a massive UG clean up in future as a group :)

src/main/java/seedu/address/MainApp.java Show resolved Hide resolved
@@ -14,28 +13,39 @@
/**
* Contains information regarding the Room information
*/
public class RoomList {
public class RoomList implements ReadOnlyRoomList {
private static final Logger logger = LogsCenter.getLogger(JsonAddressBookStorage.class);

private int numOfRooms;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this too. There may be errors if we forget to update this variable when needed.

@itssodium itssodium removed the request for review from w-yeehong October 6, 2020 13:49
@chiamyunqing chiamyunqing linked an issue Oct 6, 2020 that may be closed by this pull request
Copy link
Collaborator

@w-yeehong w-yeehong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@itssodium itssodium merged commit a11a645 into AY2021S1-CS2103T-W12-1:master Oct 6, 2020
@w-yeehong w-yeehong added Type.Feature New feature or request Type.Enhancement A small tweak to a feature labels Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority.High Type.Enhancement A small tweak to a feature Type.Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RoomOccupancyStorageTest: files remain after completion of tests Fixed size array in RoomList
5 participants