Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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:

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.

This is the implementation, JsonConfiguration, which stores data in a 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 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.

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 okay, but maybe could be improved.
  4. Any comment? I've read how other languages/framework does it but...
share|improve this question
    
On your point 2, why can't load throw such exception? –  Ismael Miguel Jul 17 at 14:45
    
@IsmaelMiguel It can, maybe I should add this possibility too... –  Marco Acierno Jul 17 at 14:58
    
It is implied. If the save throws, I would expect the load to throw as well. –  Ismael Miguel Jul 17 at 15:02
1  
Well, the ConfigurationLoadFailedException have the cause (IOException) so you know what went wrong. With a ConfigurationLoadFailedException I will force the configuration programmer to do try {} catch(Exception e) { throw new ConfigurationLoadFailedException(e); } so the methods always fail with ConfigurationLoadFailedException and in the load settings code one can do catch (ConfigurationLoadFailedException e) { Alert("Ops");} to inform the user. If your point was that with a ConfigurationLoadFailedException one will lose information it won't happen –  Marco Acierno Jul 17 at 15:23
1  
@maaartinus You are right, if I want to make the Configuration interface as generic as possible I can't use IOException since the load could not require IO at all. –  Marco Acierno Jul 17 at 18:32

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.