Detecting overlapped boxes and the color of overlapped area in a View

Question

Recently, I applied for a job that required skills in object-oriented programming. Although I have coded in Java at odd times, those were very small assignments. I have been mostly programming in C language. Recently I applied for a job that required object-oriented programming skills. They gave me a pre-interview assessment to find out over overlapped boxes in a view. Following is what they asked me to code:

Imagine you are working in the team responsible for rendering charts. A chart represents an area on a map (similar to Google maps). For the purpose of this exercise assume that a map consists of multiple charts drawn on top of each other, similar to the picture below.

Assume a chart has the following attributes. • Position in space and size is specified by its top left coordinate (x1,y1) and its bottom right coordinate (x2,y2). • Colour of the chart is specified in RGB, with each value between 0 – 255 (e.g. R=255, G=0, B=0 would imply a red chart)

Write a class called Chart that represents the above. B. Write a class called View that can contain a maximum of 2 charts at a time C. Implement a method DoChartsOverlap() in the View class which checks if the charts overlap. D. Implement a method GetColour(X,Y) in the View class that will return the RGB colour of a given coordinate. If two charts overlap the colour of the point should be the average of the two colours. You may assume the X and Y axis starts at 0 and has a maximum value of 100 for all of the above tasks.

Following was my solution that I submitted. It passed unit tests I wrote for it.

Chart Class:

package Foo;

import java.awt.Color;
import java.awt.Point;

public class Chart {
    private Point topLeftCord = new Point();
    private Point bottomRightCord = new Point();
    private Color chartColor;

    public Chart(Color color, int topX, int topY, int bottomX, int bottomY ) {
        this.topLeftCord.x = topX;
        this.topLeftCord.y =  topY;
        this.bottomRightCord.x = bottomX;
        this.bottomRightCord.y = bottomY;
        this.chartColor = color;
    }

    public boolean hasCoordinate(int x, int y) {
        return x >= this.topLeftCord.x && x <= this.bottomRightCord.x &&
                y >= this.topLeftCord.y && y <= this.bottomRightCord.y;
    }

    public Point getTopLeftCord() {
        return topLeftCord;
    }

    public Point getBottomRightCord() {
        return bottomRightCord;
    }

    public Color getChartColor() {
        return chartColor;
    }
}

View Class

package Foo;

import java.awt.Color;

public class View {

    private final int BG_COLOR = 0xFFFFFF;
    private Chart c1;
    private Chart c2;
    private Color bgColor;

    public View(Chart chart1, Chart chart2) {
        c1 = chart1;
        c2 = chart2;                
        bgColor = new Color(BG_COLOR); //set background color to white
    }

    public boolean doChartsOverLap() {
        if(c1.getTopLeftCord().x > c2.getBottomRightCord().x || // if true, c1 is located on the right side of c2
           c1.getBottomRightCord().x < c2.getTopLeftCord().x || // if true, c1 is located on the left side of c2
           c1.getTopLeftCord().y > c2.getBottomRightCord().y || // if true, c1 is located on the under the c2
           c1.getBottomRightCord().y < c2.getTopLeftCord().y) // if true, c1 is located on the over the c2      
                return false;
        return true;
    }

    public Color getColor(int x, int y) {
        Color color = null;

        if(c1.hasCoordinate(x, y) && c2.hasCoordinate(x, y)) { // The coordinate lies in an overlapped area
            int r = 0, g = 0, b = 0;
            r = (c1.getChartColor().getRed() + c2.getChartColor().getRed()) / 2;
            g = (c1.getChartColor().getGreen() + c2.getChartColor().getGreen()) / 2;
            b = (c1.getChartColor().getBlue() + c2.getChartColor().getBlue()) / 2;

            color = new Color(r, g, b);
        }
        else if(c1.hasCoordinate(x, y))  
            color = c1.getChartColor();
        else if(c2.hasCoordinate(x, y))
            color = c2.getChartColor();
        else
            color = bgColor; // The coordinates lies outside the area occupied by the charts, 
                             // thus returning default color of the View. However this will be invalid
                             // if there is another chart occupying this area.  

        return color;
    }
}

Main class:

package Foo;

import java.awt.Color;

public class Foo {

    public static void main(String[] args) {

        Chart c1 = new Chart((new Color(255, 0, 0)), 10, 10, 50, 50);
        Chart c2 = new Chart(new Color(0, 255, 0), 10, 10, 50, 50);

        View view = new View(c1, c2);

        System.out.println(c1.hasCoordinate(60, 60)); // just to test
        System.out.println(view.doChartsOverLap());
        System.out.println(view.getColor(50, 50));
    }

}

After a week, I have got a vague reply that "It does not appear that you have got strong application-level skills". Is this code that bad? Can you point out improvements in this code? Have I missed out on some important object oriented concepts?

Any help would be appreciated. I need it for closure.

Answer 1

First of all, it is great that you wrote this algorithm and that it works. I'll try to collect all remarks that I would normally note while reviewing this code having the formal task.

Package naming

According to Java style guides you are not allowed to name a package using capitals letters. The correct package name will be foo. However, foo is an extremely bad package name. It doesn't carry any meaningful information. Taking into account that you are given a charts API your package could have been com.yourcompanyname.charts.

Why do you use java.awt API?

The task doesn't say that you should rely on the java.awt API. It might be that your app will expose its API through HTTP, have a CLI API, or have no UI. Using these APIs you explicitly force clients to stick to java.awt.

Define your own domain classes

Instead of the provided Color and Point, you could use your specific implementations that are designed to solve your business task.

Initialization of points

Initialization of points topLeftCord, bottomRightCor is broken in two steps. You assign a point in the field declaration and you set up coordinates in the constructor. Instead, it is better to perform initialization in one place.

Boolean operations

The following considered a code smell:

if (condition) {
    return false;
}
return true;

Instead you can just:

return !condition;

Too much comments

Often if you face a need to explain the code you are writing in a comment, it means that you can extract/structure your code in the way that your code will be self-explaining. Try to break up your complex conditions into methods/classes and use them so that a reader won't have to 'parse' blocks of 'if' conditions that carry 5 or more statements.

Immutability

Many places in your code are not designed to be mutable. For instance, points and color in the Chart. Make them final.

Color declaration

Why BG_COLOR is a constant? Why do you need this declaration in the first place? I would remove it. Also, the name violates the Java style convention.

Do not create a main class

Instead, you are supposed to cover your API with unit tests that prove that it works the way it is defined in the tasks. Not to say, it shouldn't be called Foo.

Answer 2

There are a few holes in the specification for which you have implemented code. E.g. the color outside the charts. The picture has white color but you used black. If you didn't ask clarification for those, that might have a negative effect on your evaluation.

On the other hand, the specification says that the view can have "maximum of two charts" meaning that it can also have zero or one charts. You did not account for those and introduced NullPointerExceptions.

You can always thank them for the opportunity and what specifically made them consider you to not have "strong application-level skills" (whatever that means)...

Answer 3

I like the code, what you can do more is to add interfaces. Lets say Chart can be an interface and RectangularChart implement it and used in your application. In main you will declare variable as Chart and instantiate with RectangularChart. Also don't be afraid to try to other jobs, if you trust yourself then don't pay attentions to the answer(this can hide other things in back: they like other candidates, they don't like you because you are smart ...). Think lie this: your story doesn't fit to their story, go on ;)