2
\$\begingroup\$

I'm a fairly new developer (well, engineer really, but currently working on a software project). In the Android project I'm working on, I read a node representation of a map into my app and interpret the tab-separated information as Node objects (defined below) which are then inserted into a Map<Integer, Node>. Among the information is the connections between Nodes, which is really what I'm interested in, and for easy use I represent the connections with Edge objects.

The first line contains a header and should be skipped. Each subsequent line looks like:

IDTabconnection1;connection2;TabuselessTabuselessTabuselessTabxTabyTabz

The code works fine, but I think it looks a bit messy (especially the parseText method could probably use some refactoring, but I'm not sure how without excessive passing of, for example, nodeMap).

I'm looking for input primarily on general code structure, readability, style etc.

(But if you have input on the Node/Edge representation of the data, I'm all ears (the application is a navigation app using the angle of lines in the map as reference (azimuth)).)

public static void parseText(String fileName, Map<Integer, Node> nodeMap) {
    // Path to external storage (SD)
    File sdcard = Environment.getExternalStorageDirectory();

    // File path
    File file = new File(sdcard, fileName);

    try {
        BufferedReader br = new BufferedReader(new FileReader(file));
        String line;

        // Check for empty file
        if (br.readLine() == null) {
            return;
        }

        // Parse rows until end of document
        while ((line = br.readLine()) != null) {
            String[] words = line.split("\t");
            Node n = new Node(words[0], words[5], words[6], words[7]);
            nodeMap.put(n.getID(), n);

            String[] sConnections = words[1].split(";+");
            if(!words[1].equalsIgnoreCase("NULL")) {
                for (String sID : sConnections) {
                    int id = Integer.parseInt(sID);
                    if (nodeMap.containsKey(id)) {
                        new Edge(n, nodeMap.get(id));
                    }
                }
            }
        }
        br.close();
    }

    catch (IOException e) {
        return;
    }
}

Node.java

public class Node {
    private int ID;
    private float x;
    private float y;
    private float z;
    private ArrayList<Edge> edges = new ArrayList<Edge>();

    /* Constructor */
    public Node(int ID, float x, float y, float z) {
        this.ID = ID;
        this.x = x;
        this.y = y;
        this.z = z;
    }

    /* Helper constructor for String input */
    public Node(String sID, String sX, String sY, String sZ) {
        this(Integer.parseInt(sID), Float.parseFloat(sX), Float.parseFloat(sY), Float.parseFloat(sZ));
    }

    /* Example: [id:569, x:1294, y:1399, z:-688, nodeConnections:(568, 570, 4322)] */
    public String toString() {
        String s = "[id:" + ID + ", x:" + (int)x + ", y:" + (int)y + ", z:" + (int)z + ", edges:(";

        for (int i = 0; i < edges.size(); i++) {
            s = s + edges.get(i).toString(this);
            if (i < edges.size() - 1) {
                s = s + ", ";
            }
        }
        return s + ")]\n";
    }

    /* Computes euclidean distance between Nodes */
    public static float distance(Node n1, Node n2) {
        float dx = n1.x - n2.x;
        float dy = n1.y - n2.y;
        float dz = n1.z - n2.z;
        return (float) Math.sqrt(dx * dx + dy * dy + dz * dz);
    }

    public int getID() {
        return ID;
    }

    public void connectEdge(Edge newEdge) {
        for (Edge edge : edges) {
            Edge.connectEdges(edge, newEdge);
        }
        edges.add(newEdge);
    }

    public static float getAzimuth(Node n1, Node n2) {
        return (float) Math.atan2(n1.y - n2.y, n1.x - n2.x);
    }
}

Edge.java

public class Edge {
    private float azimuth; // [0, pi)
    private float length;
    private ArrayList<Edge> edgeConnections = new ArrayList<>();
    private Node[] nodes = new Node[2];

    public Edge(Node newNode, Node oldNode) {
        this.nodes[0] = newNode;
        this.nodes[1] = oldNode;

        this.length = Node.distance(newNode, oldNode);
        this.azimuth = Node.getAzimuth(newNode, oldNode);

        newNode.connectEdge(this);
        oldNode.connectEdge(this);
    }

    public String toString() {
        return "{" + nodes[0].getID() + ", " + nodes[1].getID() + "}";
    }

    public String toString(Node caller) {
        if (nodes[0].equals(caller)) {
            return "{" + nodes[1].getID() + "}";
        } else if (nodes[1].equals(caller)) {
            return "{" + nodes[0].getID() + "}";
        }

        return "{---- UNCONNECTED NODE ----}";
    }

    /* Connects two edges if not already connected */
    public static void connectEdges(Edge e1, Edge e2) {
        if(!e1.isConnectedTo(e2)) {
            e1.edgeConnections.add(e2);
        }
        if(!e2.isConnectedTo(e1)) {
            e2.edgeConnections.add(e1);
        }
    }

    /* Returns true if Edge e is connected to this */
    private boolean isConnectedTo(Edge n) {
        return edgeConnections.contains(n);
    }

    public float getAzimuth() {
        return azimuth;
    }

    public float getLength() {
        return length;
    }
}
\$\endgroup\$
  • \$\begingroup\$ I have rolled back Rev 6 → 5. Please see What to do when someone answers. \$\endgroup\$ – 200_success Jun 30 '15 at 8:42
  • \$\begingroup\$ Thanks for keeping me on track. I suppose a self-answer would have been appropriate in this case. \$\endgroup\$ – Svj0hn Jun 30 '15 at 9:10
3
\$\begingroup\$

Write smaller (more) methods

Ideally, methods (or functions, procedures, subroutines) do just one thing and therefore are just a few lines long. The method body should be just 2, 3, but hardly ever more than 5 lines.

Your parseText() method is very very long. It prepares a setup with resources by opening a BufferedReader on a File from the external storage, reads the file and sets up the data structures. It should be split.

Also, variables which are used only once can be inlined. Some developers prefer to have them on separate lines for easier debugging. While there's a point to it, the value of that is minimal. Debugging should be avoided anyway, bugs shouldn't be found after they're made, they should be prevented by a proper developing approach (TDD). And a good debugger will help you with complex expressions.

You could extract a method openFileFromExternalStorage(final String fileName), like this:

private static BufferedReader openFileFromExternalStorage(final String fileName) {
    return new BufferedReader(
        new FileReader(
            new File(Environment.getExternalStorageDirectory(), fileName)));
}

Then the start of your parseText() method would look like this:

public static void parseText(final String fileName, final Map<Integer, Node> nodeMap) {
    try {
        final BufferedReader br = openFileFromExternalStorage(fileName);
        // ...

And remember Uncle Bob's saying: Braces are an opportunity to extract.

I'd split parseText() into at least 5 separate methods.

I hope that this gives you some ideas of how to split long methods.

Finish what you start

In case there is an exception while reading from br, the BufferedReader resp. its underlying FileReader wouldn't be closed.

The correct pattern for this would actually be (before Java 7):

try {
    final BufferedReader br = /* ... */;
    try {
        // Do something with br
    } finally {
        br.close();
    }
} catch (final IOException e) {
    // Exception Handling
}

If your environment is new enough, you can use Java 7 try-with-resources which is significantly more convenient:

try (final BufferedReader br = /* ... */) {
    // Do something with br
}

Note that there is no br.close() when using try-with-resources. When using try-with-resources, Java takes care of the close() itself.

Yes there is a tiny gap here, what if new FileReader() succeeds but new BufferedReader() fails? Usually we don't cover that because that's extremely unlikely. Not the usual extremely unlikely you hear by developers 2 seconds before the unlikely case really happens. But the only thing that could prevent new BufferedReader() from succeeding is an OutOfMemoryError.

You have to close the file - even if it's empty.

You check the file for being empty by checking whether the first line was null. If it was, and the file is empty, you return, bypassing br.close().

If br.close() was in a finally block or handled by a try-with-resources, the return wouldn't bypass the br.close() and the file would be closed properly.

Check for empty file redundant?

Your while loop already performs a head check for the EOF. A separate EOF check before is redundant.

Is skipping the first line of the file intentional?

Because you first read a line to check whether the file is empty, and then read a line again, you skip the first line of the file. Without any further explanation, this seems surprising. If the first line is really to be skipped, there should be a comment about this, or even better, a method call of a method with a good corresponding name.

In this case: Prefer for over while

Because you're using a variable which is only relevant for the loop.

Replace

String line;

// ...

while ((line = br.readLine()) != null) {

with

for (String line; (line = br.readLine()) != null;) {

Avoid out-parameters

In parseText(), nodeMap is an out parameter. It's not read, put it's written, it's updated.

If nodeMap only ever has callers where nodeMap is, prior to the call, empty, and the type of the Map is always the same, i.e. HashMap, the nodeMap could instead be created in the parseText() method and returned.

If nodeMap could be non-empty and thus couldn't be created by parseText(), I'd rearrange the signature and make nodeMap the first parameter. Out-parameters are usually easier to understand if they are the first parameter - unless there is a clear copy-pattern.

Another alternative to avoid out-parameters is to disguise them in fields using the Decorator design pattern, but in your case that would probably be overkill.

Immutable Node class?

It seems that ID, x, y, z are never assigned anywhere else than in the constructor of Node. In that case, they should be final (regardless of whether you usually make automatic variables final).

Consider using import static where it improves readability

This would change

this(Integer.parseInt(sID), Float.parseFloat(sX), Float.parseFloat(sY), Float.parseFloat(sZ));

to

import static java.lang.Integer.parseInt; import static java.lang.Float.parseFloat;

and

this(parseInt(sID), parseFloat(sX), parseFloat(sY), parseFloat(sZ));

This might also be a good candidate for your program:

import static java.lang.Math.*;

If you're keen on performance, use StringBuilder in Node.toString()

In general one could argue whether using StringBuilder is a "natural thing" in Java, or whether you should avoid it. But when appending over and over again, StringBuilder is faster because it avoids the intermediate String copies.

I personally use StringBuilder because for me it feels natural. But I wouldn't recommend it just for the sake of it: The source code reads nicer when using String + instead of StringBuilder.append().

So, this point is more for information, and in toString(), performance usually doesn't matter anyway.

Java Naming Conventions

It is unusual to see a variable name ID in Java. To Java programmers, ID looks like a constant. The expected variable name is id - all lowercase.

Don't be more specific than required (types)

Your

private ArrayList<Edge> edges = new ArrayList<Edge>();

should be

private List<Edge> edges = new ArrayList<Edge>();

All code that uses your edges could work just as well when it were a LinkedList instead, so why bind it to ArrayList if it could be less specific.

float vs double

I'd have chosen double instead of float because that needs less casts.

If you've chosen float instead of double for performance or size reasons, I'd mention that in a small comment (and if you were in my team, you would have to prove that the benefits are worth using float instead of double ;).

isConnectedTo()?

What puzzles me most is that isConnectedTo() is a private without callers. Couldn't it be deleted then? If it could, who else uses edgeConnections? Then, edgeConnections, connectEdges(), Node.connectEdge() and such could also be deleted.

The following discussion assumes that isConnectedTo() should in fact be public.

It took me a while to understand how isConnectedTo() works.

As far as I understand, isConnectedTo() is non-recursive. In other words, two edges are connected if they have one or more nodes in common.

Could isConnectedTo() also be implemented like this?

public boolean isConnectedTo(Edge e) {
    return nodes[0] == e.nodes[0] || nodes[0] == e.nodes[1] || nodes[1] == e.nodes[0] || nodes[1] == e.nodes[1];
}

Then maybe a lot of data structures would also not be needed.

public and its meaning in architecture

When you make something public, what you usually imply is that it's to be called, if useful, anyhow, anywhere, anytime.

Some of the methods, like connectEdges() and connectEdge(), seem to only fulfil the purpose of internal data structure setup. I'd avoid making such methods public wherever possible.

Naming

I wouldn't call an Edge n, that's confusing, in that context I'd think that n is a Node (isConnectedTo(Edge n)).

Disclaimer on final automatic variables

You do not need to use final the way I did in the listings above. I like to declare that my variables aren't reassigned. I'd prefer to instead declare which variables are reassigned, but alas Java works the other way round. Others say that using final everywhere pollutes the source, and I agree with that. It's a matter of priority: Pollution of the source code vs declaration what you modify and what you don't modify. Some chose one, I chose the other.

Edit: NodeParser.java

You are right about passing the nodeMap around feeling "iffy". You can avoid that by creating an object and using the nodeMap as a field of that object.

Here's a NodeParser that does exactly that:

public class NodeParser {
    private final Map<Integer, Node> nodeMap = new LinkedHashMap<>();

    private NodeParser() {}

    public static Map<Integer, Node> parseNodeMapFromFile(final String fileName) {
        final NodeParser nodeParser = new NodeParser();
        nodeParser.parseNodeMapFromFileImpl(fileName);
        return nodeParser.nodeMap;
    }

    private void parseNodeMapFromFile(final String fileName) {
        try {
            final BufferedReader br = openFileFromExternalStorage(fileName);
            try {
                parseFile(br);
            } finally {
                br.close();
            }
        } catch (final IOException e) {
            e.printStackTrace();
        }
    }

    private static BufferedReader openFileFromExternalStorage(final String fileName) throws FileNotFoundException {
        return new BufferedReader(
            new FileReader(
                new File(Environment.getExternalStorageDirectory(), fileName)));

    }

    private void parseFile(final BufferedReader br) throws IOException {
        skipFirstLine();
        parseNodeList(br);
    }

    private void skipFirstLine(final BufferedReader br) throws IOException {
        br.readLine();
    }

    private void parseNodeList(final BufferedReader br) throws IOException {
        for (String line; (line = br.readLine()) != null;)
            parseNodeLine(line);
    }

    private void parseNodeLine(final String line) {
        final String[] words = line.split("\t");
        final Node node = new Node(words[0], words[5], words[6], words[7]);
        nodeMap.put(node.getId(), node);

        if (!words[1].equalsIgnoreCase("NULL"))
            parseNodeConnections(node, words[1]);
    }

    private void parseNodeConnections(final Node node, final String sConnectionsStr) {
        final String[] sConnections = sConnectionsStr.split(";+");
        for (final String sId : sConnections)
            parseConnection(node, Integer.parseInt(sId));
    }

    private void parseConnection(final Node node, final int id) {
        if (nodeMap.containsKey(id))
            new Edge(node, nodeMap.get(id));
    }
}

See how the object is used to simplify the code.

  • The parameter nodeMap is removed.
  • The methods are no longer static.

But also see how the object that does so is intentionally kept secret. The caller doesn't notice that there's a NodeParser object. The caller shall not know of that object. The caller is only interested in the Map<Integer, Node>, and that's exactly what the caller gets. Making the NodeParser object known to the caller would be problematic from more complexity on the caller side to race conditions in multi-threading.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for the input! The first line is indeed to be skipped, good catch. Also, the isConnectedTo(Edge n) is used in the connectEdges(...) method. It could be implemented as you suggested, which would remove the need of the edgeConnections list, however the useage of the data (not shown above) only occasionally uses information about the Nodes, so I thought it better to allow navigating from Edge to Edge. I'll definitely have a look at splitting the parseText() method into smaller pieces. \$\endgroup\$ – Svj0hn Jun 30 '15 at 5:59
  • \$\begingroup\$ I added a new class NodeParser to hold the functionality of parseText(), and I tried to incorporate as much I could of the tips you gave. Is this better? Should I extract more/less? There are a lot of passing of nodeMap, which feels kinda iffy. \$\endgroup\$ – Svj0hn Jun 30 '15 at 7:08
0
\$\begingroup\$

For completeness, this is a reworked version of the parseText method after taking into account Christian Hujer's original points (see his edit for further suggestions on how to improve it).

NodeParser.java

public class NodeParser {

    public static Map<Integer, Node> parseNodeMapFromFile(String fileName) {
        Map<Integer, Node> nodeMap = new LinkedHashMap<>();
        try {
            BufferedReader br = openFileFromExternalStorage(fileName);
            try {
                parseFile(nodeMap, br);
            }
            finally {
                br.close();
            }
        } catch (IOException e) {
            e.printStackTrace();
        }

        return nodeMap;
    }

    private static BufferedReader openFileFromExternalStorage(String fileName) throws FileNotFoundException {
        return new BufferedReader(
                new FileReader(
                        new File(Environment.getExternalStorageDirectory(), fileName)));

    }

    private static void parseFile(Map<Integer, Node> nodeMap, BufferedReader br) throws IOException {
        if (fileIsEmpty(br)) { // Skips first line
            return;
        }

        parseNodeList(nodeMap, br);
    }

    /* Skips first line and returns true if file is empty */
    private static boolean fileIsEmpty(BufferedReader br) throws IOException {
        return br.readLine() == null;
    }

    private static void parseNodeList(Map<Integer, Node> nodeMap, BufferedReader br) throws IOException {
        for (String line; (line = br.readLine()) != null;) {
            parseNodeLine(nodeMap, line);
        }
    }

    private static void parseNodeLine(Map<Integer, Node> nodeMap, String line) {
        String[] words = line.split("\t");
        Node node = new Node(words[0], words[5], words[6], words[7]);
        nodeMap.put(node.getId(), node);

        if (!words[1].equalsIgnoreCase("NULL")) {
            parseNodeConnections(nodeMap, node, words[1]);
        }
    }

    private static void parseNodeConnections(Map<Integer, Node> nodeMap, Node node, String sConnectionsStr) {
        String[] sConnections = sConnectionsStr.split(";+");
        for (String sId : sConnections) {
            parseConnection(nodeMap, node, Integer.parseInt(sId));
        }
    }

    private static void parseConnection(Map<Integer, Node> nodeMap, Node node, int id) {
        if (nodeMap.containsKey(id)) {
            new Edge(node, nodeMap.get(id));
        }
    }
}
\$\endgroup\$

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.