3
\$\begingroup\$

Setting manager class for reading and managing Settings.ini file

I have started a project creating Software Installer application for auto download and auto install software, similar how Ninite works. I have created SettingsManager.java class for reading and managing Settings.ini file of the program so the rest of the program can use it. This is just a draft of the class an it is not complete, but it is functional. I would like your opinions on the overall design and implementation of this kind of class.

The SettingsManager.java class

package com.zoran_jankov.software_installer.app;

import java.io.File;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;

import org.ini4j.Ini;

public class SettingsManager
{
    private static SettingsManager instance;
    
    private static final String SETTINGS_FILE_PATH = "D:/Programing/Java/Software Installer/src/main/resources/Settings.ini";
    
    private Map<Software, String> localRepositories = new HashMap<Software, String>();
    private Map<Software, String> networkRepositories = new HashMap<Software, String>();
    private Map<Software, String> onlineRepositories = new HashMap<Software, String>();
    private Map<Software, String> arguments = new HashMap<Software, String>();
    
    private SettingsManager()
    {
        loadSettings();
    }
    
    public static SettingsManager getInstance()
    {
        if(instance == null)
        {
            instance = new SettingsManager();
        }
        return instance;
    }
    
    public void loadSettings()
    {
        Ini settings = null;
        
        try
        {
            settings = new Ini(new File(SETTINGS_FILE_PATH));
        }
        catch (IOException e)
        {
            e.printStackTrace();
        }
        
        localRepositories = getInstallerSettings(settings, "Local Repositories");
            
        networkRepositories = getInstallerSettings(settings, "Network Repositories");
                
        onlineRepositories = getInstallerSettings(settings, "Online Repositories");
                
        arguments = getInstallerSettings(settings, "Arguments");

    }
    
    private Map<Software, String> getInstallerSettings(Ini setttings, String sectionName)
    {
        Map<Software, String> list = new HashMap<Software, String>();
        
        for (Software installer : Software.values())
        {
            list.put(installer, setttings.get(sectionName, installer.name()));
        }
        
        return list;
    }
    
    public String getLocalRepository(Software software)
    {
        return localRepositories.get(software);
    }
    
    public String getNetworkRepository(Software software)
    {
        return networkRepositories.get(software);
    }
    
    public String getOnlineRepository(Software software)
    {
        return onlineRepositories.get(software);
    }
    
    public String getArguments(Software software)
    {
        return arguments.get(software);
    }
}

The Software.java enum

package com.zoran_jankov.software_installer.app;

public enum Software
{
    AIMP,
    CCleaner,
    DWGSee,
    Firefox,
    FoxitReader,
    ImgBurn,
    Java,
    LibreOffice,
    Notepad,
    Office365,
    OpenOffice,
    RocketChat,
    Skype,
    SpacePlanning,
    Speccy,
    Thunderbird,
    UltraVNC,
    VLC,
    ZabbixAgent,
    Zip;
}

The Settings.ini file

So you can better understand what is happening in the code.

\$\endgroup\$
  • 3
    \$\begingroup\$ I suggest not reinventing the wheel. It sounds like you could rather use Java Properties classes/tools to handle configuration files. Check it out docs.oracle.com/javase/tutorial/essential/environment/… \$\endgroup\$ – user985366 Oct 10 at 12:31
  • 1
    \$\begingroup\$ @user985366 he is parsing an .ini file, not really standard properties. And using the init4j.Ini library, not really reinventing the wheel IMO. \$\endgroup\$ – Marc Oct 10 at 15:21
  • \$\begingroup\$ @user985366 Well I find the sections in ini4j.Ini very convenient, and as far as I can see java Properties class doesn't have that feature. \$\endgroup\$ – Zoran Jankov Oct 11 at 10:33
  • 1
    \$\begingroup\$ @ZoranJankov Right. I was thinking more generally, that properties files could replace .ini files, but I see now that that's too far from your use case. \$\endgroup\$ – user985366 Oct 11 at 13:23
4
\$\begingroup\$

Defualt Java style is to have the opening braces on the same line, but that's down to preferences. Just be consistent in your code.


private static final String SETTINGS_FILE_PATH = "D:/Programing/Java/Software Installer/src/main/resources/Settings.ini";

You want to pass that path, somehow, as it is not portable.


