Reduce duplication between ConfigUtil and JsonUserPrefsStorage #95 #147
Conversation
|
|
||
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * Represents User's preferences. | ||
| */ | ||
| public class UserPrefs { | ||
| public class UserPrefs extends JsonFile { |
There was a problem hiding this comment.
UserPrefs should not extend JsonFile because UserPrefs should not know anything about the storage format. The duplication occurs because both are stored in the same format, and hence should be unified at storage level.
There was a problem hiding this comment.
however the current implementation saves a UserPrefs object to json rather than a GuiSettings object.
There was a problem hiding this comment.
ok, removed JsonFile.
|
|
||
| FileUtil.serializeObjectToJsonFile(new File(filePath), jsonFile); | ||
| } | ||
|
|
There was a problem hiding this comment.
These two methods are json specific, so move to a json specific class?
There was a problem hiding this comment.
moved all json specific methods to JsonUtil.
|
Some comments added. |
lejolly
force-pushed
the
95-Reduce-duplication-between-ConfigUtil-and-JsonUserPrefsStorage
branch
3 times, most recently
from
11e6837 to
088bb84
Compare
| @@ -5,9 +5,11 @@ | |||
| import java.nio.file.Files; | |||
|
|
|||
| /** | |||
| * Writes and reads file | |||
| * Writes and reads files, includes methods for reading and writing to | |||
| * preference/config json files | |||
There was a problem hiding this comment.
A class should not mention the call sites/context. i.e. preference/config
| public static <T> T deserializeObjectFromJsonFile(File jsonFile, Class<T> classOfObjectToDeserialize) | ||
| throws IOException { | ||
| return fromJsonString(FileUtil.readFromFile(jsonFile), classOfObjectToDeserialize); | ||
| } |
There was a problem hiding this comment.
Can these two methods be private? or at least package-private?
If they are used only once each, you can inline them.
There was a problem hiding this comment.
can't be private as JsonUtilTest uses them.
| File file = new File(filePath); | ||
|
|
||
| if (!file.exists()) { | ||
| logger.info("Config file " + file + " not found"); |
There was a problem hiding this comment.
Should not mention Config file
| * Similar to {@link #readUserPrefs()} | ||
| * @param prefsFilePath location of the data. Cannot be null. | ||
| * @throws DataConversionException if the file format is not as expected. | ||
| */ | ||
| public Optional<UserPrefs> readUserPrefs(String prefsFilePath) throws DataConversionException { |
There was a problem hiding this comment.
Header comment got deleted?
| @@ -14,13 +14,12 @@ | |||
| import seedu.address.logic.Logic; | |||
| import seedu.address.logic.LogicManager; | |||
| import seedu.address.model.*; | |||
| import seedu.address.commons.util.ConfigUtil; | |||
| import seedu.address.storage.ConfigStorage; | |||
There was a problem hiding this comment.
I'm reluctant to put this class in storage package because it's a more general utility the app should have even if there is no storage component.
| /** | ||
| * A class for accessing the Config File. | ||
| */ | ||
| public class ConfigStorage { |
There was a problem hiding this comment.
I don't mind if you name this ConfigUtil and put inside util package. As mentioned above, config is a util class rather than a class in the storage component.
| assert prefsFilePath != null; | ||
|
|
||
| FileUtil.serializeObjectToJsonFile(new File(prefsFilePath), userPrefs); | ||
| @Override |
There was a problem hiding this comment.
If this method doesn't have a header comment, then the parent method should have one.
There was a problem hiding this comment.
the comments are in the interface UserPrefsStorage for that method.
| /** | ||
| * Tests JSON Read and Write | ||
| */ | ||
| public class JsonUtilTest { | ||
|
|
||
| private static final File SERIALIZATION_FILE = new File(TestUtil.getFilePathInSandboxFolder("serialize.json")); |
There was a problem hiding this comment.
Use JUnit Temp Folder @Rule?
There was a problem hiding this comment.
don't think it works here, as the specified file actually exists.
|
|
||
| thrown.expect(DataConversionException.class); | ||
| read("NotJasonFormatConfig.json"); | ||
| read("NotJsonFormatConfig.json"); |
lejolly
force-pushed
the
95-Reduce-duplication-between-ConfigUtil-and-JsonUserPrefsStorage
branch
from
088bb84 to
0797764
Compare
lejolly
force-pushed
the
95-Reduce-duplication-between-ConfigUtil-and-JsonUserPrefsStorage
branch
from
0797764 to
79e960c
Compare
damithc
deleted the
95-Reduce-duplication-between-ConfigUtil-and-JsonUserPrefsStorage
branch
Fixed Checkstyle errors
Fix bug that caused data files to not load when starting the Application
GUI improvements for mid-v1.3
Fixes #95
The tests are hard to merge even though they do roughly the same thing because each is specialised for that particular json file format (along with their respective classes, etc).