4
\$\begingroup\$

I have implemented the following methods in 1 and 2 and need feedback to improve my implementation. Any different approach and ideas are welcome.

  1. A static method, minRectangle, accepts an array of rectangles (objects of type Rectangle) and returns the rectangle with the smallest area. Create this method.
  2. A static method, selectRectangles, accepts an array of rectangles (objects of type Rectangle) and a color (an object of type String), and returns those rectangles (as an array) that are of the given color. Create this method.
  3. Create an array of rectangles (objects of type Rectangle). Then use the methods selectRectangles and minRectangle to determine the smallest of the rectangles in the array that are yellow.
public class Rectangle {

// the lengths of the rectangle’s sides
private double length;
private double width;

// the rectangle’s color
private String color;

public Rectangle(double length, double width, String color) {
    this.length = length;
    this.width = width;
    this.color = color;
}

public String getColor() {
    return color;
}

public double area() {
    return length * width;

}

public String toString() {
    return "<" + this.length + ", " + this.width + "," + this.color + ">";

}

public static Rectangle minRectangle(Rectangle[] rect) {

    Rectangle min = rect[0];
    for (int i = 0; i < rect.length; i++) {
        if (rect[i].area() < min.area()) {
            min = rect[i];
        }
    }
    return min;

}

public static Rectangle[] selectRectangles(Rectangle[] rect, String color) {
    int countRect = 0;
    for (int i = 0; i < rect.length; i++) {
        if (rect[i].getColor().equals(color))
            countRect++;

    }

    Rectangle[] Rect = new Rectangle[countRect];
    int companionVar = 0;
    for (int j = 0; j < rect.length; j++) {
        if(rect[j].getColor().equals(color))
        Rect[companionVar++] = rect[j];


    }
    return Rect;
}

public static void main(String[] args) {
    Rectangle[] rect = {new Rectangle(2, 3, "yellow"),
            new Rectangle(3, 5, "red"),
            new Rectangle(1, 1, "yellow"),
            new Rectangle(2, 2, "red"),
            new Rectangle(3, 4, "red"),
            new Rectangle(2, 8, "red"),
            new Rectangle(5, 2, "yellow")};

    System.out.println("Smallest yellow Rectangle:" + minRectangle(rect));
    Rectangle[] rectangles = selectRectangles(rect, "yellow");
    System.out.println(Arrays.toString(rectangles));
    }

}
\$\endgroup\$
4
\$\begingroup\$

The minRectangle is doing what it should but could be improved by storing the value of area() in a variable. Assume you have 1.000.000 rectangles to check then the calculation would happen 2.000.000 times.

public static Rectangle minRectangle(Rectangle[] rect) {

    Rectangle min = rect[0];
    double minArea = min.area();

    for (int i = 0; i < rect.length; i++) {
        double currentArea = rect[i].area();
        if (currentArea < minArea) {
            min = rect[i];
            minArea = currentArea; 
        }
    }
    return min;
}

The selectRectangles() method is doing too much work as well. By using an List<Rectangle> to add the rectangles with the desired color and returning the list as array will speed this up.

Please don't omit braces {} although they might be optional. Omitting braces can lead to hidden and therfor hard to track bugs.

public static Rectangle[] selectRectangles(Rectangle[] rect, String color) {
    List<Rectangle> rectangles = new ArrayList<>();
    int countRect = 0;
    for (int i = 0; i < rect.length; i++) {
        if (rect[i].getColor().equals(color)) {
            rectangles.add(rect[i]);
        }
    }

    Rectangle[] results = new Rectangle[rectangles.size()];

    return rectangles.toArray(results);
}

The last task in question isn't solved correctly. You should first get the yellow rectangles in the array and then detect the smallest of these.

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

private variables

As suggested by @Heslacher, you can store the area as a private variable as well to avoid having to re-calculate each time area() is called. Do be careful about overflow, so you may want to use something like BigDecimal for that:

public class Rectangle {
    private final double length;
    private final double width;
    private final String color;
    private final BigDecimal area;

    public Rectangle(double length, double width, String color) {
        if (!Double.isFinite(length) || !Double.isFinite(width) || color == null) {
            throw new IllegalArgumentException(
                            "Length, width or color defined incorrectly.");
        }
        this.length = length;
        this.width = width;
        this.color = color;
        this.area = BigDecimal.valueOf(length).multiply(BigDecimal.valueOf(width));
    }
}

This performs some basic validation on the constructor's parameters too.

Java 8

Maybe you are restricted by the programming challenge rules, but it's good to know how much simpler Java 8 can make it:

public static Rectangle getSmallest(String color, Rectangle... rectangles) {
    return Arrays.stream(rectangles)
                .filter(r -> r.getColor().equals(color))
                .min(Comparator.comparing(Rectangle::area))
                .orElse(null); // return null if none found (unlikely?)
}

I have rearranged the parameters so that the Rectangle array can alternatively be passed in as the varargs form.

Here, we call on the Stream.min(Comparator) method to perform the comparison for us, and the Comparator is constructed by using Comparator.comparing(Function) with Rectangle::area as a method reference.

I also think question (3) is expecting a Rectangle to be returned as the result, not a Rectangle[] array.

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