Methods and decomposition

Question

I'm just starting learning Java after a few years of HTML/CSS coding so hopefully I'm not asking a old or stupid question here but any help explaining this problem would be very much appreciated.

I'm currently working through the Stanford CS106A online material and I've reached week 6, Assignment 2, Question 3 (http://see.stanford.edu/materials/icspmcs106a/13-assignment-2-simple-java.pdf).

As you can see it requires the placement of various objects on the screen to create the Graphics Hierarchy, as described. My plan was to use the centre coordinates to relatively place all the objects on the screen. However I've hit a problem that I can't seem to find an answer to. The course describes how Method Decomposition should allow each method to handle one problem (Single Responsibility Principle, I believe) so I have written the first part of my code as such:

//Import any libraries
import acm.program.*;
import acm.graphics.*;

    public class GraphicsHierarchy extends GraphicsProgram {

//Define constants
static final int BOX_WIDTH = 200;
static final int BOX_HEIGHT = 75;



public void run() {
    placeGRect();
}   

//Find centre x & y
double centre_x = getWidth() / 2; //check this
double centre_y = getHeight() * 0.5;//and this

//placeGRect method
public void placeGRect() {
    for (int count = 0; count < 4; count++) {
        GRect box = new GRect (BOX_WIDTH, BOX_HEIGHT);
        add(box);
        switch (count) {
        case 0:
            box.setLocation(centre_x, 75);
            break;
        case 1:
            box.setLocation((centre_x * 0.5), 250);
            break;
        case 2:
            box.setLocation(centre_x, 250);
            break;
        case 3:
            box.setLocation((centre_x * 1.5), 250);
            break;
        }
    }
}
}

However this doesn't work due to the centre_x & centre_y producing zero values. I discovered this by changing the program to a ConsoleProgram and having the getWidth & getHeight lines inside the run() method (and println their values on screen), which then produced the required values but didn't pass them to the GRect method (so still didn't work). However if I have the getWidth/getHeight lines listed out of the run() then they don't produce any values for relative positioning.

My question is given that each method should handle one task and (as much as possible) methods should be defined out of the run() method, then how I can I get the getWidth/getHeight values to the placeGRect() method without having one big block of code within the run() method. Which I understand is bad practise.

I'm not after any code to solve this, I really need to understand the principles of this so I can write effective code in the future. I prefer understanding to parrot-fashion code copying.

Thanks in advance for any help.

Answer 1

In your specific example:

You have declared centre_x and centre_y as instance variables. When your program first creates an instance of GraphicsHierarchy the order of object creation is as such:

1. ClassLoader loads the class... static variables (BOX_WIDTH,BOX_HEIGHT) are assigned specified values;

2. Space is allocated on the heap for an instance of GraphicsHierarchy (enough space to hold the instance variables - a double for centre_x and a double for centre_y- including space for base class instance variables)

3. Instance variables are set to default values: centre_x = 0, centre_y = 0

4. The GraphicsHierarchy default constructor is called (which does nothing other than call the base class constructor - GraphicsProgram).

5. The base class will go through steps 1-4 and when it's finished execution returns to GraphicsHiearchy which now evaluates explicit instance variable initializers before executing any remaining constructor statements (which in the case of the default constructor, there are none).

(additional reference on this process http://java.dzone.com/articles/java-object-initialization)

Having said all of that, it would appear that when your class GraphicsHierarchy gets to step 5 and tries to assign values to centre_x and centre_y, the subsystem that getWidth and getHeight rely upon is not ready (i.e. a window or canvas has not been created yet, so the methods return 0). But when you moved your assignments inside run and getWidth/getHeight returned values, that would imply whatever method is calling run has first gone through the necessary window creation steps.

Etienne de Martel's suggestion is fine. It delays assignment of your centre values until right before they are needed. If you'd rather, you could create an init method and move the assignments inside the init method, and then call init as the first step of run

private void init() {
    centre_x = getWidth / 2;
    centre_y = getHeight * 0.5;
}

public void run() {
    init();
    placeGRect();
}

This is nearly the same thing as Martel's suggestion, although if you find later you have other initialization code that needs to happen, you can throw it in the same place.

As for crafting flexible code you might think about renaming placeGRect to placeGRects and passing in array of points (or Collection if you prefer) placeGRects(Point[] points)

(you can use Java.awt.Point or define your own Point class)

In this way, your placeGRects method is simplified. It no longer decides how many boxes to render (the array that is passed in does). It also doesn't determine where those new boxes are located (again the array of Point objects do). It simply loops through the size of the array, makes a new box, adds it, and sets the location.

private Point[] boxPoints;

public void run() {
    init();
    placeGRects(boxPoints);
}

public void placeGRects(Point[] points) {
    for(int i=0;i<points.length;i++) {
        GRect b = new GRect(BOX_WIDTH,BOX_HEIGHT); 
        add(b);
        b.setLocation(points[i].x,points[i].y);
    }
}

And you can put your Point array initialization inside your new init() method.

private void init() {
    centre_x = getWidth / 2;
    centre_y = getHeight * 0.5;
    boxPoints = {new Point(centre_x, 75),new Point(centre_x * 0.5, 250)}; 
}

It makes your code easier to understand and modify when needed.

Answer 2

Perhaps I don't understand your question, but why not pass them as parameters?

protected void placeGRect(double centre_x, double centre_y) {
    // ...
}

You can then call placeGRect like so:

public void run() {
    placeGRect(getWidth() / 2, getHeight() * 0.5);
}

Answer 3

Very nice question! How to compose your methods is a matter of intuition rather than strict guidelines.

Certainly, methods should be focused on doing one thing and one thing only. Firstly, having short methods (even one-liners!) improves the understandability of the code. As a very rough example, think of this:

if (DateUtils.before(ticket.getExpirationDate(), new Date())) {
   accept(ticket);
}

and then this

if (isNotExpired(ticket)) {
   accept(ticket);
}

...

private boolean isNotExpired(Ticket t) {
   return DateUtils.before(t.getExpirationDate(), now());
}

private Date now() {
  return (new Date());
}

Pay attention to how the introduction of one line methods isNotExpired() and now() significantly improved your undestanding of what the code does.

Here's another example, this time that has to do with constructing objects:

Loan l1 = new Loan(15000, 36, f7.2, 2.5);
Loan l2 = new Loan(15000, 36, f7.2);

vs.

Loan l1 = Loan.newSubsidizedLoan(15000, 36, f7.2, 2.5);
Loan l2 = Loan.newNormalLoan(15000, 36, f7.2);

Note in this example how wrapping the constructors in two different methods significantly improves the documentation of code (without even needing to write comments);

If you are interested on the general topic of coding style, you should read this book.

Cheers

L.

Answer 4

Your code doesn't seem to include the getWidth() and getHeight() methods. Also, the following piece of code is totally wrong as a placement and should be placed in a constructor:

double centre_x = getWidth() / 2; //check this
double centre_y = getHeight() * 0.5;//and this

Should become

private double centre_x;
private double centre_y; 
GraphicsHierarchy(){
    centre_x = GraphicsHierarchy.BOX_WIDTH / 2;
    centre_y = GraphicsHierarchy.BOX_HEIGHT * 0.5;
}

This code will at least compile, but consider the solution described below, which is even better.

Considering that you have defined BOX_WIDTH and BOX_HEIGHT as static variables, you can always find centre_x and centre_y. Therefore, you don't even need to define BOX_WIDTH and BOX_HEIGHT

You can define your class like this:

//Import any libraries
import acm.program.*;
import acm.graphics.*;

public class GraphicsHierarchy extends GraphicsProgram {
public void run() {
    placeGRect();
}   
//Define constants
public static final double CENTRE_X= 100.00; 
public static final double CENTRE_Y = 37.50;
//placeGRect method
public void placeGRect() {
    for (int count = 0; count < 4; count++) {
        GRect box = new GRect (200, 75);
        add(box);
        switch (count) {
        case 0:
            box.setLocation(GraphicsHierarchy.CENTRE_X, 75);
            break;
        case 1:
            box.setLocation((GraphicsHierarchy.CENTRE_X * 0.5), 250);
            break;
        case 2:
            box.setLocation(GraphicsHierarchy.CENTRE_X, 250);
            break;
        case 3:
            box.setLocation((GraphicsHierarchy.CENTRE_X * 1.5), 250);
            break;
        }
    }
}
}

In my opinion, you can go even further by eliminating all computations and replace such stuff

GraphicsHierarchy.CENTRE_X * 1.5 

with

150

Come on, have it easy on your Virtual Machine! Your class uses a whole load of static information, so there is no need for so much computation. But having a BOX_WIDTH and BOX_HEIGHT is completely useless as constants, as they are used only internally and only on one place. Calculating centre_x and centre_y out of the BOX_WIDTH and BOX_HEIGHT is also useless, as as they are final, you can easily do the computation yourself and reduce unnecessary creation of variables.

In addition, you don't use the centre_y value anywhere, so you should ditch it.

To further add some helpful advice, a decent IDE, like NetBeans, Eclipse, or IntellIJIDEA should have code completion and syntax highlighting and will help you immensely in becoming a better (or more knowledgable, which is even better) programmer.