current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 10 down vote favorite
2

This is a Java method that takes as input a 2 dimensional array called grid.

It uses the i1 and j1 values from when the corresponding grid[i1][j1] position is true for the first time, and then i2 and j2 from when the corresponding grid[i2][j2] position is true for the second time.

A call is then made to drawLine(i1, j1, i2, j2) to draw line between those two points in the screen.

I wrote the following code to take all stuff I want to plot, but the code works but looks bad, is there any better way to write this. This code works fine, I just want to improve on it.

    for(int x1 = 0; x1 < rows; x1++) { 
        for (int y1 = 0; y1 < columns; y1++) { 
            if (grid[x1][y1] == true) { 
                int x2 = x1 + 1; 
                for(int y2 = 0; y2 < 500; y2++) { 
                    if (grid[x2][y2] == true) { 
                        drawLine(x1, y1, x2, y2); 
                        break; 
                    } 
                } 
            } 
        } 
    }
share|improve this question

migrated from stackoverflow.com Oct 15 '12 at 13:33

This question came from our site for professional and enthusiast programmers.
    
Watever language the above code is, Its a small piece of code so performance wont matter much unless you are providing really large data. However, declaring a integer inside two for loops may decrease the performance. – Anil Oct 12 '12 at 0:55
3  
Also, what's the significance of that "magic number" 500. You should declare this as a well-made constant so that its meaning is clear (and so it can be re-used more easily without needing to update its value in more than one place). – Sepster Oct 12 '12 at 0:56
    
Your break statement breaks out of only the inner-most for loop. Do you intend for it to continue looking for other lines to draw? – phatfingers Oct 13 '12 at 3:47
up vote 3 down vote accepted

My implementation:

(Is C#.Net but you'll be able to translate easily).

    public class Point()
    {
        public int X {get; set;}
        public int Y {get; set;}

        public Point (int x, int y)
        {
            this.X=x;
            this.Y=y;
        }
    }

    public void calcLine(bool[][] grid)
    {
        int rows = 500;
        int columns = 500;

        Point p1 = new Point(0, 0);
        // Change this outer 'if' to 'while' if should continue searching for all lines to draw
        if (canFindNextPoint(grid, rows, columns, p1))
        {
            Point p2 = new Point(p1.X + 1, 0);
            if (canFindNextPoint(grid, rows, columns, p2))
            {
                drawline(p1.X, p1.Y, p2.Y, p2.Y);
            }
        }
    }

    private bool canFindNextPoint(bool[][] grid, int rows, int columns, Point p)
    {
        bool found = false;
        for (int x = p.X; x < rows; x++)
            for (int y = p.Y; y < columns; y++)
            if (grid[x][y])
            {
                found = true;
                p.X = x;
                p.Y = y;
                break;
            }
        return found;
    }
share|improve this answer
    
Which is actually pretty much a concrete implementation of @Algebra's answer, I think. – Sepster Oct 12 '12 at 1:29
    
I understood your idea but I cannot write this in Java as I cannot pass parameters by reference in Java? like is there any "passing parameters by value" alternative of this code? – user1739762 Oct 12 '12 at 1:54
    
Ouch. A cleaner approach would be to wrap your x,y values as a "Point" object, with x and y properties. You can then pass your "point" in (and while the reference to your point object will be passed as a by-value parameter, you'll still be able to work on the point object that is referenced, and its new values will be available to the calling scope. – Sepster Oct 12 '12 at 2:12
    
@user1739762 Have updated answer to reflect this suggestion. Thanks for the accept! – Sepster Oct 12 '12 at 2:17
    
Thank you, I really appreciate your effort. – user1739762 Oct 12 '12 at 2:30
up vote 2 down vote

I would create a support method that knows to look for the next true value in the grid

void FindTrue(int &x, int &y, bool[][] grid)
{
    for(; x < rows; x++, y = 0)
        for(; y < columns; y++)
            if (grid[x][y]) return;
}

now your method can call this method like this:

  1. With x=0,y=0 and after that store them to x1, x2.
  2. Advance x, y by one locating (may need to drop a line).
  3. With untouched x, y.
  4. Call drawLine(x1,y1,x,y)

consider this pseudo code cuz I am not fluent in java.

share|improve this answer
    
+1. NB my answer is roughly a concrete (albeit C#.Net, not java) implementation of this answer. – Sepster Oct 12 '12 at 1:39
up vote 0 down vote

try the while loop

while(true) {
     int x1 = 0;
     if(x1 < rows) break;

     for (int y1 = 0; y1 < columns; y1++) {    
       if (grid[x1][y1] == true) { 
          int x2 = x1 + 1;  
          for(int y2 = 0; y2 < 500; y2++) {   
             if (grid[x2][y2] == true) {  .
                drawLine(x1, y1, x2, y2); 
                break;    
             }
          }
       }
     }
     x1++;
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.