3
\$\begingroup\$

This library provides methods for storing items into an inventory. the container is based on a grid as well as the items going into that container.

enter image description here

can you please review the code in any way?

public interface GridContainer {
    public Collection<GeoPoint> getContainer();
    public Collection<GridShape> getContent();
    public boolean fitsInside(GridShape container, GeoPoint location);
    public void add(GridShape shape, GeoPoint location);
    public GridShape remove(GeoPoint location);
}
public interface GridShape {
    public boolean fitsInside(GridContainer container, GeoPoint pointer);
    public Collection<GeoPoint> getShape();
    public GeoPoint getPointer();
    public Collection<GeoPoint> getRelativeShape(GeoPoint point);
}

so far we have an implementation for rectangle shaped containers and items

public class RectangleContainer implements GridContainer {

    private final List<GeoPoint> shape;
    private final Map<GeoPoint, GridShape> content;

    public RectangleContainer(int width, int height){
        shape = Collections.unmodifiableList(RectangleUtility.createShape(width, height));
        content = new HashMap<>();
    }

    @Override
    public Collection<GeoPoint> getContainer() {
        return shape;
    }

    @Override
    public Collection<GridShape> getContent() {
        return content.values();
    }

    @Override
    public boolean fitsInside(GridShape shape, GeoPoint location) {
        Collection<GeoPoint>relativeShape = shape.getRelativeShape(location);
        boolean hasIntersections = hasIntersection(relativeShape);
        boolean isInBounds = isInBounds(relativeShape);
        return !hasIntersections && isInBounds;
    }

    private boolean isInBounds(Collection<GeoPoint>relativeShape) {
        for (GeoPoint point: relativeShape){
            if (!shape.contains(point)){
                return false;
            }
        }
        return true;
    }

    private boolean hasIntersection(Collection<GeoPoint>relativeShape) {
        for (Map.Entry<GeoPoint, GridShape> entry: content.entrySet()){
            for (GeoPoint pointOfContent: entry.getValue().getRelativeShape(entry.getKey())){
                if (relativeShape.contains(pointOfContent)){
                    return true;
                }
            }
        }
        return false;
    }

    @Override
    public void add(GridShape shape, GeoPoint location) {
        content.put(location, shape);
    }

    @Override
    public GridShape remove(GeoPoint location) {
        return content.remove(location);
    }
}
public class RectangleItem<I> implements GridShape, ItemHolder<I> {

    private final List<GeoPoint> shape;
    private I item;

    public RectangleItem(int width, int height){
        shape = Collections.unmodifiableList(RectangleUtility.createShape(width, height));
    }

    @Override
    public boolean fitsInside(GridContainer container, GeoPoint pointer) {
        return container.fitsInside(this, pointer);
    }

    @Override
    public Collection<GeoPoint> getShape() {
        return shape;
    }

    @Override
    public GeoPoint getPointer() {
        return shape.get(0);
    }

    @Override
    public Collection<GeoPoint> getRelativeShape(GeoPoint point) {
        return shape.stream().map(p -> new GeoPoint(p.getX()+point.getX(), p.getY() + point.getY())).collect(Collectors.toList());
    }

    @Override
    public I getItem() {
        return item;
    }

    @Override
    public void setItem(I item) {
        this.item = item;
    }
}

Note: i've skipped the test classes and the RectangleUtility class, the geoLib project and the ItemHolder since they are not scope of the review. If they are required please feel free to add a comment asking for them, then i'll add them. (i think this question is already bloated, that's the reason for deciding so)

\$\endgroup\$
2
\$\begingroup\$

General

Task

In my opinion you lost the focus of your task description:

 [...]storing items into an inventory. The container is based on a grid as well as the items going into that container.

The word container can easily be replaced by inventory:

 [...] storing items into an inventory. The inventory is based on a grid as well as the items going into that inventory.

The goal is to store items in an Inventory/Container but currently it stores Shapes instead of Items:

public class RectangleContainer implements GridContainer {

  /* ... */
  private final Map<GeoPoint, GridShape> content;

  /* ... */

  @Override
  public boolean fitsInside(GridShape shape, GeoPoint location) { /* ... */ }

  /* ... */

  @Override
  public void add(GridShape shape, GeoPoint location) { /* ... */ }


}

What is an Item

 What an Item currently is

public class RectangleItem<I> implements GridShape, ItemHolder<I> { /* ... */}

To make it simpler to discuss:

public class Item<I> implements Shape, ItemHolder<I> { /* ... */}

Is Item an Item? When I see only the class name I would say yes, but after looking on the signature I would say: "It could be".  From the signature it is an item that is a shape that holds an item. Is it an item that holds an other item?

What an Item should be

Item is something that has a Shape.

public class Item<I> {

    private I value;
    private Shape shape;

    public Item(I value, Shape shape) { /* ... */ }

}

and an ìtem should be packed into an inventory/container:

public class RectangleContainer implements GridContainer {
    /* ... */
    @Override    public void add(Item item, GeoPoint location) {/* ... */}
}

Code Smell

Feature Envy

Inside RectangleContainer#fitsInside(GridShape shape, GeoPoint location) is the following line:

Collection<GeoPoint> relativeShape = shape.getRelativeShape(location);

The relativeShape gets passed into hasIntersection and isInBounds of RectangleContainer:

@Override
public boolean fitsInside(GridShape shape, GeoPoint location) {
  Collection<GeoPoint> relativeShape = shape.getRelativeShape(location);
  boolean hasIntersections = hasIntersection(relativeShape);
  boolean isInBounds = isInBounds(relativeShape);
  return !hasIntersections && isInBounds;
}

Because RectangleContainer works with the internals of shape (relativeShape) it is a feature envy.

You could return a custom object PositionedItem that has the methods intersectsNotInside(inventory) and isInBoundOf(inventory):

@Override
public boolean fitsInside(Item item, GeoPoint location) {
   PositionedItem positionedItem = shape.at(location);
   return !positionedItem.intersectsNotInside(this) && positionedItem.isInBoundOf(this);
}

The benefit will be that you will not have duplicate logic for hasIntersections and isInBounds in the different subtypes of GridContainer.

Redundant Public

In both interfaces GridContainer and GridShape you have used the key word public. Per default are fields in an interface are public.

Also valid java:

public interface GridShape {
    boolean fitsInside(GridContainer container, GeoPoint pointer);
    Collection<GeoPoint> getShape();
    GeoPoint getPointer();
    Collection<GeoPoint> getRelativeShape(GeoPoint point);
}

Everything is addable

@Override
public void add(GridShape shape, GeoPoint location) {
   content.put(location, shape);
}

I could add a shape, that is 1000 times bigger as a RectangleContainer..

The adding of a shape should be checked so that it is not possible to at wrong shapes

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for spending time on this review! I'm so glad to learn so many things from you! \$\endgroup\$ – Martin Frank Nov 13 at 19:12

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.