23
\$\begingroup\$

I've been working on learning Java and this was a challenge as part of a chapter on data types:

Create a program that computes how far away, in feet, a listener is from a sound. Sound travels approximately 1,100 feet per second through air.

You can also compute the distance to a large object, such as a rock wall, by timing the echo. For example, if you clap your hands and time how long it takes for you to hear the echo, then you know the total round-trip time.

Java: A Beginner's Guide, Sixth Edition - by Herbert Schildt · McGraw-Hill Education (Chapter 2)

I'm super-new to OOP and am trying to nip bad habits in the bud, so-to-speak. I've divided the logic in one class as much as I think is possible (at my current skill level).

SoundSpeed.java

public class SoundSpeed {
    private final double SOUND_TRAVEL_SPEED = 1100;
    public double timeInSeconds;
    public boolean isEcho;
    private int distanceDivisor;

    double getResult() {
        if(isEcho) {
            distanceDivisor = 2;
        } else {
            distanceDivisor = 1;
        }
        return (timeInSeconds * SOUND_TRAVEL_SPEED) / distanceDivisor;
    }
}

SoundSpeedCalc.java

class SoundSpeedCalc {
    public static void main(String args[]) {
        SoundSpeed soundSpeed = new SoundSpeed();
        soundSpeed.timeInSeconds = 35.079;
        soundSpeed.isEcho = true;
        String verb;

        if(soundSpeed.isEcho) {
            verb = "echo";
        } else {
            verb = "travel";
        }
        System.out.println("The sound took " + soundSpeed.timeInSeconds + " seconds to " 
        + verb + " and thus the distance is " + soundSpeed.getResult() + " feet away.");
    }
}

Example output (using above parameters in main):

The sound took 35.079 seconds to echo and thus the distance is 19293.45 feet away.

Example output if I set soundSpeed.isEcho to false:

The sound took 35.079 seconds to travel and thus the distance is 38586.9 feet away.

\$\endgroup\$
30
\$\begingroup\$

What you want to do is simply

public static double distanceBySounding(double seconds, boolean isEcho) {
    return (isEcho ? 0.5 : 1.0) * seconds * 1100 /* ft. per second */;
}

The question is, how should this function be expressed in an object-oriented way? It's pretty safe to say that this…

SoundSpeed soundSpeed = new SoundSpeed();
soundSpeed.timeInSeconds = 35.079;
soundSpeed.isEcho = true;
System.out.println(soundSpeed.getResult());

… is an unpalatable interface. Manipulating public instance variables like that is not only cumbersome, it is an unsound practice (pardon the pun). Given a choice between the two options above, I would much prefer the simpler function to the contorted object. And that's fine. Java allows you to write code that doesn't fully conform to OOP.


What if you wanted an object-oriented design? The key to object-oriented design is that you want to model objects. That means that you have to find plausible objects to model — the more realistic, the better. What's a SoundSpeedCalc? I don't know; you can't buy one. But you can sure buy a sonar! It makes sense to instantiate a Sonar with its configuration parameters (the sound medium and its active/passive mode), then you can start "aiming" it at stuff by calling .distance().

public class Sonar {
    public enum Medium {
        AIR(1100);

        private final double ftPerSec;

        Medium(double ftPerSec) {
            this.ftPerSec = ftPerSec;
        }
    }

    private final Medium medium;
    private final boolean isEcho;

    public Sonar(Medium medium, boolean isEcho) {
        this.medium = medium;
        this.isEcho = isEcho;
    }

    public double distance(double seconds) {
        return medium.ftPerSec * seconds / (isEcho ? 2 : 1);
    }
}

Here's how you can use it effectively:

public static void main(String[] args) {
    double seconds = 35.079;
    boolean isEcho = true;

    Sonar sonar = new Sonar(Sonar.Medium.AIR, isEcho);
    String verb = isEcho ? "echo" : "travel";
    System.out.printf("The sound took %s seconds to %s and thus the distance is %s feet.\n",
                      seconds, verb, sonar.distance(seconds));
}

Note the use of System.out.printf() as a nicer alternative to string concatenation. Also, I've used conditional expressions — it's a perfect way to assign one of two values depending on a boolean switch.

\$\endgroup\$
  • \$\begingroup\$ OP's "object oriented" approach reminds me of my AP Computer Science class. Not correct or practical, but enough to teach the basics of objects \$\endgroup\$ – Cole Johnson Feb 23 '15 at 14:18
  • 4
    \$\begingroup\$ @ColeJohnson: indeed, the problem with these examples often is that by removing any substantial content that might obscure the view of the OOP techniques involved, you end up with a problem so trivial that it's very difficult to justify throwing any kind of technique at it all, and the resulting code is ridiculous ;-) More realistically, the value of these abstractions can be seen in the test harness, since that shows all major uses of it. If your ambition is just a function that multiplies its input by 1100 then you've already lost, we need interesting requirements to write interesting code! \$\endgroup\$ – Steve Jessop Feb 23 '15 at 16:30
  • \$\begingroup\$ +1 Love the point to model real objects. All too often we lose track of this point. I would prefer an interface and final implementations for MEDIUM over an enum for extensibility purposes. \$\endgroup\$ – psaxton Feb 25 '15 at 4:00
