HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Setting the positions of nodes based on a layout

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
nodesthepositionslayoutsettingbased

Problem

I have a bit of code that looks like this:

for (Node n : g.getNodes().values()){
        n.setPosition(new Position(new Double(lo.getX(new Integer(n.getId()))).intValue(), new Double(lo.getY(new Integer(n.getId()))).intValue()));        
    }


Essentially all I'm wanting to do is for each node in the collection, fetch and assign the position for that node from the layout.

Let me explain:

  • Node has an id of type int which is returned by n.getId().



  • The Layout lo will return the X and Y positions of the node by calling getX(Integer id) and getY(Integer id). The type returned is double.



  • A Node has a Position object which has constructor (int, int).



So:

  • In order to pass the id into the layout i need to use new Integer(id)



  • In order to convert the returned double to int I need to first upgrade the double to Double using new Double and then use .intValue() to convert it to an int.



It all seems ridiculous and verbose.

I am the one writing the Position and Node classes, so I can change the constructor and method signatures and return types if want. The Layout class is an external library, so I'll have to work with their signatures.

For example, an easy fix would be to create a Position(double x, double y) constructor, and do the double -> Double -> int conversion in the constructor.

public Position (double x, double y) {
    this.x = new Double(x).intValue(); 
    this.y = new Double(y).intValue(); 
}


This would change the code to:

for (Node n : g.getNodes().values()){
        n.setPosition(new Position(lo.getX(new Integer(n.getId())),lo.getY(new Integer(n.getId()))));       
    }


I could also create a second getter on Node to return Integer.

public Integer getIdAsInteger(){
    return new Integer(this.id); 
}


Is this a good idea?

This would reduce the code to:

```
for (Node n : g.getNodes().values()){
n.setPosition(new Position(lo.getX(n.getId

Solution

If you have the control over the Position class, why not have a setPosition which takes the layout as a parameter? This would render the loop to be:

for (Node n: g.getNodes().values) {
    n.setPosition(lo);
}


This way you hide the conversion within the method, but it is still clear that your position is set relative to the layout, lo. How you implement the setPosition() isn't then as interesting, but you could then easily do something like:

public void setPosition(Layout lo) {

    Position newPos = new Position(
        new Double(lo.getX(this.id)), 
        new Double(lo.getY(this.id)));

    setPosition(newPos());
}


This assumes that the id is available as an Integer within the Node class, but you'll get the gist of the idea and can surely complete it.

Code Snippets

for (Node n: g.getNodes().values) {
    n.setPosition(lo);
}
public void setPosition(Layout lo) {

    Position newPos = new Position(
        new Double(lo.getX(this.id)), 
        new Double(lo.getY(this.id)));

    setPosition(newPos());
}

Context

StackExchange Code Review Q#121396, answer score: 3

Revisions (0)

No revisions yet.