Simple service that wraps access to json file

Question

I'm wondering if this is the best way to write the Java code I have written. It works, but looks overly verbose, is it possible to rewrite any of the code to make it more concise? Please ignore the constructors for now as I'm going to add DI in the coming days. I did google beforehand but wasn't getting much luck with the search results. I'm coming from a C# background and still learning so if anything looks out of place please let me know. TIA.

package com.professorofprogramming.services;

import org.json.JSONException;
import org.json.JSONObject;

import java.io.IOException;

public class SettingsService {
    AppSettingsFileService appSettingsFileService;
    SimpleCacheService simpleCacheService;

    SettingsService() {
        appSettingsFileService = new AppSettingsFileService();
        simpleCacheService = new SimpleCacheService();
    }

    SettingsService(AppSettingsFileService asfs) {
        appSettingsFileService = asfs;
    }

    SettingsService(SimpleCacheService scs) {
        simpleCacheService = scs;
    }

    SettingsService(AppSettingsFileService asfs, SimpleCacheService scs) {
        appSettingsFileService = asfs;
        simpleCacheService = scs;
    }

    public String getSetting(String settingName) throws JSONException, IOException {
        return getSetting(settingName, true);
    }

    public String getSetting(String settingName, boolean useCache) throws JSONException, IOException {
        JSONObject jObject;

        String cachedSettingsKey = SettingsService.class.getName();
        if (useCache && simpleCacheService.contains(cachedSettingsKey)) {
            jObject = simpleCacheService.get(cachedSettingsKey);
        }
        else {
            jObject = readSettingsFile();
        }

        return jObject.getString(settingName);
    }

    private  JSONObject readSettingsFile() throws JSONException, IOException {
        String appSettingsFileContents = appSettingsFileService.getContents();
        return new JSONObject(appSettingsFileContents);
    }
}

The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?, as well as How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. — Sᴀᴍ Onᴇᴌᴀ
– Sᴀᴍ Onᴇᴌᴀ ♦, Commented Nov 6, 2023 at 23:12
Implementing caching as a full fledged feature of the SettingsService violates the single responsibility principle and is a form of reinventing the wheel. There are many well known caching libraries that can be configured using annotations. If you really require a home cooked cache implementation, then it should be implemented as a decorator around the SettingsService. This requires defining the service as an interface. — TorbenPutkonen
– TorbenPutkonen, Commented Nov 7, 2023 at 8:02
@TorbenPutkonen I'm going to be wiring in DI, so this won't be the case, but I'm interested in the libraries you mention if you could mention a few of them so that I can take a look, thanks. — Professor of programming
– Professor of programming, Commented Nov 7, 2023 at 16:57

Martin Frank · Accepted Answer · 2023-11-09 10:28:11Z

Using default Values

in Android apps it is common practice to provide a default value if there is exists no defined value

public String getSetting(String settingName, String defaultValue){
    String value = defaultValue;
    try{
        String setting = getSetting(settingName, true);
        return Strings.isNullOrEmpty(string) ? defaultValue : setting;
    }catch (Exception e){
        //fallback in case of emergency ;-)
        return defaultValue;
    }
}

appying this practice you can give your app an valid value even if there is no. and this gives you the opprotunity to handle exceptions in a better way

update cache

if there is no value in your cache service and you have to load from file... then why don't you update your cache service, when you get an update?

naming

with these limited information it is not clear, why you call it an Setting Service. and the usage of this service could be used anywhere where you have to read some values from json (or cacheservice)

maybe you create a purpose-independet service and just give the variable a proper name?

LookupService settingService = new LookUpService(...)*;

*) just in case your naming would lead to LookupService

I spotted the issue where I was updating the cache and fixed that the other day. I'm less worried about what the code is doing and more worried about how verbose it is, say for instance the constructors, is this the correct way to have constructors which allow a varying number of arguments to be passed in Java, and the same with the getSetting methods, I'm really scratching at the basics at the moment and will worry about what the code is doing later. — Professor of programming
– Professor of programming, Commented Nov 9, 2023 at 15:48

Martin Frank · Accepted Answer · 2023-11-10 07:47:39Z

slighty confused about requested question

Please ignore the constructors for now

and

I'm [...] more worried about how verbose it is, say for instance the constructors

a comment to

Dependecy Injection

since your class is totally dependent on AppSettingsFileService and totally dependent on SimpleCacheService the proper way to address these dependency is using dependency Injection.

That is implemented through

one proper constructor that provides all dependencies that are essential
and by making these essential fields final
the dependencies are interfaces not implementing classes

you can add on top a dependency injection container like maybe spring but don't fall into the pitfalls of DIC, read this side note about its perils - and no, they are not evil ;-)

a proper constructor would be

public class SettingsService {
    private final AppSettingsFileService appSettingsFileService; //new: final interface
    private final SimpleCacheService simpleCacheService; //new: final interface

    //nope - not this one
    //SettingsService() {
    //    appSettingsFileService = new AppSettingsFileService();
    //    simpleCacheService = new SimpleCacheService();
    //}

    //nope - not this one either
    //SettingsService(AppSettingsFileService asfs) {
    //    appSettingsFileService = asfs;
    //}

    //nope - not this, too 
    //SettingsService(SimpleCacheService scs) {
    //    simpleCacheService = scs;
    //}

    //yes, that is the one and only 
    SettingsService(AppSettingsFileService asfs, SimpleCacheService scs) {
        appSettingsFileService = asfs;
        simpleCacheService = scs;
    }

}

some benefits of this approach:

SOLID O: Open for enhancement close for manipulation - you cannot by mistake or any other means forget to set the proper depenedcies (closed for manipulation)
Testability: you can easily replace your real services with test mocks for tests, since you are using only interfaces, define functionallity
SOLID D: Dependency inversion, since that approach forces to define a well-working interface and the implementation has to follow it

Stack Exchange Network

Simple service that wraps access to json file

2 Answers 2

Using default Values

update cache

naming

Dependecy Injection

You must log in to answer this question.

Hot Network Questions

Simple service that wraps access to json file

2 Answers 2

Using default Values

update cache

naming

Dependecy Injection

You must log in to answer this question.

Related

Hot Network Questions