3
\$\begingroup\$

In this code the angle between the minute and hour hands of an analog clock are found and calculated and the smaller angle between them returned. This code works perfectly and I am getting back the results I expect.

We were told to do this and any testing if necessary.

I got asked this in an interview question and wanted to see if there was any way to improve what I came up with.

Is there any way you could think to do all this better and shorten this. I am not exactly sure if the validation is the best either. I put in if statements to stop anyone from putting in a random number or more digits then needed but I am not sure if there is a better or more efficient way to do that.

I thought about maybe trying to split it individually into every digit so I could validate the time that way but i was not sure it was worth doing it that way either.

If there is something I should be doing better to make my code look better or anything that would earn more points within an interview would be great.

Anything is appreciated as coding in interviews is difficult and I was wondering if there was anything I could of done better.

Example input in main method

            //valid input
            System.out.println(calcAngle("17:00")); //should return 150
            System.out.println(calcAngle("15:15")); //should return 7.5
            System.out.println(calcAngle("3:15")); //should return 7.5
            System.out.println(calcAngle("9:30")); //should return 105
            System.out.println(calcAngle("17:59")); //should return 174.5
            System.out.println(calcAngle("11:15")); //should return 112.5
            System.out.println(calcAngle("23:15") + "\n"); //should return 112.5
            
            //invalid input should return error message
            System.out.println(calcAngle("24:60"));
            System.out.println(calcAngle("00:-1"));
            System.out.println(calcAngle(":00"));
            System.out.println(calcAngle(":"));
            System.out.println(calcAngle("25:00"));
            System.out.println(calcAngle("11:80"));
public static String calcAngle(String time)
    {

        double hour=0;
        double min=0;
        
        //Seperating the string hour and minute to different array value
        String[] sep = time.split(":");
        
        /*
         * Two if statements to catch any errors
         * Stops any incorrect formatting of time causing an error or exception
         */
        
        //stops any value if time has more than 5 or less than 3
        if(time.length()>5 || time.length()<3) {
            return "Time " + time +" is not formated correctly format as hh:mm with 14:00 or 2:00";
        }
        
        //getting values of array to check
        String checkHour = sep[0];
        String checkMinute= sep[1];
        
        //does not allow values to be read if they are empty or greater than 2
        if(checkHour=="" || checkMinute=="" || checkMinute.length()>2 || checkHour.length()>2) {
            return "Time " + time + " is not formated correctly format as hh:mm with 14:00 or 2:00";
        }
        
        //turning strings to doubles
        hour = Double.parseDouble(sep[0]);
        min = Double.parseDouble(sep[1]);
        
        
     //value check
        if(hour<0 || hour>24 || min<0 || min>59) {
            return "Time " + time + " out of bounds for a 24 hour clock";
        } 
        
        
        // handle 24-hour notation
        hour = hour % 12;
 
        // find the position of the hour's hand
        double hours = (hour * 360) / 12 + (min * 360) / (12 * 60);
 
        // find the position of the minute's hand
        double minute = (min * 360) / (60);
 
        // calculate the angle difference
        double angle = Math.abs(hours - minute);
 
        // consider the shorter angle and return it
        if (angle > 180) {
            angle = 360 - angle;
        }
 
        //returns in The answer for 17:00 is 150.0 Degrees
        return "The answer for " + time + " is " + angle + " Degrees ";
    }
\$\endgroup\$
1
  • 1
    \$\begingroup\$ "0930" will raise an ArrayIndexOutOfBoundsException. Also, "12:00:59" incorrectly returns 0.0 degrees. \$\endgroup\$
    – AJNeufeld
    Nov 24 at 23:16
2
\$\begingroup\$

Declare variables close to where they're used. hour and min are declared and then ignored for half the body of the method. They can be assigned at declaration time, rather than being set to 0 first. And consider making them final, as they should not be reassigned. sep can also be assigned later.

It is unclear to me why hour and min are doubles and not integers.

Avoid abbreviations in names where possible. min should probably be minute. sep doesn't mean anything to me. Separation? Maybe timeParts would be better?

Most of the comments don't provide information to the reader. Comments should be used to explain why something is done. Readers can tell what is being done by reading the code. If they can't, the code needs to be fixed. "What" comments tend to become stale and incorrect as code is modified.

In idiomatic java, whitespace surrounds < and >. There is also whitespace between a control flow operator, such as if, and the opening parenthesis.

Checking the length of the string is duplicated by later checks. Likewise, checking the length of the numbers duplicates work later which ensures they have good numeric values.

It's not clear to me if the declaration of the calcAngle method can be changed. If it can, returning a String is probably not correct. Throw an exception on failure, and return a numeric angle on success.

