Skip to main content
We’ve updated our Terms of Service. A new AI Addendum clarifies how Stack Overflow utilizes AI interactions.
added 2 characters in body; edited tags
Source Link
RubberDuck
  • 31.2k
  • 6
  • 74
  • 177
  1. As I said, the concepts save/load and read are mixed together in one interface, make sense separate them and implement a Factory to load and save settings?
  2. I want to remove the throws IOException from save because it's not consistent with load which doesn't throw IOException.
  3. Can I improve the load of JsonConfigurationSection? The ifsifs seems okay, but maybe could be improved.
  4. Any comment? I've read how other languages/framework does it but...
  1. As I said, the concepts save/load and read are mixed together in one interface, make sense separate them and implement a Factory to load and save settings?
  2. I want to remove the throws IOException from save because it's not consistent with load which doesn't throw IOException.
  3. Can I improve the load of JsonConfigurationSection? The ifs seems okay, but maybe could be improved.
  4. Any comment? I've read how other languages/framework does it but...
  1. As I said, the concepts save/load and read are mixed together in one interface, make sense separate them and implement a Factory to load and save settings?
  2. I want to remove the throws IOException from save because it's not consistent with load which doesn't throw IOException.
  3. Can I improve the load of JsonConfigurationSection? The ifs seems okay, but maybe could be improved.
  4. Any comment? I've read how other languages/framework does it but...
Tweeted twitter.com/#!/StackCodeReview/status/624039347763970049
Spelling.
Source Link
ferada
  • 11.4k
  • 26
  • 66

I didcreated an API to load, save and read user configuration in my application. I've a Configuration interface which provides the basic methods to read, save and read configuration data and then in the application I've the implementation JsonConfiguration.

I tried to keep the Configuration interface as generic as possible. So I didn't make any assumption about how the settings are stored, it can be plain text as well as something else.

It'sThis is my Configuration interface:

(I've removed documentation from getAsX since it's the same.) (Yes, I know there is no getAsFloat, I will add it after the review.)

As you can see, the concept of save/load and read are mixed in the same interface. My first question is, should I move the two concepts in two Interfacesinterfaces? Maybe using a Factory to load and save the settings and Configuration will only handle the reading of the data.

It'sThis is the implementation, JsonConfiguration, which stores data in a JSON format:

The interface ConfigurationSection is without documentation:

and the implementation:

As you can see from load everything is stored as StringString in the map (so yes, values can be changed from <String, Object> to <String, String> but my plan is to support arrays and objects soon... so).

My application has a Application interface with a getConfiguration method so Plugins/Application use it to access Configurationa Configuration object and read settings. The fact that the concept of save/load is of the Boot class only (which prepare/load etc the application) it could be an incentive to use a Factoryfactory instead of the current interface (so I hide the two things).

I'm planning to add a Transformer concept to convert a configuration format to another, but it's just an idea for now.

  1. As I said, the concepts save/load and read are mixed together in one interface, make sense separate them and implement a Factory to load and save settings?
  2. I want to remove the throws IOException from save because it's not consistent with load which doesn't throw IOException.
  3. Can I improve the load of JsonConfigurationSection? The ifs seems OKokay, but maybe could be improved.
  4. Any comment? I've read how other languages/framework does it but...

I did an API to load, save and read user configuration in my application. I've a Configuration interface which provides the basic methods to read, save and read configuration data and then in the application I've the implementation JsonConfiguration.

I tried to keep the Configuration interface as generic as possible. So I didn't make any assumption about how the settings are stored it can be plain text as well as something else.

It's my Configuration interface

(I've removed documentation from getAsX since it's the same) (Yes, I know there is no getAsFloat, I will add it after the review)

As you can see, the concept of save/load and read are mixed in the same interface. My first question is, should I move the two concepts in two Interfaces? Maybe using a Factory to load and save the settings and Configuration will only handle the reading of the data.

It's the implementation, JsonConfiguration which stores data in JSON format

The interface ConfigurationSection is without documentation

and the implementation

As you can see from load everything is stored as String in the map (so yes, values can be changed from <String, Object> to <String, String> but my plan is to support arrays and objects soon... so).

