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

Massive Update for storage model #54

Merged
merged 10 commits into from
Mar 3, 2019

Conversation

ToonDragon
Copy link

Added 3 files to handle source storage function.

Source object will be the primary type for storing source information. Contains the following fields. More can be added as necessary.

  1. Source Title (compulsory field)
  2. Source Type (compulsory field)
  3. Source Details (compulsory field)
  4. Source Tags (can contain 0 or more tags)

Functions should be able to accomplish the following:

  1. Add a new source to the database of sources
  2. Delete sources from the database by index
  3. Mass delete all sources from the database
  4. Load existing database from an external txt file
  5. Write current database to external txt file
  6. Edit the each of the parameters in each source individually and by index

#51

ToonDragon and others added 4 commits February 25, 2019 16:43
Merge from Main 25/2/2019 4.57pm
Added 3 files to handle source storage function.

Source object will be the primary type for storing source information. Contains the following fields. More can be added as necessary.
1) Source Title (compulsory field)
2) Source Type (compulsory field)
3) Source Details (compulsory field)
4) Source Tags (can contain 0 or more tags)

Functions should be able to accomplish the following:
1) Add a new source to the database of sources
2) Delete sources from the database by index
3) Mass delete all sources from the database
4) Load existing database from an external txt file
5) Write current database to external txt file
6) Edit the each of the parameters in each source individually and by index
@ToonDragon ToonDragon added this to the v1.1 milestone Mar 2, 2019
@ToonDragon
Copy link
Author

Pending review

Copy link

@case141 case141 left a comment

Choose a reason for hiding this comment

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

I think we should create it similar to how the persons model is created and not just one whole sourcemodel. Because this may cause problems in the future code. You should add the individual title/tag/type/detail classes. Should be done as soon as possible in future versions.

@ToonDragon
Copy link
Author

Modified the Java Docs accordingly and a few minor changes

@@ -6,16 +6,16 @@
* Represents the model of a source.
*/
public class Source {
/** Title of the source as detailed by the user, can be anything **/
// Title of the source as detailed by the user, can be anything
Copy link

Choose a reason for hiding this comment

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

You don't need to comment if your variable name is descriptive, e.g. sourceTitle is obvious enough

private String sourceTitle;

/** Type of the source, defined by the user but should follow some convention like "Book" or "Website" **/
// Type of the source, defined by the user but should follow some convention like "Book" or "Website"
private String sourceType;
Copy link

Choose a reason for hiding this comment

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

This should be an enumeration

private String sourceDetails;

/** List of tags for this source, while user defined, should follow some conventions to avoid duplicates **/
// List of tags for this source, while user defined, should follow some conventions to avoid duplicates
Copy link

Choose a reason for hiding this comment

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

Why not use a HashSet to avoid duplicates?

Also you might wanna consider canonicalizing them, e.g. strip whitespaces and lowercase them, so you won't end up with e.g. "Biology" and "biology".

@@ -25,7 +25,9 @@ public Source (String sourceTitle, String sourceType, String sourceDetails, Arra
this.sourceTags = sourceTags;
}

/** Public get functions for all the details of the source, to be used for retrieving information **/
/**
Copy link

Choose a reason for hiding this comment

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

Not 100% sure about this, but I'm pretty sure (99%) this gets interpreted as javadoc comment for getSourceTitle(), so if you try to use getSourceTitle() IntelliJ will pop up with the description "Public get functions for all the details of the source, to be used for retrieving information".

Have you tried this?

@@ -39,7 +41,9 @@ public String getSourceDetails() {
return this.sourceTags;
}

/** Public set functions for all the details of the source, to be used for editing information **/
/**
Copy link

Choose a reason for hiding this comment

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

Same

public SourceDatabase() {
this.database = SourceStorageOperations.loadExistingDatabase();
}

// Functions to manipulate source objects in the database
/**
Copy link

Choose a reason for hiding this comment

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

Same as prev

/**
* Constructor for the main database object, will automatically load any existing entries in the database file
* The database will be empty if there is nothing in the existing file to load
*/
public SourceDatabase() {
this.database = SourceStorageOperations.loadExistingDatabase();
Copy link

Choose a reason for hiding this comment

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

You forgot to remove this this. :P

}
public void editSourceTitleAtIndex (int index, String newTitle) {
this.database.get(index).setSourceTitle(newTitle);
database.get(index).setSourceTitle(newTitle);
Copy link

Choose a reason for hiding this comment

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

Eh, database.get(index) potentially throws IndexOutOfBoundsException. I think you should wrap it in a try catch clause to fail gracefully (maybe log it and feedback to user), otherwise the whole program will just crash :P

@ToonDragon ToonDragon merged commit 33390d6 into CS2103-AY1819S2-W13-3:master Mar 3, 2019
ToonDragon added a commit to ToonDragon/main that referenced this pull request Mar 5, 2019
@case141 case141 removed this from the v1.1 milestone Mar 10, 2019
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