Ggthjy

I have this algorithm that generates waypoints to fly over a specified area.
I would like to get some feedback to the coding inside the method, not about the structure of interfaces, I think this is pretty good here.
Despite any feedback is appreciated:)

public class RectangleWaypointGenerator implements WaypointGenerationAlgorithm {

    public List<waypoints.Waypoint> addAutoGeneratedWaypoints(List<Waypoint> polygon_waypoints) {

        List<Waypoint> waypoints = new ArrayList<>();

        AreaTester tester = new GPSPolygonTester(polygon_waypoints);

        GPSCoordinateCalculator coordinateCalculator = new GPSCoordinateCalculator();

        CustomGPSMapper mapper = new CustomGPSMapper();



        boolean finish = false;

        String mode = "r";

        Waypoint prev = polygon_waypoints.get(0);

        double toRight = coordinateCalculator.calculateAngleBetweenWaypoints(polygon_waypoints.get(1).getPosition().getLatitude(), polygon_waypoints.get(1).getPosition().getLongitude(),

                polygon_waypoints.get(2).getPosition().getLatitude(), polygon_waypoints.get(2).getPosition().getLongitude());

        double toLeft = coordinateCalculator.calculateAngleBetweenWaypoints(polygon_waypoints.get(2).getPosition().getLatitude(), polygon_waypoints.get(2).getPosition().getLongitude(),

                polygon_waypoints.get(1).getPosition().getLatitude(), polygon_waypoints.get(1).getPosition().getLongitude());



        double toUpRight = coordinateCalculator.calculateAngleBetweenWaypoints(polygon_waypoints.get(3).getPosition().getLatitude(), polygon_waypoints.get(3).getPosition().getLongitude(),

                polygon_waypoints.get(2).getPosition().getLatitude(), polygon_waypoints.get(2).getPosition().getLongitude());

        double toUpLeft = coordinateCalculator.calculateAngleBetweenWaypoints(polygon_waypoints.get(0).getPosition().getLatitude(), polygon_waypoints.get(0).getPosition().getLongitude(),

                polygon_waypoints.get(1).getPosition().getLatitude(), polygon_waypoints.get(1).getPosition().getLongitude());

        double distanceDown = mapper.distanceInKmBetweenGPSCoordinates(polygon_waypoints.get(2), polygon_waypoints.get(3));

        double travelledDown = 0;

        while (!finish) {

            if (mode.equals("r")) {

                double[] nextA = coordinateCalculator.movePoint(prev.getPosition().getLatitude(), prev.getPosition().getLongitude(), 15.0, toRight);

                Waypoint next = new DefaultWaypoint(nextA[0], nextA[1]);

                if (tester.isInsideArea(next)) {

                    waypoints.add(next);

                    prev = next;

                } else {

                    mode = "l";

                    double[] nextB = coordinateCalculator.movePoint(prev.getPosition().getLatitude(), prev.getPosition().getLongitude(), 10, toUpRight);

                    Waypoint next2 = new DefaultWaypoint(nextB[0], nextB[1]);

                    waypoints.add(next2);

                    travelledDown += mapper.distanceInKmBetweenGPSCoordinates(prev, next);

                    prev = next2;

                }

            } else {

                double[] nextA = coordinateCalculator.movePoint(prev.getPosition().getLatitude(), prev.getPosition().getLongitude(), 15, toLeft);

                Waypoint next = new DefaultWaypoint(nextA[0], nextA[1]);

                if (tester.isInsideArea(next)) {

                    waypoints.add(next);

                    prev = next;

                } else {

                    mode = "r";

                    double[] nextB = coordinateCalculator.movePoint(prev.getPosition().getLatitude(), prev.getPosition().getLongitude(), 10, toUpLeft);

                    Waypoint next2 = new DefaultWaypoint(nextB[0], nextB[1]);

                    waypoints.add(next2);

                    travelledDown += mapper.distanceInKmBetweenGPSCoordinates(prev, next);

                    prev = next2;

                }

            }

            if (travelledDown >= distanceDown) {

                finish = true;

            }

        }



       return waypoints.stream().map(p -> new waypoints.Waypoint(p)).collect(Collectors.toList());

    }

}

Without doing a full re-write:

Are the methods in your helper classes static? Are they used elsewhere? Can they be made static in their home class? You may want to statically import these helper classes if they are not holding state.

You have a lot of indirection (a.b.c()) fetching the same objects repeatedly - consider putting these in variables before you use them. this will clean up the code and might reveal another method you can extract

Is there any reason movePoint() cannot just return a Waypoint?

There is a lot of common code in the if and else clauses - can this be extracted as a single method that takes the single letter flag?

You can just assign the finishing condition check to the finish variable and get rid of the if clause.

Overall - try to aim for a top level method that 'describes the algorithm' then use subordinate methods to take care of sub-operations.

Instead of using a variable 'finish', put the condition inside the loop:

do {

...

} while (travelledDown >= distanceDown)

Your modes of "r" and "l" aren't very descriptive. You should use an ENUM here (Or atleast rename r/l if you don't want an ENUM)

public Enum Mode

{

    LEFT("L"),

    RIGHT("R")

}

mode = Mode.RIGHT

if (mode == Mode.RIGHT)

The last line looks a little odd but you explained why. I'd suggest adding a comment in the code.

//

return waypoints.stream().map(p -> new waypoints.Waypoint(p)).collect(Collectors.toList());

'nextA nextB next1 next2' should be renamed to be more meaningful