2
\$\begingroup\$

This is exercise 3.2.23. from the book Computer Science An Interdisciplinary Approach by Sedgewick & Wayne:

Write a recursive Turtle client that draws dragon fractal.

The following is the data-type implementation for Turtle graphics from the book which I beautified:

public class Turtle {
    private double x;
    private double y;
    private double angle;

    public Turtle(double x, double y, double angle) {
        this.x = x;
        this.y = y;
        this.angle = angle;
    }
    public void turnLeft(double delta) {
        angle += delta;
    }
    public void goForward(double step) {
        double oldX = x, oldY = y;
        x += step * Math.cos(Math.toRadians(angle));
        y += step * Math.sin(Math.toRadians(angle));
        StdDraw.line(oldX, oldY, x, y);
    }
}

StdDraw is a simple API written by the authors of the book.

Here is my program:

public class Dragon {
    public static void drawDragonCurve(int n, double step, Turtle turtle) {
        if (n == 0) {
            turtle.goForward(step);
            return;
        }
        drawDragonCurve(n - 1, step, turtle);
        turtle.turnLeft(90);
        drawNodragCurve(n - 1, step, turtle);

    }
    public static void drawNodragCurve(int n, double step, Turtle turtle) {
        if (n == 0) {
            turtle.goForward(step);
            return;
        }
        drawDragonCurve(n - 1, step, turtle);
        turtle.turnLeft(-90);
        drawNodragCurve(n - 1, step, turtle);
    }
    public static void main(String[] args) {
        int n = Integer.parseInt(args[0]);
        double step = Double.parseDouble(args[1]);
        Turtle turtle = new Turtle(0.67, 0.5, 0);
        drawDragonCurve(n, step, turtle);
    }
}

I checked my program and it works. Here is one instance of it:

Input: 12 0.007

Output:

enter image description here

Is there any way that I can improve my program?

Thanks for your attention.

\$\endgroup\$

1 Answer 1

7
\$\begingroup\$

Whitespace

There should be a blank line between two method definitions.

Also, some blank lines inside of the methods would give the code room to breathe, and allow you to visually separate individual "steps" from each other, for example in your main method:

int n = Integer.parseInt(args[0]);
double step = Double.parseDouble(args[1]);
Turtle turtle = new Turtle(0.67, 0.5, 0);

drawDragonCurve(n, step, turtle);

Magic values

There are several magic values in your code. While it is probably clear what 90 means in a turtle graphic environment, the values 0.67 and 0.5 mean nothing to me. What are they? And more importantly: Why are they the values that they are? Why not 0.66 or 0.68?

Variables allow you to give explanatory, intention-revealing names to your values.

final var initialXCoordinate = 2.0 / 3.0;
final var initialYCoordinate = 0.5;
final var initialAngle = 0;
turtle = new Turtle(initialXCoordinate, initialYCoordinate, initialAngle);

Type inference

I am a big fan of type inference, especially in cases where the type is obvious:

double step = Double.parseDouble(args[1]);

How often do I have to told that this is a double? I get it!

var step = Double.parseDouble(args[1]);

Same here:

Turtle turtle = new Turtle(0.67, 0.5, 0);

This almost sounds like someone stuttering. I don't need the turtle variable to be explicitly annotated with the Turtle type to understand that a variable called turtle being initialized with a Turtle is probably a turtle:

var turtle = new Turtle(0.67, 0.5, 0);

final

I am also a big fan of making everything that can be made final explicitly final. And even for things that can't be made final as written, I'd investigate whether it can be rewritten so it can be made final.

Note, by "everything" I mean primarily variables and fields. However, unless a class is explicitly designed to be extended, it should also be marked final. And of course, immutable classes need to be final anyway.

private

None of your methods are used by any outside client (except for main), so they should all be private.

Naming

n is a terrible name. I have no idea what it means. I promise you, your keyboard is not going to wear out from making your variable names a little more descriptive. I believe it is referring to the order of the fractal.

Also, I have no idea what a Nodrag is.

Scope

It seems that step is always the same. There is no need to pass it as an argument if it is always the same. The purpose of a parameter is to allow a method to differ in its behavior. There is no need for it if it is never different.

The same applies to Turtle.

Code Duplication

This is the big one. There is a lot of code duplication. In fact, drawDragonCurve and drawNodragCurve are 100% identical except for one single character.

The only thing that is changing is the sign of the turn angle, or in other words, the direction of the turn. We can turn the sign into a parameter of the method, and pass it along accordingly.

I will keep the original drawDragonCurve method in place as an overload, which calls the new overload with the correct starting parameter, because a client should not need to know whether 1 or -1 is the correct starting value.

public class Dragon {
    private static double step;
    private static Turtle turtle;

    private static void drawDragonCurve(final int order) {
        drawDragonCurve(order - 1, 1);
    }

    private static void drawDragonCurve(final int order, final int sign) {
        if (order == 0) {
            turtle.goForward(step);
            return;
        }

        drawDragonCurve(order - 1, 1);
        turtle.turnLeft(sign * -90);
        drawDragonCurve(order - 1, -1);
    }

    public static void main(final String[] args) {
        final var order = Integer.parseInt(args[0]);
        step = Double.parseDouble(args[1]);

        final var initialXCoordinate = 2.0 / 3.0;
        final var initialYCoordinate = 0.5;
        final var initialAngle = 0;
        turtle = new Turtle(initialXCoordinate, initialYCoordinate, initialAngle);

        drawDragonCurve(order);
    }
}

