McGill-ECSE321-Winter2022 / project-group-group-05

project-group-group-05 created by GitHub Classroom
3 stars 3 forks source link

Domain Model and Primary Keys #28

Closed jimcv closed 2 years ago

jimcv commented 2 years ago

(Final) Domain Model for deliverable 1

154190388-c0d72cc3-0886-45f2-bd07-27ed9ddd4773

jimcv commented 2 years ago

Primary Keys

For EmployeeSchedule, Purchase, SpecificItem, we need to add a new attribute id: long to the model

@HwangPHS @PeiniCheng @frankie453 @oliverqueen1009

jimcv commented 2 years ago

Additionally, I feel like we can just delete the GroceryStore class altogether. The compositions will take alot of space in the database (one more table for each composition), and they don't seem useful at all because we can already navigate to any object we want using CrudRepository. Another reason why they're redundant is because compositions are unidirectional too, so we can only navigate down from GroceryStore, not up, which defeats the entire purpose of having that big class that contains everything else.

In the tutorial, RegistrationManager was never mentioned again after showing up in the domain model. They didn't even implement a Repository for it.

jimcv commented 2 years ago

Can we also have an additional attribute dateOfPurchase: Date for the Purchase class? I think it would be useful info to keep record of as well as for sorting purposes.

PeiniCheng commented 2 years ago

Adding purchase date sounds good to me. But don't we need a GroceryStore instance to start the application? Cause in 223 we basically need a ClimbsSafe instance for everything.

jimcv commented 2 years ago

don't we need a GroceryStore instance to start the application?

We don't, seems like it's ran by spring boot. In 223, when testing, we did have to use a ClimbSafe object. But here notice in the tutorial there was no such thing.

Here instead we need a CrudRepository for everything.

jimcv commented 2 years ago

