4
\$\begingroup\$

I was wondering if I could get some feedback on the library I created to persist data online.

JitterPushDemo.java

public class JitterPushDemo {

    public static void main(String[] args) {

        Jitter jitter = new Jitter("scores");


        // create hash map to map scores to players

        HashMap<String, String> map = new HashMap<String, String>();
        map.put("Bob", "300");
        map.put("Sam", "500");

        // push data online to jitter object
        jitter.pushData(map);

    }

}

JitterPullDemo.java

public class JitterPullDemo {

    public static void main(String[] args) {

        Jitter jitter = new Jitter("scores");

        // now pulling data from online and iterating
        for (Map.Entry<String, String> item: jitter.pullData().entrySet()) {
            System.out.println("Player: " + item.getKey());
            System.out.println("Scores: " + item.getValue());
        }  

    }

}

Source Code

package org.main;

import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.net.URL;
import java.net.URLEncoder;
import java.util.HashMap;
import java.util.Map;

import org.json.JSONArray;
import org.json.JSONObject;

public class Jitter {

    URL location;
    String thing;

    public Jitter(String thing) {
        this.thing = thing;
    }

    private void setLocation(String place) {

        try 
        {
            location = new URL(place);
            location.openConnection();
        }

        catch (Exception e) 
        {

        }

    }

    public String readData() {

        InputStream dataStream = null;
        InputStreamReader read = null;
        String result = ""; int value;

        try 
        {
            dataStream = location.openStream();
            read = new InputStreamReader(dataStream);

            while ((value = read.read()) != -1)
                result += String.valueOf((char) value);
        }

        catch (Exception e)
        {

        }

        return result;
    }

    public void pushData(HashMap<String, String> map) {

        String url = "https://dweet.io/dweet/for/" + thing + "?";

        for (Map.Entry<String, String> item : map.entrySet()) {
            String k = URLEncoder.encode(item.getKey());
            String v = URLEncoder.encode(item.getValue());
            String sub = k + '=' + v + "&"; url += sub;
        }

        try 
        {
            setLocation(url);
        }

        catch (Exception e) 
        {

        }

        readData();

    }

    public void pushData(String key, String value) {
        String url = "https://dweet.io/dweet/for/" + thing + "?";
        setLocation(url + key + "=" + value); readData();
    }

    public String pullData(String key) {

        setLocation("https://dweet.io/get/latest/dweet/for/" + thing);
        JSONArray main = new JSONObject(readData()).getJSONArray("with");
        JSONObject data = ((JSONObject) main.get(0)).getJSONObject("content");

        return data.optString(key);
    }

    public HashMap<String, String> pullData() {

        setLocation("https://dweet.io/get/latest/dweet/for/" + thing);
        JSONArray main = new JSONObject(readData()).getJSONArray("with");
        JSONObject data = ((JSONObject) main.get(0)).getJSONObject("content");

        HashMap<String, String> map = new HashMap<String, String>();

        for (String key : data.keySet()) {
            map.put(key, data.optString(key));
        }

        return map;
    }

}
\$\endgroup\$
5
\$\begingroup\$

FGITW:

Duplication

JSONArray main = new JSONObject(readData()).getJSONArray("with");
JSONObject data = ((JSONObject) main.get(0)).getJSONObject("content");

You have the duplicated code block above in a few places, it's probably better to put them in a method.

private JSONObject getJSONObject(final String jsonOutput) {
    return ((JSONObject) new JSONObject(jsonOutput).getJSONArray("with").get(0))
        .getJSONObject("content").
}

Same goes for the String https://dweet.io, extract it to a final field perhaps?

Input sanitization?

You are currently constructing your URLs directly by appending thing. I'm not sure how dweet handles weird keys/data, but you should consider doing some kind of sanity check here.

Handling for one or multiple keys/values

I will prefer to convert the arguments to the pushData(String, String) method as a Map so that I can pass it to the other pushData(Map) method. This ensures your URI encoding is handled correctly - as you can see currently you aren't handling that for pushData(String, String). Also, I might be missing something here, but why do you need to readData() after pushing data to the web service?

Don't swallow Exceptions

readData() does not appear to handle thrown Exceptions in any meaningful way. Should it log to some console output? Fail fast? Wrap the Exception and throw it to the caller?

I'm also not to keen on calling your InputStreamReader instance read, because read.read() sounds... strange. Why not just call it reader? And I think you'll need to close() it when you're done with it.

Define variables/parameters with interfaces, not implementations

Just one more thing, it's better to declare Map<String, String> map = new HashMap<>(); (>= Java 7 syntax), since your code should not need to be aware of the actual implementation of map. Same goes for public void pushData(Map<String, String> map).

\$\endgroup\$
  • \$\begingroup\$ Firstly, thank your for the input. The read data method simply presses request thus adding the key value pairs in queue. Read the dweet documentation or more. \$\endgroup\$ – Malik Brahimi Mar 10 '15 at 1:01
  • \$\begingroup\$ I've updated my answer a bit... \$\endgroup\$ – h.j.k. Mar 10 '15 at 1:15
  • 1
    \$\begingroup\$ Pushing data only occurs when the stream is read from the given URL. Thus I need to do it in all four methods. \$\endgroup\$ – Malik Brahimi Mar 10 '15 at 1:25
  • \$\begingroup\$ Have you tried the library? Is it useful and does it work as expected? \$\endgroup\$ – Malik Brahimi Mar 10 '15 at 1:26
  • \$\begingroup\$ If that's the case, you can further combine setLocation(...); return readData(); into a single method too... ;) \$\endgroup\$ – h.j.k. Mar 10 '15 at 1:37
4
\$\begingroup\$
  • Off the bat, your code contains no comments. Comments are useful to know what the code does.

  • Your code is very ambiguous. What is

    String thing; // a username?
    

    ... it behooves you to name this more descriptively.

    setLocation()
    

    I notice it sets the subdirectory. You can make this more specific.

  • URLEncoder.encode is marked as deprecated in Netbeans 8. You'll probably want to look into a more forward-compatible alternative:

    public static String encode(String s, String enc);
    
  • Strings are hardcoded all over. It's much easier to maintain code when you put all of those type of things in one place.

  • You are eating your errors. This is very bad. Errors tell you what is going wrong. When you eat errors you summon the wrath of the Gods; and you will never know when your code isn't working.

\$\endgroup\$
  • \$\begingroup\$ I can't find a url encoder that isn't deprecated. As for thing, it is the "thing" you are storing in dweet, according to the documentation. \$\endgroup\$ – Malik Brahimi Mar 10 '15 at 1:04
  • 1
    \$\begingroup\$ Excess comments are bad too. You shouldn't use a comment to state what thing is, you should name it appropriately. \$\endgroup\$ – Hosch250 Mar 10 '15 at 1:22
  • \$\begingroup\$ I think it's because dweet refers to that as a thing pretty much extensively on their website... :) \$\endgroup\$ – h.j.k. Mar 10 '15 at 1:33

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.