2
\$\begingroup\$

I've been working on this mini project to teach myself how to better use the graphical functions and nested loops in Java. I have achieved my basic goal with this project to be able to draw on a screen pixel by pixel in blocks and diagonally.

The basic idea is that it draws on the screen pixel by pixel in two different ways, one is diagonally and one is block by block.

I'm looking for any way at all to improve my program without implementing code very much more advanced than the code used in the program so far. I'm really looking for any improvements at all. A way to make it more compact, faster running, basically anything at all to make it 'better'.

    /**
     * @author Samuel
     */
    package main;
import java.awt.Color;
import java.awt.Dimension;
import java.awt.Graphics;
import java.util.Random;
import javax.swing.JFrame;
@SuppressWarnings("serial")
public class Draw extends JFrame{
    public static int width = 1280;
    public static int height = (width/16)*9;
    int x = 3;
    int y = 25;
    Color randomColor = Color.BLACK;
    public Draw() {

        this.setPreferredSize(new Dimension(width,height));
        this.setResizable(false);
        this.setLocationRelativeTo(null);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        this.pack();
        this.setVisible(true);
    }
    @Override
    public void paint(Graphics g) {
        super.paint(g);
        while(true) {
            nextPixel2(); //Change to nextPixel() for block, nextPixel2 for diagonal
            g.setColor(randomColor);
            g.drawLine(x, y, x, y);
            pixels++;
        }
    }
    //250000~ timer value = 1 Second/Pixel
    int timer = 1;
    int pixels = 0; 
    int xLock = x;
    int xStart = x;  
    int yLock = y;
    int yStart = y;
    int blockSize = 50;
    int changeColor = 5; //Diagonal change color
    int i = 0;
    public void nextPixel2(){ //Diagonal
        x--;
        y++;
        for(int i = 0;i < timer;i++){
            i++;
            System.out.println("Pixels drawn: "+pixels);
        }
        while(x < xStart || y > height){
            xLock++;
            x=xLock;
            y=yLock;
            while(x > width) { 
                xLock--;
                x=xLock;
                yLock++;
                y=yLock;
            }
            if(y == height && x == width){
                xLock = xStart;
                x = xLock;
                yLock = yStart;
                y = yLock;
            }
            if(i < changeColor){ 
                i++;
            }else{
            generateColor();
            i=0;
            }
        }
    }
    public void nextPixel(){ //Blocks
        x++;
        pixels++;
            for(int i = 0;i < timer;i++){ 
                i++;
                System.out.println("Pixels drawn: "+pixels);
            }
            while(x-xLock >= blockSize){ 
                x=xLock;
                y++;
                while(y-yLock >= blockSize){ 
                    y=yLock;
                    xLock+= blockSize;
                    x=xLock;
                    while(xLock >= width){ 
                        yLock+= blockSize;
                        y=yLock-1;
                        xLock=xStart;
                        if(yLock >= height){ 
                            y = yStart;
                            yLock = yStart;
                            x = xStart;
                            xLock = xStart;
                        }
                    }
                generateColor();
                }
            }
        }
    public void generateColor(){ 
        Random rand = new Random(); 
        float red = rand.nextFloat();
        float green = rand.nextFloat();
        float blue = rand.nextFloat();
        randomColor = new Color(red, green, blue); 
    }
    public static void main(String[] args){
        Draw draw = new Draw();
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ Welcome to Code Review! Can you please add a description in your question about what your program does? We are all here to improve our code in one way or another, but what makes your question unique is what your code does. \$\endgroup\$ Oct 14 '14 at 18:04
  • \$\begingroup\$ @SimonAndréForsberg I did mention in my text that the program "draw on a screen pixel by pixel in blocks and diagonally.". Do I need to describe in detail what each individual piece of code does? \$\endgroup\$
    – Samy
    Oct 14 '14 at 19:10
  • \$\begingroup\$ Absolutely not. Just a generic description is fine. Sorry, it was just very easy to miss your description. \$\endgroup\$ Oct 14 '14 at 19:18
  • \$\begingroup\$ I'll make it clearer then. \$\endgroup\$
    – Samy
    Oct 14 '14 at 19:41
  • \$\begingroup\$ Much better, thanks! I would love to take a look at your code, but I am unable to do that today. I hope someone else will take a look at your code shortly. Welcome to Code Review! \$\endgroup\$ Oct 14 '14 at 20:00
3
\$\begingroup\$

Naming

Names of methods should describe the purpose they have. If a comment is needed to describe what a method does //Change to nextPixel() for block, nextPixel2 for diagonal, one should think about another more meaningful name.
What would Mr.Maintainer think at reading this code ?

@Override
public void paint(Graphics g) {
    super.paint(g);
    while(true) {
        nextPixel2(); //Change to nextPixel() for block, nextPixel2 for diagonal
        g.setColor(randomColor);
        g.drawLine(x, y, x, y);
        pixels++;
    }
}

He would get the conclusions

  1. I need to dig into the netxPixel2() method
  2. Inside this method the value of randomColor and a location (defined by x and y) will maybe be changed.
  3. What does the nextPixel2() do and why does it change 2 things
  4. There is another method nextPixel() involved, and I need to dig into this too
  5. This is a never ending while loop

Decoupling
As the pixel manipulation code only relies on a Graphics, there is no need for this code to be inside a JFrame extending class. You should better use composition over inheritence.

Style

The indention of the code can be improved a lot. Maybe some of this came from pasting here but for sure not this

if(i < changeColor){ 
    i++;
}else{
generateColor();
i=0;
}

Ok, let us dig into nextPixel2()

What do we see ?

Neither xStart nor yStart will be changed ever (after a quick glance into nextPixel()), so we should make them final and also give them a new name like e.g xBorderPosition

for(int i = 0;i < timer;i++){
   i++;
   System.out.println("Pixels drawn: "+pixels);
 }

This is just boilerplate code, as pixels isn't increased anywhere in the method, it stays at the initial value of 0, this should be removed, also because the responsibility of this method does not involve output of any data, but manipulating some values.

Algorithm

The x value will be decremented and the y value will be incremented. This will happen until

x < xStart or in other words until x == 2. At this point xLock will be incremented, x = xLock and y = 25 = yLock = yStart.

or

y > height which will be true after xLock == 699 so this will be the case we need to dig into

If the first condition is true and xLock will be incremented to xLock == 699 we start again with y = 25 = yLock = yStart and x = xLock = 699. After some iterations of the while(true) of the paint() method we will reach at the entrance to nextPixel() with the values x == 4 and y == 720 and after incrementing and decrementing we have x == 3 and y == 721. Se let us see what the following striped code will do then

while(x < xStart || y > height){
    xLock++; // now 700
    x=xLock; // now 700
    y=yLock; // now 25
    while(x > width) { // still false
        xLock--;
        x=xLock;
        yLock++;
        y=yLock;
    }
}

So the xLock == 699 border doesn't change anything as x is still <= width. But what we see is, if xLock would reach the value 1281 then x > width will be true and something will happen.

yLock will be incremented by one and xLock will become 1280 and the x > width won't be true again.

So as there is always only one iteration, we can replace this while loop with an if statement. So if anytime x > width we need to increment yLock by one and set xLock == width. And as we set x = xLock we can check if xLock > width and assign then the xLock and yLock values to x and y

Refactoring

while(x < xStart || y > height){
    xLock++;
    if (xLock > width) {
        yLock++;
        xLock = width;
    } 
    x = xLock;
    y = yLock;
    if(y == height && x == width){
        xLock = xStart;
        x = xLock;
        yLock = yStart;
        y = yLock;
    }
    if(i < changeColor){ 
        i++;
    }else{
        generateColor();
        i=0;
    }
}

As we have seen x == width will only be true, if xLock > width, we can check y == height inside this if statement.

Refactoring again

while(x < xStart || y > height){
    xLock++;
    if (xLock > width) {
        yLock++;
        xLock = width;
        if(yLock == height){
            xLock = xStart;
            yLock = yStart;
        }
    } 
    x = xLock;
    y = yLock;
    if(i < changeColor){ 
        i++;
    }else{
        generateColor();
        i=0;
    }
}  

If we look at the remaining while conditions we see, that after this condtions will be true the first time, neither x < xStart will be true nor y > height will be true. So there is no need for a while loop. To save an additional indention, we also revert the condition to be a guard condition. Additional we extract the check for generating a random color to a separate method isColorChangeNeeded(), rename the class variable i to colorCheckCounter and the changeColor variable to colorCheckLimit which should be final also. And finally we will rename the method to assignDiagonalPixelLocation

public void assignDiagonalPixelLocation(){ 
    x--;
    y++;
    if (x >= xStart && y <=height) { return; }

    xLock++;
    if (xLock > width) {
        yLock++;
        xLock = width;
        if(yLock == height){
            xLock = xStart;
            yLock = yStart;
        }
    } 
    x = xLock;
    y = yLock;
    if(isColorChangeNeeded()){ 
        generateColor();
    }
}

private boolean isColorChangeNeeded() {
    colorCheckCounter++;
    if (colorCheckCounter <= colorCheckLimit){ return false; }

    colorCheckCounter=0;
    return true;
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your reply! I agree with most things you said and I've now implemented most of it in my code, if you think of many more ways to improve the code or any smart feature that I should add feel free to let me know :) \$\endgroup\$
    – Samy
    Oct 15 '14 at 11:40
  • \$\begingroup\$ @Samy Feel free to ask a follow up question with your improved code. \$\endgroup\$
    – Heslacher
    Oct 15 '14 at 11:44

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.