My application has a Application interface with a getConfiguration method so Plugins/Application use it to access Configuration object and read settings. The fact that the concept of save/load is of the Boot class only (which prepare/load etc the application) it could be an incentive to use a Factory instead of the current interface (so I hide the two things).

I'm planning to add a Transformer concept to convert a configuration format to another, but it's just an idea for now

  1. As I said, the concepts save/load and read are mixed together in one interface, make sense separate them and implement a Factory to load and save settings?
  2. I want to remove the throws IOException from save because it's not consistent with load which doesn't throw IOException
  3. Can I improve the load of JsonConfigurationSection? The ifs seems OK but maybe could be improved.
  4. Any comment? I've read how other languages/framework does it but...

I created an API to load, save and read user configuration in my application. I've a Configuration interface which provides the basic methods to read, save and read configuration data and then in the application I've the implementation JsonConfiguration.

I tried to keep the Configuration interface as generic as possible. So I didn't make any assumption about how the settings are stored, it can be plain text as well as something else.

This is my Configuration interface:

(I've removed documentation from getAsX since it's the same.) (Yes, I know there is no getAsFloat, I will add it after the review.)

As you can see, the concept of save/load and read are mixed in the same interface. My first question is, should I move the two concepts in two interfaces? Maybe using a Factory to load and save the settings and Configuration will only handle the reading of the data.

This is the implementation, JsonConfiguration, which stores data in a JSON format:

The interface ConfigurationSection is without documentation:

and the implementation:

As you can see from load everything is stored as String in the map (so yes, values can be changed from <String, Object> to <String, String> but my plan is to support arrays and objects soon... so).

My application has a Application interface with a getConfiguration method so Plugins/Application use it to access a Configuration object and read settings. The fact that the concept of save/load is of the Boot class only (which prepare/load etc the application) it could be an incentive to use a factory instead of the current interface (so I hide the two things).

I'm planning to add a Transformer concept to convert a configuration format to another, but it's just an idea for now.

  1. As I said, the concepts save/load and read are mixed together in one interface, make sense separate them and implement a Factory to load and save settings?
  2. I want to remove the throws IOException from save because it's not consistent with load which doesn't throw IOException.
  3. Can I improve the load of JsonConfigurationSection? The ifs seems okay, but maybe could be improved.
  4. Any comment? I've read how other languages/framework does it but...
Source Link
Marco Acierno
  • 2.2k
  • 16
  • 18

Configuration concept and implementation

I did an API to load, save and read user configuration in my application. I've a Configuration interface which provides the basic methods to read, save and read configuration data and then in the application I've the implementation JsonConfiguration.

I tried to keep the Configuration interface as generic as possible. So I didn't make any assumption about how the settings are stored it can be plain text as well as something else.

It's my Configuration interface

public interface Configuration {
  /**
   * Called when the configuration should load the settings
   *
   * @param inputStream In this stream you can found the settings of
   *                    the user.
   *                    An implementation can decide what is inside this
   *                    stream.
   *                    Example: If an implementation
   *                    uses JSON to save and read settings, it should
   *                    assume that this stream contains a valid json.
   *                    If it doesn't, it is allowed to throw
   *                    any exception since it's not
   *                    their job to convert the stream to your
   *                    format.
   *                    Do not close the stream. You don't own it.
   */
  void load(@NotNull final InputStream inputStream);

  /**
   * Called when the configuration should save ALL the settings
   *
   * @param outputStream Write in this stream the settings of the
   *                     user in the form of the implementation.
   *                     Do not close the stream. You don't own it.
   *
   * @throws IOException Throw if something went wrong during the
   *                     save process
   */
  void save(@NotNull final OutputStream outputStream) throws IOException;

  /**
   * @param name The setting name
   * @return The setting as integer (if it's an integer)
   *         or NumberFormatException if not
   */
  @NotNull
  OptionalInt getAsInt(@NotNull final String name);

  @NotNull
  OptionalDouble getAsDouble(@NotNull final String name);