Note that I might have made a mistake with the flipping of the sign. However, there is no way for me to test your code, because Dragon is intimately linked to Turtle and Turtle is intimately linked to StdDraw, which I don't have access to.

This brings me to my last point:

Testability, Modularity, Overall Design

  • All your methods are static, in other words, they aren't really methods at all, they are glorified procedures. There are no objects!
  • All your methods return void, in other words, they don't return anything, they purely perform side-effects.
  • All your classes are intricately linked to each other, there is no way to separate them, to use them independently, to test them independently.

It is hard to test code if I can't instantiate an object that I can test. It is hard to test code if everything is just a side-effect, and I can't simply compare return values. It is hard to test code if I always need the entire thing and can't test pieces independently or swap out pieces for test versions.

At the moment, the only way to test your code, is to run the entire thing, take a screenshot and compare it with a pre-recorded one. This adds a huge amount of complexity to the tests, and is very slow. Ideally, you want to be able to run your tests every couple of seconds.

For example, Dragon should be a true object, not a static class. It should have the dependency on Turtle injected, not hard-coded. The Turtle dependency should be abstracted behind an interface, so that, for testing purposes, I can replace the turtle with a version that records the commands and compares them to a pre-recorded sequence instead of drawing on the screen.

Likewise, StdDraw should be a true object as well, hidden behind an interface, and I should be able to instantiate a version that uses ASCII art instead of Swing, or that simply creates a bit matrix that I can compare with a pre-recorded one for testing.

At least, Turtle is a class that can actually be instantiated, and has behavior.

This is just a sketch:

App.java

class App {
    public static void main(final String[] args) {
        final var order = Integer.parseInt(args[0]);
        final var step = Double.parseDouble(args[1]);

        final var initialXCoordinate = 2.0 / 3.0;
        final var initialYCoordinate = 0.5;
        final var initialAngle = 0;
        final var turtle = new StdDrawTurtle(initialXCoordinate, initialYCoordinate, initialAngle);

        final var dragon = new Dragon(order, step, turtle);

        dragon.drawDragonCurve();
    }
}

Dragon.java

public record Dragon(final int order, final double step, final Turtle turtle) {
    public void drawDragonCurve() {
        drawDragonCurve(order - 1, 1);
    }

    private void drawDragonCurve(final int order, final int sign) {
        if (order == 0) {
            turtle.goForward(step);
            return;
        }

        drawDragonCurve(order - 1, 1);
        turtle.turnLeft(sign * -90);
        drawDragonCurve(order - 1, -1);
    }

Turtle.java

public interface Turtle {
    public void turnLeft(final double delta);
    public void goForward(final double step);
}

StdDrawTurtle.java

public class StdDrawTurtle implements Turtle {
    private double x;
    private double y;
    private double angle;

    public StdDrawTurtle(final double x, final double y, final double angle) {
        this.x = x;
        this.y = y;
        this.angle = angle;
    }

    public void turnLeft(final double delta) {
        angle += delta;
    }

    public void goForward(final double step) {
        final double oldX = x, oldY = y;

        x += step * Math.cos(Math.toRadians(angle));
        y += step * Math.sin(Math.toRadians(angle));

        StdDraw.line(oldX, oldY, x, y);
    }
}

FakeTurtle.java

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class FakeTurtle implements Turtle {
    record TurtleState(double x1, double y1, double x2, double y2) {}

    private final List<TurtleState> recording;
    private double x;
    private double y;
    private double angle;

    public FakeTurtle(final double x, final double y, final double angle) {
        recording = new ArrayList<>();
        this.x = x;
        this.y = y;
        this.angle = angle;
    }

    public List<TurtleState> getRecording() {
        return Collections.unmodifiableList(recording);
    }

    public void turnLeft(final double delta) {
        angle += delta;
    }

    public void goForward(final double step) {
        final double oldX = x, oldY = y;

        x += step * Math.cos(Math.toRadians(angle));
        y += step * Math.sin(Math.toRadians(angle));

        recording.add(new TurtleState(oldX, oldY, x, y));
    }
}
```
\$\endgroup\$
5
  • \$\begingroup\$ Nodrag appears to be a mis-reversal of Dragon. \$\endgroup\$ Sep 19, 2020 at 10:39
  • \$\begingroup\$ Thanks for your great answer! But one word to final: in general I agree to that. I just like to mention that this (IMHO) does not apply to classes and methods. If this is your opinion too you might mention that in the answer. \$\endgroup\$ Sep 19, 2020 at 14:25
  • \$\begingroup\$ @TimothyTruckle: In general, classes need to be explicitly designed for extension, so I would assume that when you are sure that your class is extensible, you will know to remove the final modifier. Also, I am big fan of immutable objects and those obviously require classes to be final. But you are right, I should clarify that in the answer. \$\endgroup\$ Sep 19, 2020 at 15:30
  • \$\begingroup\$ "In general, classes need to be explicitly designed for extension," One of your points is "testability". When talking about unit test we need to be able to mock dependencies of the unit under test. But mocking is hard for final declared classes. And no, PowerMock is not a solution, but a surrender to bad design... ;o) \$\endgroup\$ Sep 19, 2020 at 15:48
  • \$\begingroup\$ @JörgWMittag Thank you very much for the detailed and beautiful answer. Very much appreciated. :) \$\endgroup\$ Sep 19, 2020 at 21:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.