I just noticed a new problem... If the owner decides to delete a particular item (e.g. the store doesn't sell it anymore), what do we do with the specificItems associated with it?

Since we're keeping them as a record in a users purchase history, we do not want to delete them. However, because of the ManyToOne association they have with the item, they're the dependent object and should actually be deleted first before deleting the item itself (see tutorial, JUnit testing, Registration has to be deleted before Person and Event because of ManyToOne)

Edit: moved to #35

HwangPHS commented 2 years ago
HwangPHS commented 2 years ago

So to update the Class Diagram @PeiniCheng:

HwangPHS commented 2 years ago

A few more things about the domain model: There is a java.sql.Time class that we could potentially use to store the value for startTime and endTime in the Shift Class. They used it in the tutorial for the Event Class, maybe we should change it?

Do you think it's worth attaching an enum to the Employee Class defining what kind of employee it is? I feel like a cashier should have a separate interface from a stocker for example.

@jimcv @PeiniCheng

jimcv commented 2 years ago

A few more things about the domain model: There is a java.sql.Time class that we could potentially use to store the value for startTime and endTime in the Shift Class.

Yeah agreed. It also applies to the OpeningHours class.

Do you think it's worth attaching an enum to the Employee Class defining what kind of employee it is? I feel like a cashier should have a separate interface from a stocker for example.

Prof mentioned today during lecture to not specify what's an employee's exact task, it becomes too complicated. (Thus it should be fine to provide all employees the same interface)

jimcv commented 2 years ago

Can we also have an additional attribute dateOfPurchase: Date for the Purchase class? I think it would be useful info to keep record of as well as for sorting purposes.

I forgot we should also add timeOfPurchase: Time for the Purchase class for the sorting to work well. We don't necessarily need to display it but it's just an internal thing since Date drops the hour/minute/seconds info when created.

Nevermind, I just had a way better idea. We only need one attribute timeOfPurchaseMillis: long. This way both Date and Time are being kept in one single column of the database and we can do the conversions outside of the database (in getters) just by using the constructors from Date(millis) and Time(millis). It's way better to sort this way. (p.s. this is how i implemented the current time display on our heroku welcome page right now)

TLDR @PeiniCheng, for the Purchase class, the only extra attribute we'll add to keep track of the Date and Time is timeOfPurchaseMillis: long (forget about dateOfPurchase: Date, timeOfPurchase: Time)

PeiniCheng commented 2 years ago

Do you mean for example convert 12:00 December 10 2022 to something like 202212101200? (Not very sure how it's implemented this is just a guess)

HwangPHS commented 2 years ago

Do you mean for example convert 12:00 December 10 2022 to something like 202212101200? (Not very sure how it's implemented this is just a guess)

Not quite, the idea of keeping track of time with "millis" is that you just have a super long decimal value, like 105731059135 or something. And that simply represents the number of milliseconds that have passed since Jan 1st, 1970 (at least, this is my understanding of the Date/Time format). We get methods that will automatically convert that millisecond time into Date and Time instances, which lets us convert that time value to something readable by humans without wanting to cry

jimcv commented 2 years ago

Do you mean for example convert 12:00 December 10 2022 to something like 202212101200? (Not very sure how it's implemented this is just a guess)

No, it's not up to us to do the conversion.

Harrison is correct. Basically, the method in Java that let us get current system returns a value of type 'long'. We're going to store this long value in the database. In the getters, I will use constructors from java.sql.Date and java.sql.Time to return what the controllers would request (because they both take that 'long' value as parameter). Storing one 'long' in the database is simply more space and time efficient. It also makes sorting way easier.

the setter

image

the getters

image

PeiniCheng commented 2 years ago

update Class Diagram drawio (6)

jimcv commented 2 years ago

@PeiniCheng timeOfPurchaseMillis: long should be in Purchase, not SpecificItem. And this image

Otherwise looks good!

PeiniCheng commented 2 years ago

fixed Class Diagram drawio (7)

jimcv commented 2 years ago

image it's super weird but there's still this little multiplicity thing there. (Is it only showing on my side or?)

PeiniCheng commented 2 years ago

My bad Class Diagram drawio (8)

jimcv commented 2 years ago

Domain model seems good for now.

jimcv commented 2 years ago

Reopen for reference

HwangPHS commented 2 years ago

change "isAvailable" to "isDiscontinued"

PeiniCheng commented 2 years ago

update Class Diagram drawio (10)

jimcv commented 2 years ago

Great! Updated in top comment as well.

HwangPHS commented 2 years ago

@jimcv @PeiniCheng So one of the outlined requirements in the project description is "customers with local addresses get free delivery, but customers outside the town borders must pay for delivery service". We have a boolean "isLocal" in the customer to describe whether or they're local, so we can decide whether they need to pay the delivery fee. But we never really store how much that delivery fee is. I can't believe we missed this, but there is no good place to store this value, that I know of. Furthermore, presumably the owner should be able to change this value.

We could store it as a static value in the Purchase class - probably works extremely poorly with databases? I honestly don't know how static values are parsed when it comes to databases. Even then, static values aren't really even touched on when it comes to domain models.

We could store it in the Owner class - this is probably going to be considered bad. The value is a delivery fee, it has to do with the system, not the Owner. But even then, this is the closest I can get, since it certainly doesn't belong anywhere else in the system.

Maybe we could calculate the fee based off the customer's address using an algorithm in the controller? - This is probably going to lead to a whole bunch of headaches down the line in terms of how exactly such an algorithm would work.

Maybe we could add a double deliveryFee to the Purchase class? - This would mean that the deliveryFee is set per-item. I can already imagine this being considered bad practice as well, since if the Owner wanted to set a static deliveryFee for the entire store, they'd have to set it for every single item there.

I honestly can't think of an effortless way of solving this problem cleanly. Let me know what you guys think

HwangPHS commented 2 years ago

To add on to what I wrote above, our system requirements do not cover these lines in the project description: "customers with local addresses get free delivery, but customers outside the town borders must pay for delivery service. There is no service fee for pickup" "the owner has all the privileges of an employee"

That having been said, the grading scheme doesn't actually have much of a section for "requirements are properly prioritized" so I'm not sure I care, just thought I'd mention it.

jimcv commented 2 years ago

We didn't have to absolutely cover everything in the requirements, only the 15 we think matters the most.

As for the delivery fee I think we'll need to ask the prof what he actually expects. Should it be a customizable amount, or fixed. Because irl the system will need to communicate with the delivery provider to get() an amount (which again is not stored in the GSS itself)

HwangPHS commented 2 years ago

I think we're good to close this issue, shouldn't be anything else to do with the domain model

PeiniCheng commented 2 years ago

update Class Diagram drawio (11)

jimcv commented 2 years ago

Added to wiki