Branch house #61
Closed
Branch house #61
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
redcolorbicycle
requested review from
limcaaarl,
zengzihui,
felixchanyy and
KhoonSun47
limcaaarl
reviewed
Mar 18, 2024
Comment on lines
+23
to
+24
| import seedu.address.model.house.*; | ||
| import seedu.address.model.person.*; |
limcaaarl
reviewed
Mar 18, 2024
| @@ -21,6 +26,7 @@ | |||
| import seedu.address.logic.Messages; | |||
| import seedu.address.logic.commands.exceptions.CommandException; | |||
| import seedu.address.model.Model; | |||
| import seedu.address.model.house.*; | |||
limcaaarl
reviewed
Mar 18, 2024
Comment on lines
+38
to
+39
| boolean HASBLOCK = true; | ||
| boolean HASLEVEL = true; |
There was a problem hiding this comment.
avoid using ALL CAPS for non-constant variables
limcaaarl
reviewed
Mar 18, 2024
Comment on lines
+135
to
+138
| * Parses a {@code String street} into a {@code Street}. | ||
| * Leading and trailing whitespaces will be trimmed. | ||
| * | ||
| * @throws ParseException if the given {@code street} is invalid. |
limcaaarl
reviewed
Mar 18, 2024
| public class House { | ||
| public static final String MESSAGE_CONSTRAINTS = | ||
| "Housing types can only be HDB, Condominium or Landed."; | ||
| public static final String[] VALIDATION_REGEX = {"hdb", "condominium", "landed"}; |
There was a problem hiding this comment.
I think you can consider using enums for this one
limcaaarl
reviewed
Mar 18, 2024
Comment on lines
+18
to
+23
| /** | ||
| * Constructs a {@code Level}. | ||
| * | ||
| * @param postalCode The postal code of the house. | ||
| * @param street The street of the house. | ||
| */ |
There was a problem hiding this comment.
Wrong javadocs description here and incomplete params
limcaaarl
reviewed
Mar 18, 2024
Comment on lines
+24
to
+50
| public House(UnitNumber unitNumber, PostalCode postalCode, Street street) { | ||
| this.postalCode = postalCode; | ||
| this.street = street; | ||
| this.unitNumber = unitNumber; | ||
| } | ||
|
|
||
| public House(UnitNumber unitNumber, House house) { | ||
| this.unitNumber = unitNumber; | ||
| this.postalCode = house.getPostalCode(); | ||
| this.street = house.getStreet(); | ||
| } | ||
| public House(PostalCode postalCode, House house) { | ||
| this.unitNumber = house.getUnitNumber(); | ||
| this.postalCode = house.getPostalCode(); | ||
| this.street = house.getStreet(); | ||
| } | ||
| public House(Street street, House house) { | ||
| this.unitNumber = house.getUnitNumber(); | ||
| this.postalCode = house.getPostalCode(); | ||
| this.street = street; | ||
| } | ||
|
|
||
| public House(House house) { | ||
| this.unitNumber = house.getUnitNumber(); | ||
| this.postalCode = house.getPostalCode(); | ||
| this.street = house.getStreet(); | ||
| } |
There was a problem hiding this comment.
I think for the House constructor we just need these two params Postal and Street. Can refer to the rough sketch of our class diagram in telegram.
limcaaarl
reviewed
Mar 18, 2024
| * @param street The street of the house. | ||
| */ | ||
| public Landed(UnitNumber unitNumber, PostalCode postalCode, Street street) { | ||
| super(unitNumber, postalCode, street); |
There was a problem hiding this comment.
I think to show proper inheritance we can separate this out to something like:
from:
super(unitNumber, postalCode, street);
to
super(postalCode, street);
this.unitNumber = unitNumber;
limcaaarl
reviewed
Mar 18, 2024
|
|
||
|
|
||
| /** | ||
| * Constructs a {@code Level}. |
There was a problem hiding this comment.
Remember to change the javadocs description here too.
limcaaarl
reviewed
Mar 18, 2024
| * @param unitNumber The unit number of the house. | ||
| */ | ||
| public NonLanded(Block block, Level level, PostalCode postalCode, Street street, UnitNumber unitNumber) { | ||
| super(unitNumber, postalCode, street); |
There was a problem hiding this comment.
Can change to something like this super(postalCode, street);
The rest of the class variables looks good 👍
limcaaarl
reviewed
Mar 18, 2024
| * @param street The street of the house. | ||
| * @param unitNumber The unit number of the house. | ||
| */ | ||
| public NonLanded(Level level, PostalCode postalCode, Street street, UnitNumber unitNumber) { |
There was a problem hiding this comment.
What's this overloaded constructor for?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add House class and capabilities. Replace address