  @NotNull
  OptionalLong getAsLong(@NotNull final String name);

  @NotNull
  Optional<String> getAsString(@NotNull final String name);

  @NotNull
  Optional<Boolean> getAsBoolean(@NotNull final String name);

  /**
   * Set the value
   *
   * @param name The name of the setting
   * @param value The value to save
   * @param <T> The type of the value
   */
  <T> void set(@NotNull final String name, @NotNull final T value);
}

(I've removed documentation from getAsX since it's the same) (Yes, I know there is no getAsFloat, I will add it after the review)

As you can see, the concept of save/load and read are mixed in the same interface. My first question is, should I move the two concepts in two Interfaces? Maybe using a Factory to load and save the settings and Configuration will only handle the reading of the data.

It's the implementation, JsonConfiguration which stores data in JSON format

public class JsonConfiguration implements Configuration {
  private Map<String, ConfigurationSection> configurations = new HashMap<>();

  @Override
  public void load(@NotNull final InputStream inputStream) {
    final JsonParser parser = new JsonParser();
    final JsonObject root = parser.parse(new InputStreamReader(inputStream)).getAsJsonObject();

    for (final Map.Entry<String, JsonElement> entry : root.entrySet()) {
      final String confName = entry.getKey();

      final ConfigurationSection configuration = new JsonConfigurationSection();
      configuration.load(new ByteArrayInputStream(
              entry
                  .getValue()
                  .toString()
                  .getBytes(StandardCharsets.UTF_8))
      );

      configurations.put(confName, configuration);
    }
  }

  @Override
  public void save(@NotNull final OutputStream outputStream) throws IOException {
    final Type type = new TypeToken<Map<String, Configuration>>() {}.getType();
    final OutputStreamWriter out = new OutputStreamWriter(outputStream);
    final JsonWriter writer = new JsonWriter(out);

    new GsonBuilder()
        .registerTypeAdapter(JsonConfigurationSection.class, new JsonConfigurationSectionSerializer())
        .create()
        .toJson(configurations, type, writer);

    writer.flush();
    out.flush();
  }

  @NotNull
  private <T> Optional<T> get(@NotNull final String name) {
    final int dotSeparatorPosition = name.indexOf('.');

    if (dotSeparatorPosition == -1) {
      throw new IllegalArgumentException(name + " is not a correct user preference setting name");
    }

    final String section = name.substring(0, dotSeparatorPosition);
    final String element = name.substring(dotSeparatorPosition + 1);

    if (!configurations.containsKey(section)) {
      throw new IllegalArgumentException("Section " + section + " doesn't exists.");
    }

    return configurations.get(section).get(element);
  }

  @NotNull
  @Override
  public OptionalInt getAsInt(@NotNull final String name) {
    final Optional<String> optional = get(name);
    return !optional.isPresent() ? OptionalInt.empty() : OptionalInt.of(Integer.parseInt(optional.get()));
  }

  @NotNull
  @Override
  public OptionalDouble getAsDouble(@NotNull final String name) {
    final Optional<String> optional = get(name);
    return !optional.isPresent() ? OptionalDouble.empty() : OptionalDouble.of(Double.parseDouble(optional.get()));
  }

  @NotNull
  @Override
  public OptionalLong getAsLong(@NotNull final String name) {
    final Optional<String> optional = get(name);
    return !optional.isPresent() ? OptionalLong.empty() : OptionalLong.of(Long.parseLong(optional.get()));
  }

  @NotNull
  @Override
  public Optional<String> getAsString(@NotNull final String name) {
    return get(name);
  }

  @NotNull
  @Override
  public Optional<Boolean> getAsBoolean(@NotNull final String name) {
    final Optional<String> optional = get(name);
    return !optional.isPresent() ? Optional.empty() : Optional.of(Boolean.parseBoolean(optional.get()));
  }

