4
\$\begingroup\$

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.getIdAsInteger()),lo.getY(n.getIdAsInteger())));       
    }

Finally, I could also create an additional setter setPosition(double x, double y).

public void setPosition(double x, double y) {
    this.position = new Position(x, y); 
}

which would reduce the code to:

    for (Node n : g.getNodes().values()){
        n.setPosition(lo.getX(n.getIdAsInteger(),lo.getY(n.getIdAsInteger()));      
    }

The question is - is this the right way to go about structuring my code? It still seems very verbose, and a lot of boiler plate, I'm wonder if there's another much simpler design pattern that addresses this.

\$\endgroup\$
  • 1
    \$\begingroup\$ The for loop is just a symptom of problems with your Position and Layout classes. I strongly suggest that you add those classes to the question so that we may advise you properly. \$\endgroup\$ – 200_success Feb 29 '16 at 1:13
  • \$\begingroup\$ Does a Node has a reference to the Layout ? \$\endgroup\$ – Spotted Feb 29 '16 at 6:56
  • 2
    \$\begingroup\$ 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. any reason? \$\endgroup\$ – abuzittin gillifirca Mar 1 '16 at 7:41
3
\$\begingroup\$

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

No you don't. Your biggest problem is you do not know about autoboxing(google it). Basically you can pass a primitive parameter if a method expects a wrapper, and vice versa.

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.

No you don't. You can cast a double to int. Like so: int i = (int)3.14159;

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.

Don't mar the interface of an class for the needs of another. Also don't do work in constructor. Use a factory method for this need.

I could also create a second getter on ...

Like I said don't disfigure your classes.

Finally, I could also create an additional setter ...

Just don't.

which would reduce the code to:

for (Node n : g.getNodes().values()){
    n.setPosition(lo.getX(n.getIdAsInteger(),lo.getY(n.getIdAsInteger()));

}

It isn't pretty at all. And now your classes are ugly too.

Instead using autoboxing and a factory method and extracting the functionality of getting the position of a node from a layout to a separate method, you get more readable code.

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

/// get the position of a node from a layout
static Position getPosition(Layout layout, Node node) {
    return createPosition(
                layout.getX(node.getId()), 
                layout.getY(node.getId()));
}

/// create a position from coordinates
/// truncating them if necessary
static Position createPosition(double x, double y) {
    return new Position((int)x, (int)y);
}

Further possible improvements

Static methods are not object-oriented really, they indicate that you might need to do more object modelling, (if worth the effort). For example Layout already knows about node IDs, maybe it could know about nodes, too. If so a next step would be moving getPosition to Layout. We need to know more about the classes involved as @200_success told.

Setters can be an indication of a modelling problem too. If you need to keep setting a property on an object, it might not want to manage that piece of information. Does a node have a position only with respect to a layout? Would it be more sensible if the users of that information asked to the layout for a nodes position (see above for Layout.getPosition)?

About converting doubles to ints, why do you need it? Do you need to reify a step, like rasterization converting continuous positions to screen coordinates, as a method or a service?

Would anyone call layout.getX without calling layout.getY? Should you be returning java.awt.geom.Point2D or a similar class?

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks heaps. :) Checkmark for the frank and to the point style, this is what I'm looking for. \$\endgroup\$ – dwjohnston Mar 2 '16 at 21:37
  • \$\begingroup\$ @dwjohnston Thanks for taking the time to explain the reasoning behind your decisions. Also if you are writing your own layout engine you can get a lot of useful feedback here. And if you ask more question it could be an opportunity for the rest of us, as you receive feedback, to learn new things about design and performance considerations etc. \$\endgroup\$ – abuzittin gillifirca Mar 3 '16 at 7:32
3
\$\begingroup\$

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.

\$\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.