3
\$\begingroup\$

I am relatively new to Java, but have been reading up on how to improve the quality of my code.

I am writing a system where I take in a series of points from a file with x and y co-ordinates and by checking slopes relative to each other, am calculating and drawing a line if there is four or more co-linear points on the line. The code works and is doing what is required, but I would like to tidy up this method by reducing the if statements, but am not sure on how to, or if it is even necessary. The parameters being passed in is an array of doubles, which are slopes relative to an origin. I am iterating this array to look for 3 or more equal slopes in a row which would give a line and adding the corresponding points and any further equal points to a set. Once a line has been found, the rest of the slopes in array are checked in similar fashion. Any advice would be appreciated.

I think it's messy, but not sure how to rectify it.

public static void checkForLines(Double[] arrayOfSlopes, Point[] tempArray)
{
    int count = 0; // Keeps track of number of duplicates
    int i = 1;

    while (i < arrayOfSlopes.length - 1)
    {
        Double first = arrayOfSlopes[i];
        Double next = arrayOfSlopes[i + 1];

        if (first.equals(next))
            // Compares two consecutive indexes in array for equality
        {
            count = count + 1; // If match found increase the count

            if (count == 2)
                // At this point a line has been found
            {
                SortedSet<Point> line = new TreeSet<Point>();
                //Create a set to store points from this line
                line.add(tempArray[0]);
                line.add(tempArray[i - 1]);
                line.add(tempArray[i]);
                line.add(tempArray[i + 1]);
                //Store four found points

                int j = i + 1;

                while (j + 1 < arrayOfSlopes.length - 1 && (arrayOfSlopes[j].equals(arrayOfSlopes[j + 1])))
                {
                    line.add(tempArray[j + 1]);
                    //Store any other further duplicates if exist
                    j++;
                }

                if (arrayOfSlopes[j].isInfinite() && arrayOfSlopes[arrayOfSlopes.length - 1].isInfinite() ||
                    arrayOfSlopes[arrayOfSlopes.length - 1].equals(first))
                {
                    line.add(tempArray[tempArray.length - 1]);
                    //Check the last element for equality
                }


                if (lines.isEmpty())
                {
                    lines.add(line);
                }

                else
                {
                    lines.add(0, line);
                    ;
                    weedDuplicates(line);
                }
                i = j + 1; //Continue iteration from this point
            }

            if (count < 2)
            {
                i++;
            }
        }

        else if (first.equals(next) == false)
        {
            count = 0;
            i++;
        }
    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Well, to start, you should indent/format your code properly/consistently. \$\endgroup\$ – Zong Zheng Li Nov 19 '13 at 16:35
  • 1
    \$\begingroup\$ You should give your question a more descriptive title - even simply that it looks for lines in a set of points. \$\endgroup\$ – Hannele Nov 19 '13 at 16:46
  • 1
    \$\begingroup\$ Small thing, but nobody's mentioned it yet - I'd replace count = count + 1; with either count++; or maybe ++count;. (Some people prefer the latter, and in some cases it makes for slightly more efficient code. Shouldn't make a difference here though.) Also, instances of if(___ == false) should be replaced with if(!___). \$\endgroup\$ – Darrel Hoffman Nov 19 '13 at 23:42
  • \$\begingroup\$ looks like the task is an assignment from Coursera's "Algorithms and Data Structures, part 1" — class.coursera.org/algs4partI-004/assignment/… \$\endgroup\$ – Sarge Borsch Apr 5 '14 at 7:33
3
\$\begingroup\$

My first observation is that there are too many "This code is doing X" comments. If you feel the need to put those comments in, try refactoring the code out into a descriptive function.

if (concurrentPointsAreEqual(first, next))
{
    ...
}

private bool concurrentPointsAreEqual(Double first, Double second)
{
    return first.equals(second);
}

This will remove the comments and portray the intent right in the code.

I would also rename count to numberOfDuplicates thus removing the need for comments on it, and it will be more descriptive in your code.

This line else if (first.equals(next) == false) is redundent. An else will capture this condition because first is either equal or not equal to next.

I believe in Java, the standard is to put the opening bracket on the same line as the statement

if (concurrentPointsAreEqual(first, next)) {

I don't like the variable name tempArray, it tells me nothing of the data it is storing. Maybe something like pointsInLine.

Similar to the if statement I mentioned, extracting the points should be moved to its own function:

private TreeSet<Point> extractLineFromPoint(Point[] points) {

    TreeSet<Point> line = new TreeSet<Point>();

    line.add(points[0]);
    line.add(points[1]);
    line.add(points[2]);
    line.add(points[3]);

    return line;
}

You can then grab a sub-list from the original points an pass it in, removing the comments from the original call.

Your loops freak me out, especially with all the + 1, and increments on the loop counters. I'm not sure why its being done, or how to fix them, they just smell really bad.

I'm not sure what the weedDuplicates function does, but I think you should just be adding everything to the lines, then removing duplicates, otherwise you could potentially be iterating the lines list every iteration of the main loop. This will also remove one of the if statements.

\$\endgroup\$
  • \$\begingroup\$ Okay, thanks I will edit the code. Thanks for your help. \$\endgroup\$ – PhilipGough Nov 19 '13 at 17:20
  • \$\begingroup\$ Good answer, just one thing that bothers me: Java method names should start with lowercase letters as per the coding convention! :) \$\endgroup\$ – Simon Forsberg Nov 20 '13 at 0:31
  • \$\begingroup\$ Good point, leave it to a mainly C# to capitalize the methods. More than likely it was Resharper (I used it to reformat the code) ;) \$\endgroup\$ – Jeff Vanzella Nov 20 '13 at 16:38
5
\$\begingroup\$

The easiest way to flatten this kind of a structure is to reverse the sense of the if. That is, where you have

if (first.equals(next)) {
    // do stuff
}

Instead write

if (!first.equals(next)) {
    continue;
}
// do stuff

This only works where you don't have an else clause (so unfortunately I chose a bad example), but it can work wonders in bringing your code closer to the left margin.

From there I would start seeing where I could extract methods, especially but not exclusively where there is duplication in the code. The block of code beginning with

SortedSet<Point> line = new TreeSet<Point>();

looks like a good candidate for this.

\$\endgroup\$
3
\$\begingroup\$

Several comments

1) according to java coding conventions we write { at the same line where operator is, i.e.

if () {
}

and not

if () 
{
{

2) If you do not have special reason to use primitive wrapper (e.g. Double) use primitive double

The next comments are not about java but about general programming style

3) avoid writing so complicated conditions into while loop 4) try to simplify conditions. I think that list.add(0, element) works exactly like list.add(element) when list is empty. Therefore code:

                        if (lines.isEmpty())
                        {

                          lines.add(line);
                        }

                        else
                         {
                            lines.add(0, line);;
                            weedDuplicates( line);
                         }

Can be re-written as:

if (!lines.isEmpty()) {
    weedDuplicates( line);
}
lines.add(0, line);;

5) the code

if (first.equals(next) == false)

is the same as:

if (!first.equals(next))

Well, I frankly speaking I did not understand exactly what are you doing in this code fragment but probably the algorithm can be simplified too.

\$\endgroup\$
  • 1
    \$\begingroup\$ Also im using Double because Im returning infinity in some instances \$\endgroup\$ – PhilipGough Nov 19 '13 at 16:54
2
\$\begingroup\$

I'm not convinced that the code does what you claim.

First, let's clarify the parameters. Is there a relationship between arrayOfSlopes and tempArray? Your description suggests that arrayOfSlopes can be derived from tempArray, with each arrayOfSlopes[i] being the slope of a line between the origin and tempArray[i]. If that is so, then your function should only take a Point[] argument, named better (points will do for a name). This being a public function, you should make the interface as caller-friendly as possible, and requiring the caller to pass in two arrays that obey some unwritten consistency rule between them is inappropriate.

It appears that arrayOfSlopes and tempArray must follow rules about the ordering of their elements — elements must be sorted by slope? That's not obvious from the non-existent JavaDoc. You also treat tempArray[0] specially, though I can't tell why.

Next, what is lines? It appears to be a static class variable (it's the only way I can imagine to make this code compile). Putting the answer in a class variable makes no sense. Either you return lines as a result from this function, or you make this code a non-public instance method and store the result in an instance variable.

If arrayOfSlopes contains the slope of each point relative to the origin, then matching slopes will only yield lines that pass through the origin.

Does Point refer to java.awt.Point? You store them in a SortedSet, though, which means that they must be Comparable to each other. How is the comparison function defined?

Your else if is redundant — a simple else would be equivalent.

\$\endgroup\$
  • \$\begingroup\$ Yeah this is only one method that I wanted to clean up from all of the classes in the project, hence the statement "The code works and is doing what is required, but I would like to tidy up this method by reducing the if statements". So having already stated the code works and does what is required, why are you not convinced the code works and does what is required? \$\endgroup\$ – PhilipGough Nov 19 '13 at 18:21
  • \$\begingroup\$ Skeptical because 1) You only find lines that intersect the origin — a limitation that was not apparent in your description; 2) The parameters have to satisfy surprisingly specific preconditions that aren't documented; 3) tempArray[0] is mysteriously special; 4) You haven't provided complete runnable code with test cases to prove that it works. \$\endgroup\$ – 200_success Nov 19 '13 at 18:47
  • \$\begingroup\$ No but I have said it works, tempArray[0] is where I have set the origin to. If a number of points make the same slope with a point they are on the one line and collinear, correct? Hence by checking for consecutive equal slopes of the sorted array Im finding points on that line. I didn't say the code was perfect thats why I was looking for help. I did however say it works and also that I am new to programming. So be skeptical all you want, I wasn't asking anyone did they think the code worked, I asked how I could tidy it up. A point the two previous answers helped me with. \$\endgroup\$ – PhilipGough Nov 19 '13 at 18:54
  • \$\begingroup\$ In that case, my review boils down to this: your code is fragile because it relies on many hidden assumptions. Either document all of those preconditions, or better yet, find a way to relieve the caller from having to satisfy them. (Please don't be offended by the review. If I really thought your code was broken, I would have nominated the question to be closed for being non-working code instead of reviewing it.) \$\endgroup\$ – 200_success Nov 19 '13 at 19:04

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.