  /**
   * Sets the value of a section. The name
   * is composed of: sectionName.entryName
   *
   * If the section doesn't exists it will
   * be created.
   *
   * If the entryName doesn't exists it will
   * be created.
   *
   * @param name The name of the setting
   * @param value The value to save
   * @param <T> The type of the value
   * @throws IllegalArgumentException If the name passed is not of the format: sectionName.entryName
   */
  @Override
  public <T> void set(@NotNull final String name,
                      @NotNull final T value) {
    final int dotSeparatorPosition = name.indexOf('.');

    if (dotSeparatorPosition == -1) {
      throw new IllegalArgumentException(name + " is not a correct user preference setting name");
    }

    final String sectionName = name.substring(0, dotSeparatorPosition);
    final String element = name.substring(dotSeparatorPosition + 1);

    ConfigurationSection section = configurations.get(sectionName);

    if (section == null) {
      section = new JsonConfigurationSection();

      configurations.put(sectionName, section);
    }

    section.set(element, value.toString());
  }
}

Since I don't make any assumption in the Configuration interface, the concept of "sections" exists only in the implementation and the user access an entry in a section using the notation sectionName.entryName.

The interface ConfigurationSection is without documentation

public interface ConfigurationSection {
  void load(@NotNull final InputStream inputStream);
  void save(@NotNull final OutputStream outputStream) throws IOException;
  <T> Optional<T> get(@NotNull final String name);
  <T> void set(@NotNull final String name, @NotNull final T value);
}

and the implementation

public class JsonConfigurationSection implements ConfigurationSection {
  @NotNull
  private ConcurrentMap<String, Object> values = new ConcurrentHashMap<>();

  @Override
  public void load(@NotNull final InputStream inputStream) {
    values = new ConcurrentHashMap<>();

    final JsonParser parser = new JsonParser();
    final JsonObject root = parser.parse(new InputStreamReader(inputStream)).getAsJsonObject();

    for (final Map.Entry<String, JsonElement> entry : root.entrySet()) {
      final String key = entry.getKey();
      final JsonElement value = entry.getValue();

      if (value.isJsonPrimitive()) {
        final JsonPrimitive primitive = value.getAsJsonPrimitive();
        values.put(key, primitive.getAsString());
      } else if (value.isJsonNull()) {
        throw new IllegalArgumentException("null is not a valid parameter");
      } else if (value.isJsonArray()) {
        throw new UnsupportedOperationException("Arrays not supported yet");
      } else if (value.isJsonObject()) {
        throw new UnsupportedOperationException("Objects not supported yet");
      }
    }
  }

  @Override
  public void save(@NotNull final OutputStream outputStream) throws IOException {
    final Type type = new TypeToken<Map<String, Object>>() {}.getType();
    final OutputStreamWriter out = new OutputStreamWriter(outputStream, StandardCharsets.UTF_8);
    final JsonWriter writer = new JsonWriter(out);

    new Gson().toJson(values, type, writer);

    writer.flush();
    out.flush();
  }

  @NotNull
  @Override
  public <T> Optional<T> get(@NotNull final String name) {
    return Optional.ofNullable((T) values.get(name));
  }

  @Override
  public <T> void set(@NotNull final String name,
                      @Nullable final T value) {
    values.put(name, value);
  }

  @NotNull
  Map<String, Object> getValues() {
    return Collections.unmodifiableMap(values);
  }
}

As you can see from load everything is stored as String in the map (so yes, values can be changed from <String, Object> to <String, String> but my plan is to support arrays and objects soon... so).

ConfigurationSection interface and Configuration are pretty similar as interfaces, but have different concepts.

My application has a Application interface with a getConfiguration method so Plugins/Application use it to access Configuration object and read settings. The fact that the concept of save/load is of the Boot class only (which prepare/load etc the application) it could be an incentive to use a Factory instead of the current interface (so I hide the two things).

I'm planning to add a Transformer concept to convert a configuration format to another, but it's just an idea for now

My questions:

  1. As I said, the concepts save/load and read are mixed together in one interface, make sense separate them and implement a Factory to load and save settings?
  2. I want to remove the throws IOException from save because it's not consistent with load which doesn't throw IOException
  3. Can I improve the load of JsonConfigurationSection? The ifs seems OK but maybe could be improved.
  4. Any comment? I've read how other languages/framework does it but...