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

Added primary key to Person as our first internal state field to track and save #80

Merged
merged 8 commits into from Oct 13, 2019

Conversation

podocarp
Copy link
Member

@podocarp podocarp commented Oct 11, 2019

  • Created new InternalState* classes to track and store the internal state of the program
  • Examples of internal states:
    • Command history
    • Primary keys of other thingawumuts
    • etc.

The UserPrefs now store one more path, that is the path of the internal state save file. By default it is data/state.json.

The only things that need to be touched when adding more fields in is the {update, apply}InternalState functions. Be sure to update them!

… track of, and implemented save functionality for it as well.
@podocarp podocarp modified the milestones: Sprint 3, v1.2 Oct 12, 2019
@podocarp podocarp changed the title Added primary key to Person as our first internal state field to keep… Added primary key to Person as our first internal state field to track and save Oct 12, 2019
Copy link

@daekoon daekoon left a comment

Choose a reason for hiding this comment

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

Mostly ok, but we should at least do this before merging:

  • Update test helper classes PersonBuilder. Add in a method to set primaryKey.
  • Change the naming of the primaryKeyCounter to be more specific towards Person class.

These should be future sprint enhancements:

  • Tests for InternalStorage and related classes (low priority)
  • Validation to check if primaryKeyCounter for InternalStorage is valid upon file load. (Higher priority)

@podocarp
Copy link
Member Author

* Update test helper classes `PersonBuilder`. Add in a method to set `primaryKey`.

primarykey being exposed is quite contentious. Some points about the behavior now:

  • Person.equals does not care about primary key.
  • PersonBuilder constructor can be made to copy the primary key.
  • PersonBuilder.build() calls a constructor of Person which does not care about the primary key.

So it is quite messy. I will add some tests for other behavior, but I don't think I will ever want to change the equals behavior. For sure the primary key will be different, what's there to check (it's validated on start)? And so it follows that if equals behaves like this, the other functions can do anything they want, but nothing will change. e.g. PersonBuilder.build() can use any constructor it wants and nothing will change.

* Tests for `InternalStorage` and related classes (low priority)

Done

* Validation to check if `primaryKeyCounter` for `InternalStorage` is valid upon file load. (Higher priority)

Done

Copy link

@daekoon daekoon 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 for catching those edge cases regarding .equal method for Person class. Should document it in the Developer guide in the future.
Personally I think your equal method is ok - it should be comparing the content of the Person object, and I would argue that primaryKey isn't really a content , but an identity of that object and hence shouldn't be used for .equal comparison (similar to how hashcode doesn't get compared)

Copy link

@liakify liakify left a comment

Choose a reason for hiding this comment

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

Looks good to me

@liakify liakify merged commit 1ec591f into master Oct 13, 2019
@liakify liakify deleted the save-internal-state branch October 13, 2019 15:32
@liakify liakify restored the save-internal-state branch October 13, 2019 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants