2
\$\begingroup\$

I realized I had just created two classes that were virtually identical for simply saving objects to their own json files, so I've attempted to make it more generic. My only noticeable problem is because I am using gson to serialize / deserialize, that requires an explicit class type, which gets squashed if I use the generic T. So I'm using a @StringDef annotation so the class can figure out which file and object I want to serialize / deserialize - couldn't really thing of a nice way of doing but would like to know what people think:

public class Storage<T> {

    //Objects my app can serialize
    public static final String BUSINESS = "b.json";
    public static final String TRANSACTION = "t.json";
    @StringDef({BUSINESS, TRANSACTION})
    @Retention(RetentionPolicy.SOURCE)
    public @interface Type {}

    private static final String TAG = "Storage";
    private TSerializer mSerializer;

    public Storage(Context appContext, @Type String type) {
        mSerializer = new TSerializer(appContext, type);
    }

    public ArrayDeque<T> getFromStorage() {
        try {
            return mSerializer.load();
        } catch (IOException e) {
            e.printStackTrace();
            return null;
        } catch (JSONException e) {
            e.printStackTrace();
            return null;
        }
    }

    public T getLastFromStorage() {
        ArrayDeque<T> array = getFromStorage();
        if (array != null && array.size() > 0)
            return array.getLast();

        return null;
    }

    public boolean saveItem(T item) {
        try {
            mSerializer.save(item);
            Log.d(TAG, "Item saved to file");
            return true;
        } catch (Exception e) {
            Log.e(TAG, "Error saving Item", e);
            return false;
        }
    }

    public boolean wipeItems() {
        try {
            mSerializer.wipe();
            return true;
        } catch (Exception e) {
            Log.e(TAG, "Error wiping storage", e);
            return false;
        }
    }

    public void saveItems(ArrayDeque<T> items) {
        if (items != null && items.size() > 0) {
            for (T t : items)
                saveItem(t);
        }
    }

    private class TSerializer {

        private Context mContext;
        @Storage.Type private String mType;

        private TSerializer(Context c, @Storage.Type String type) {
            mContext = c;
            mType = type;
        }

        private void save(T item)
                throws JSONException, IOException {

            ArrayDeque<T> tList = load();
            if (tList == null)
                tList = new ArrayDeque<>();
            tList.add(item);

            saveList(tList);
        }

        private void save(ArrayDeque<T> items)
                throws JSONException, IOException {
            ArrayDeque<T> tList = load();
            if (tList == null)
                tList = new ArrayDeque<>();
            tList.addAll(items);

            saveList(tList);
        }

        private void saveList(ArrayDeque<T> items)
                throws JSONException, IOException {
            String json = new Gson().toJson(items);
            JsonParser parser = new JsonParser();
            JsonArray array = parser.parse(json).getAsJsonArray();
            String tString = array.toString();

            Writer writer = null;
            try {
                OutputStream out = mContext.openFileOutput(mType, Context.MODE_PRIVATE);
                writer = new OutputStreamWriter(out);
                writer.write(tString);
            } finally {
                if (writer != null)
                    writer.close();
            }
        }

        private void wipe()
                throws JSONException, IOException {
            Writer writer = null;
            try {
                OutputStream out = mContext.openFileOutput(mType, Context.MODE_PRIVATE);
                writer = new OutputStreamWriter(out);
                writer.write("");
            } finally {
                if (writer != null)
                    writer.close();
            }
        }