public class SettingsManager
{
    private static SettingsManager instance;

    public static SettingsManager getInstance()
    {
        if(instance == null)
        {
            instance = new SettingsManager();
        }
        return instance;
    }
}

This is not thread-safe, in case you care about it. An obligatory number of threads could end up with their own, not-shared instances of the SettingsManager. There are three ways to prevent that:

The first is to use a static initializer:

public class SettingsManager
{
    private static SettingsManager instance;

    static
    {
        instance = new SettingsManager();
    }
}

Static initializers are guaranteed to be synchronized and will only be executed once the class is accessed.

The second option is to synchronize the whole method:

public class SettingsManager
{
    private static SettingsManager instance;

    public static synchronized SettingsManager getInstance()
    {
        if(instance == null)
        {
            instance = new SettingsManager();
        }
        return instance;
    }
}

This guarantees a single instance for everyone, but it means that an expensive lock is acquired for each access. "Expensive" in this case means "compared to simply getting the value", in 99.99% of the cases you will not notice the performance difference to method #3.

The third method is to check twice whether the value is available, once "cheap", and then synchronized:

public class SettingsManager
{
    private static SettingsManager instance;

    public static SettingsManager getInstance()
    {
        if(instance == null)
        {
            synchronized(SettingsManager.class)
            {
                if(instance == null)
                {
                    instance = new SettingsManager();
                }
            }
        }
        return instance;
    }
}

That skips the locking in most instances, except when required, namely if two threads try to access the instance for the first time.


private static final String SETTINGS_FILE_PATH = "D:/Programing/Java/Software Installer/src/main/resources/Settings.ini";

You want to pass that path, somehow, as it is not portable in the slightest in this way.

Now, that leads to the problem that you only have one single, global instance, and if you pass the path you have to ask yourself the question what should be done when a second instance with a different path is requested. It might be better to have a single instance which you pass around in your application, or which you make globally accessible throughout your application, and not through the class level of this class.


        Ini settings = null;
        
        try
        {
            settings = new Ini(new File(SETTINGS_FILE_PATH));
        }
        catch (IOException e)
        {
            e.printStackTrace();
        }

You most likely want to re-throw that exception, or let it propagate freely by letting loadSettings throw IOException. Silently swallowing exceptions might lead to a really bad headache when debugging problems.


Map<Software, String> list = new HashMap<Software, String>();

That's not a list, that's a map. Naming like that is confusing. Also, in Java 8+ you can omit the types in the constructor, as they are derived from the declaration:

Map<Software, String> list = new HashMap<>();

public enum Software
{
    AIMP,
    CCleaner,

Java convention is that enum members are considered constants, therefor should be UPPER_SNAKE_CASE.


Now, what you don't want is that Software enum, that should be configuration that you read, too, because that allows easier maintenance and extension of the software. Overall, your ini format seems not suited for what you want, it looks more like you want to invert the format:

[AIMP]
Local Repository=...
Network Repository=...
Online Repository=...
Arguments=...

[CCleaner]
Local Repository=...
Network Repository=...
Online Repository=...
Arguments=...

Because with that in place, you can kill off Software completely and make your application driven by configuration. You simply read the ini file and display the list of software that is configured instead of having it hard-coded.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your extensive a answer and advices. I will try implementing everything you mentioned in the answer except the Java Code Style Format, namely the to have the opening braces on the same. I don't like it because it makes code tightly packed and more harder to see code bloks, for me anyway. I follow C++ code style format for every language. \$\endgroup\$ – Zoran Jankov Oct 11 at 10:28
  • 1
    \$\begingroup\$ And as I said, that's fine as long as you're consistent (and if your working with somebody else, provide an automatic formatter). \$\endgroup\$ – Bobby Oct 11 at 11:29
  • \$\begingroup\$ How can I get the names of all sections in .ini file from ini4j.ini class? \$\endgroup\$ – Zoran Jankov Oct 11 at 12:03
  • 1
    \$\begingroup\$ Skimming the documentation and source code, keySet most likely. \$\endgroup\$ – Bobby Oct 11 at 12:25
  • 1
    \$\begingroup\$ That's not necessarily a bad thing. Abstracting any dependencies can be a good idea to have control over the API within your application. \$\endgroup\$ – Bobby Oct 12 at 16:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.