16
\$\begingroup\$

Does a result by any other name smell as sweet?

  • The name timeInSeconds might be expressed just as well as seconds.

  • The usual name of SOUND_TRAVEL_SPEED is SPEED_OF_SOUND

  • SOUND_TRAVEL_SPEED might benefit from a comment stating the units and any other assumptions:

    /* approximate, feet/second at sea level */
    
  • getResult could use a more descriptive name. Perhaps feet would do.

To instance or not to instance, that is the question

The SoundSpeed might not need to be instantiated. It might be fine as just a place to hold the SOUND_TRAVEL_SPEED constant, and a static function to do the calculation:

public class SoundSpeed {
    private final double SOUND_TRAVEL_SPEED = 1100;
    public static double feet(double seconds, boolean isEcho) {
        double feet = seconds * SOUND_TRAVEL_SPEED;
        if(isEcho)
            feet = feet / 2;
        return feet;
    }
}
\$\endgroup\$
  • \$\begingroup\$ I disagree with your first line. Never call a variable or parameter by its units. time would be acceptable, as would elapsedTime, travelTime or other variation on that concept. Exception: If you're doing unit conversions, then units would be acceptable parameter names. \$\endgroup\$ – Chris Cudmore Feb 23 '15 at 17:42
  • \$\begingroup\$ @ChrisCudmore I thought we were doing unit conversions. \$\endgroup\$ – Wayne Conrad Feb 23 '15 at 18:56
  • \$\begingroup\$ @ChrisCudmore, why not? I have multiple references that including the unit of measurement in the variable name is a good habit. \$\endgroup\$ – Vorac Feb 24 '15 at 12:41
  • \$\begingroup\$ Let's look at the function call: feet(double seconds, boolean isEcho) What does it return? feet? No. it returns a distance. What parameter does it take? It takes a time. You can make them more explicit, such as distanceTravelled and sonarPingTime. Units should be part of the API documentation, or more robustly, as a parameter such as Units.SECONDS \$\endgroup\$ – Chris Cudmore Feb 24 '15 at 13:30
  • 1
    \$\begingroup\$ @ChrisCudmore If the meaning of the variables are clear in their context, and changing them would require adding comments to communicate the same intent, then that rule is hurting the code, not helping it. However, I do agree that making units explicit--by using a unit library, or having a unit parameter, is often better. \$\endgroup\$ – Wayne Conrad Feb 24 '15 at 16:26
13
\$\begingroup\$
  • SOUND_TRAVEL_SPEED should also be static:

    private static final double SOUND_TRAVEL_SPEED = 1100;
    

    This is because it's not owned by a particular class instance.

  • The other data members should still be private:

    private double timeInSeconds;
    private boolean isEcho;
    

    This applies to all classes in general.

  • The calculations in getResult() can be simplified with a ternary statement:

    distanceDivisor = (isEcho) ? 2 : 1;
    

    The same thing can be done with verb's assignment in main().

\$\endgroup\$
  • \$\begingroup\$ I tried to set those two as private but I got compiler errors when I ran SoundSpeedCalc... The field SoundSpeed.timeInSeconds is not visible, etc. Is there something else I need to change? \$\endgroup\$ – Phrancis Feb 23 '15 at 1:30
  • 2
    \$\begingroup\$ @Phrancis: That's because you're setting its value after declaring an instance, so it wouldn't be allowed to modify that private field. You should construct soundSpeed with that value instead. \$\endgroup\$ – Jamal Feb 23 '15 at 1:32
  • 1
    \$\begingroup\$ "SOUND_TRAVEL_SPEED should also be static:" this would preclude the usage of this class underwater for SONAR. \$\endgroup\$ – Aron Feb 23 '15 at 6:52
8
\$\begingroup\$

Just one thing:

Do not have a boolean field that begins with is. Like this one:

public boolean isEcho;

(200_success and others is right that this should not be public)

In Java, the naming conventions for boolean getters are that they should begin with is, followed by the name of the variable they are a getter for. So let's say this variable were to have a getter:

public boolean isIsEcho() {
    return this.isEcho;
}

Now that just sounds strange!

The correct naming convention for the variable and its getter method is:

private boolean echo;

public boolean isEcho() {
    return this.echo;
}
\$\endgroup\$

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.