johnding1996 / HKUST-COMP3111H-Group

Group Project of COMP 3111H in Fall 2017 at HKUST
Apache License 2.0
0 stars 0 forks source link

Review and propose changes of database package v1.1 #41

Closed johnding1996 closed 6 years ago

johnding1996 commented 6 years ago

Database package v1.1 is released recently, but, as excepted, there might be bugs and things to review and revise in it. This issue serves as the dialog to propose changes of v1.1. @WSGuo and I will update it to v1.2 very soon.

johnding1996 commented 6 years ago

Issue 1: querier.get method may cause exceptions when not found, and this is not even covered by unittests.

johnding1996 commented 6 years ago

Issue 2: change user_id to hash code of length 32 char(32).

johnding1996 commented 6 years ago

Issue 3: the set of states are changed, update this in StateKeeper class

johnding1996 commented 6 years ago

Issue 4: maybe reference the set of valid states inside the controller, to avoid duplicated definition of states in the whole project.

johnding1996 commented 6 years ago

Issue 5: Querer.add expect knowing the ids before adding it to the DB, but actually we do not know which id is occupied. Please use 100~200 currently

johnding1996 commented 6 years ago

Issue 6: some overload methods of add, update, delete could be implemented easily and good to use.

johnding1996 commented 6 years ago

Issue 7: some constructors are not public, should made default and useful constructor public

johnding1996 commented 6 years ago

All potential issue collected, will update database package and merge to develop tomorrow morning. The new version number will be v1.2

thomaszhouan commented 6 years ago

The visibility of constructors of querier classes is default, how to instantiate a querier in another package say agent?

johnding1996 commented 6 years ago

@thomaszhouan thanks for pointing out this, it already list in issue 7.

johnding1996 commented 6 years ago

@thomaszhouan As for Issue 4, I just found that stateNameSet is already defined in State class. Should I make it public to use it in the StateKeeper? A better method is to make a public get method.

thomaszhouan commented 6 years ago

For validation, you could call the public method validateStateName

johnding1996 commented 6 years ago

@thomaszhouan Great, sorry for not finding it before.

johnding1996 commented 6 years ago

Closed since database v1.2 is released