If the hour or minute is not a number, the code throws a NumberFormatException which doesn't clearly indicate which value is the problem.

The validation code considers 2459 to be a valid hour/minute pairing. If supporting a 24-hour clock, it should roll from 2359 to 0000.

hours and minute are very confusing. hourAngle and minuteAngle would be strong improvements. angleDifference would be better than angle. Since the unit of measure is degrees, it would be nice if that was included in the variable names as well. Let's avoid crashing into Mars.

If minute is computed before hours, its computed value can be used when computing hours.

Using JUnit tests would be highly preferable to a battery of System.out.println calls and visual inspection of the results.

\$\endgroup\$
1
\$\begingroup\$
  1. Comments - there is too many of them, usually in places where you could extract code to the well-named method (or just remove comments), try to make the comments unnecessary by improving the code structure - leaving only the ones that explain something not obvious (not needed here) e.g. consider value of:
//turning strings to doubles
hour = Double.parseDouble(sep[0]);
  1. Consider actual unit tests instead of manual verification of code outputs. So instead of System.out.println(calcAngle("17:00")); //should return 150 you can go with assertEquals(calcAngle("17:00"), "150"); where assertEquals may come from your favorite testing library or you might write it on your own (given project's small scope) - so it throws exception on equality failure

  2. Instead of returning strings describing how the program failed you probably want to simply throw exceptions (given small scope just RuntimeException or IllegalArgumentException with proper message should be fine)

  3. Consider duplication of your validations (1st vs 2nd) - maybe you don't need to validate whole string if you are validating its parts (or the other way around)? Also current code will fail on missing ":" sign (in, importantly, not predicted, inconsistent way - failure in itself on incorrectly formatted input is expected behavior) - exceptions may help addressing this. Also I'd consider allowing the code to just fail during parsing - if errors are to be read by programmer default exceptions are usually descriptive enough

  4. Duplication of checking checkMinute and checkHour length vs cheking values range (0-60 and 0-24) - if values are in range they automatically have proper length

  5. checkHour and checkMinute names are confusing - check is a verb and variables usually represent data - nouns with adjectives are usually more helpful describing it

  6. It might be nitpicking but you might consider Double floating point precision - parsing input to Integer preserves more accurate representation of integer values for longer and you can still explicitly decide how you want to handle integer division (have you considered BigIntegers?) - which seems to be the reason for doubles usage

  7. double hours and double minutes variable names - these are already angles (between position at 12:00 and current), it would be nice to include this fact in names BTW: consider simplifying calculations to something like hour * ONE_HOUR_ANGLE + minutes / 2; where ONE_HOUR_ANGLE = 360/12

\$\endgroup\$
1
\$\begingroup\$

Eric Stein and Azahe have covered many points already. I won't repeat those here, although some of their points may be reflected in my code.

Decomposition

This code is made up of one large function. The function does a lot of things, any of which we might want to pull out and used separately. For example, both the hour and minute hand angles are computed and discarded. If we wanted to actually draw a clock with an hour and minute hand, we'd have to add more code with exactly the same internal calculations.

Instead, we can break the function up into several smaller functions which determine the angles of the hour and minute (and second) hands, and use those functions in another function to calculate the angle between the hour and minute hands.

public class AnalogClock {
    private final static int FULL_CIRCLE_DEGS = 360;
    private final static int HALF_CIRCLE_DEGS = FULL_CIRCLE_DEGS / 2;

    public static double hourHandAngle(LocalTime time) { ... }
    public static double minuteHandAngle(LocalTime time) { ... }
    public static double secondHandAngle(LocalTime time) { ... }

    public static double angleBetweenHands(LocalTime time) {
        double hour_hand_degs = hourHandAngle(time);
        double minute_hand_degs = minuteHandAngle(time);

        double angle_diff_degs = Math.abs(hour_hand_degs - minute_hand_degs);
        if (angle_diff_degs > HALF_CIRCLE_DEGS)
            angle_diff_degs = FULL_CIRCLE_DEGS - angle_diff_degs;

        return angle_diff_degs;
    }
}

Use built-in classes where possible

Java has many built-in class for working with times. The class LocalTime is "a description of the local time as seen on a wall clock", making it ideal for representing the time for your problem.

The .toSecondOfDay() method (or the .toNanoOfDay() method if you want sub-second accuracy) will return a value you can use to compute the hand angles from.

It even has a .parse(CharSequence text) method for converting a time string into a LocalTime object, which pretty much solves most of your parsing issues.

    public static double angleBetweenHands(String time_string) {
        return angleBetweenHands(LocalTime.parse(time_string));
    }

You can use a DateTimeFormatter object for additional flexibility, if needed.

\$\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.