        private ArrayDeque<T> load() throws IOException, JSONException {
            ArrayDeque<T> transactions = null;
            BufferedReader reader = null;
            try {
                //Open and read the file into a string builder
                InputStream in = mContext.openFileInput(mType);
                reader = new BufferedReader(new InputStreamReader(in));
                StringBuilder jsonString = new StringBuilder();
                String line;
                while ((line = reader.readLine()) != null) {
                    //Line breaks are omitted and irrelevant
                    jsonString.append(line);
                }
                java.lang.reflect.Type type;
                switch (mType) {
                    case Storage.BUSINESS:
                        type = new TypeToken<ArrayDeque<Business>>() {
                        }.getType();
                        break;
                    case Storage.TRANSACTION:
                        type = new TypeToken<ArrayDeque<Transaction>>() {
                        }.getType();
                        break;
                    default:
                        type = new TypeToken<ArrayDeque<Business>>() {
                        }.getType();
                        break;
                }
                transactions = new Gson().fromJson(jsonString.toString(), type);

            } catch (FileNotFoundException e) {
                //Ignore this one, happens when launching for the first time
            } finally {
                if (reader != null)
                    reader.close();
            }
            return transactions;
        }
    }

I then save / retrieve like so:

//save
Storage<Business> s = new Storage<>(getContext(), Storage.BUSINESS);
s.saveItem(b);

//retrieve
Business b = s.getLastFromStorage();
\$\endgroup\$
3
\$\begingroup\$

This feels very over-engineered, dangerous, and not very extensible to me.

Looking at your constructor:

public Storage(Context appContext, @Type String type) {
    // ...
}

Why not simply pass in the Type object you need right there?

public Storage(Context appContext, Type typeOfT) {
    // ...
}

Then when you go to serialize or deserialize, you can simply call the toJson and fromJson methods in Gson.

public void saveItems(final ArrayDeque<T> items) {
    json = gson.toJson(items);
}

public ArrayDeque<T> getItems() {
    return gson.fromJson(json, typeOfT);
}

Forcing your Storage class to anticipate every possible type parameter is a VERY bad idea - there's no way you can possible know in advance every single type that could ever be used.

Just a couple other notes:

  1. There isn't a need to have both saveItem() and saveItems() methods - only saveItems() is necessary. If you only need to save one item, just pass an ArrayDeque with one item.
  2. Similarly, getLastFromStorage() is unnecessary. A consumer can call getLast() themselves. There's also nothing stopping a consumer from making the call to getLast() on their own anyway and ending up with a different result because your method returns null.
  3. IMO wipeItems() can be named better - perhaps removeAll() or something more standard?
  4. Don't be afraid to make new files for classes! Even though we can get rid of it, TSerializer can (and should, IMO) be in a separate class. That will make your code more readable, and easier to test because it isn't a private inner-class.
  5. Stay away from using annotations for things if you can help it. A quick Google search on why to stay away from annotations should be pretty convincing as to why.

So, cutting out all the unnecessary type strings, etc., we end up with something like this:

public class RevisedStorage<T> {

    private final Gson gson;
    private final Type typeOfT;

    private String json;

    /**
     * @param gson
     */
    public RevisedStorage(final Gson gson, final Type typeOfT) {
        this.gson = gson;
        this.typeOfT = typeOfT;

        json = gson.toJson(new ArrayDeque<T>()); // Temporary for this example
    }

    public boolean removeItems() {
        // Do whatever you need to do here
        return true;
    }

    public void saveItems(final ArrayDeque<T> items) {
        writeToExternalStorage(gson.toJson(items));
    }

    public ArrayDeque<T> getItems() {
        return gson.fromJson(readFromExternalStorage(), typeOfT);
    }

    // For the sake of example, just write to a private String instead of doing file I/O
    private String readFromExternalStorage() {
        return json;
    }

    private void writeToExternalStorage(final String data) {
        json = data;
    }

}

And a quick unit test:

public class RevisedStorageTest {

    private final String businessName = "ABC Corp.";

    @Test
    public void test() {
        final Type businessArrayDequeType = new TypeToken<ArrayDeque<Business>>() {
        }.getType();
        final RevisedStorage<Business> businessStorage = new RevisedStorage<Business>(new Gson(),
                businessArrayDequeType);

        final ArrayDeque<Business> businessDeque = new ArrayDeque<Business>();
        businessDeque.add(new Business(businessName));

        businessStorage.saveItems(businessDeque);

        Assert.assertEquals(businessName, businessStorage.getItems().getFirst().getName());
    }

}

public class Business {

    private final String name;

    public Business(final String name) {
        this.name = name;
    }

    public String getName() {
        return name;
    }

}
\$\endgroup\$
  • \$\begingroup\$ I never did accept this, all good responses, thank you! \$\endgroup\$ – Daniel Wilson Jan 7 at 13:27

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.