31
\$\begingroup\$

In physics class we are doing an investigation thing. Some of the numbers might seem plucked out of thin air but they aren't. The maths is right(I think) I just want some feedback on the code.

import datetime
import random
import time
import math


player_1 = input("Player one: ")
speed = int(input("Speed in km/h: "))
speed_ms = ((speed)/3.6)
conditions = input("Wet or dry conditions? (all lowercase)")

if conditions == "wet":
    friction_coefficient = 0.4
else:
    friction_coefficient = 0.6


time.sleep(0.5)
print("Get ready..")
time.sleep(random.randint(1,12))
then = datetime.datetime.now()
t = input("GO!! ")
now = datetime.datetime.now()
diff = then-now
reaction_time = round(abs(diff.total_seconds()), 2)


Rt = (math.sqrt((reaction_time*98)/490))
print('Your final reaction time was: ', (Rt), '!\n')

Rd = (Rt * speed_ms)
print('Your final reaction distance was: ', (Rd), '!\n')

Bd = (((speed_ms)**2) / 19.6 * (friction_coefficient))
print('Your final breaking distance was: ', (Bd), '!\n')

Sd = ((Rd)+(Bd))

############################################################################

print("\n\n---===========FINAL STOPPING DISTANCE===========---")
print('--------------', (Sd), 'meters', '------------')
\$\endgroup\$
  • 2
    \$\begingroup\$ Spelling - braking (from verb "to brake"), not "breaking" (from "to break"). Different things, though easily confused as they sound the same. \$\endgroup\$ – Toby Speight Mar 15 '18 at 8:57
  • 1
    \$\begingroup\$ I'm not a mathematician nor a physician but I doubt that I could stop a car going 130km/h in 40 meters, even with an immediate reaction \$\endgroup\$ – Thomas Ayoub Mar 15 '18 at 9:45
  • 12
    \$\begingroup\$ @ThomasAyoub: I hope physicians only need to get involved when the braking distance is too large anyway. \$\endgroup\$ – Joey Mar 15 '18 at 10:53
  • 1
    \$\begingroup\$ This doesn’t warrant an answer but you should really follow PEP8, the Python style guide. In particular, your identifiers should follow Python style. \$\endgroup\$ – Konrad Rudolph Mar 15 '18 at 12:17
  • \$\begingroup\$ I think it came from an incorrect interpretation/manipulation of the "drop the ruler" reaction time t = sqrt(2*d / g), with distance in cm, which simplified to t = sqrt(d/490). \$\endgroup\$ – Benjamin Mar 17 '18 at 3:15
37
\$\begingroup\$
  • Some of the numbers might seem plucked out of thin air but they aren't

    Still better have them explained. I can understand 3.6 and even 19.6 (it is \$2g\$, isn't it?), but I have no idea what 98/490 stands for. Declare them as symbolic constants with meaningful names.

  • I don't see a need for abs in

        reaction_time = round(abs(diff.total_seconds()), 2)
    

    Here diff may not be negative. If it is, I'd rather throw an exception, because the result is meaningless anyways.

  • You are rounding too early. There is no penalty computing Rt with reaction time not being rounded, but rounding leads to loss of precision. Only round when printing results.

  • You have too many parenthesis. Sd = Bd + Rd is much cleaner than Sd = ((Rd)+(Bd)).

\$\endgroup\$
  • 4
    \$\begingroup\$ Surely now will always be greater than then, so diff will always be negative? Though would probably make more sense to remove the abs, and set diff = now - then \$\endgroup\$ – Mick O'Hea Mar 15 '18 at 11:25
  • 3
    \$\begingroup\$ now may not always be greater than then. For example the last second of 2016, 2016-12-31T23:59:60 might be less than the penultimate second 2016-12-31T23:59:59.99 because I'm not sure if en.wikipedia.org/wiki/Leap_second is implemented properly for UNIX times. You should assert now > then \$\endgroup\$ – csiz Mar 15 '18 at 17:56
  • 4
    \$\begingroup\$ Using a timer function like time.time() or timeit.default_timer(), instead of datetime, should prevent the issue, I would imagine. It is probably also a more appropriate abstraction for what is being measured, i.e. a length of elapsed time from a stopwatch, rather than a distance between two points on a calendar. See: stackoverflow.com/questions/7370801/… \$\endgroup\$ – wwarriner Mar 15 '18 at 22:48
  • \$\begingroup\$ Crossover commenting from Mattias's post: Why has no one pointed out that 98 / 490 is / 5? \$\endgroup\$ – hjpotter92 Mar 16 '18 at 4:20
  • 1
    \$\begingroup\$ @hjpotter92 There must be a reason why 1/5 is honored with such encoding. Otherwise division by 5 would still make no sense to me. \$\endgroup\$ – vnp Mar 16 '18 at 4:51
18
\$\begingroup\$

I suggest breaking the code into functions, so you can easily import your code into a Python shell and test that your computation is exact (and not just thinking it is).

To expand on other answers, you may:

  • retry asking the speed if it is not an integer (detected when the int function raises a ValueError);
  • in the same vein, since you don't really need to check for both wet and dry, you could ask a simple yes/no question;
  • flush the input stream before printing GO (using termios on linux or msvcrt on windows);
  • prefer time.perf_counter() to datetime.now() to compute the time elapsed between two points in the program;
  • compute all the results before printing them.

Proposed improvements:

import random
import time


def flush_input():
    try:
        import msvcrt
        while msvcrt.kbhit():
            msvcrt.getch()
    except ImportError:
        import sys, termios    #for linux/unix
        termios.tcflush(sys.stdin, termios.TCIOFLUSH)


def ask_speed():
    while True:
        speed = input("Speed in km/h: ")
        try:
            return int(speed)
        except ValueError:
            print("An integer is required!")


def ask_is_wet():
    is_wet = input("Is the road wet? [y/n] ")
    return is_wet.lower() in ('y', 'yes')


def compute_distances(speed_kmh, reaction_time, is_wet):
    speed_ms = speed_kmh / 3.6
    friction_coefficient = 0.4 if is_wet else 0.6

    reaction_time = (reaction_time * 98 / 490)**.5
    reaction_distance = reaction_time * speed_ms
    braking_distance = speed_ms**2 / (19.6 * friction_coefficient)
    total_distance = reaction_distance + braking_distance

    return reaction_time, reaction_distance, braking_distance, total_distance


def get_reaction_time(prompt="GO!! "):
    time.sleep(random.randint(1, 12))
    then = time.perf_counter()
    flush_input()
    input(prompt)
    now = time.perf_counter()
    return now - then


def main():
    name = input("Player one: ")
    speed = ask_speed()
    is_wet = ask_is_wet()
    time.sleep(0.5)
    print("Get ready..")
    reaction_time = get_reaction_time()

    reaction, distance, braking, total = compute_distances(speed, reaction_time, is_wet)

    print('Your final reaction time was: ', reaction, '!\n')
    print('Your final reaction distance was: ', distance, '!\n')
    print('Your final braking distance was: ', braking, '!\n')
    print("\n\n---===========FINAL STOPPING DISTANCE===========---")
    print('--------------', total, 'meters', '------------')


if __name__ == '__main__':
    main()
\$\endgroup\$
  • 6
    \$\begingroup\$ Why has no one pointed out that 98 / 490 is / 5? \$\endgroup\$ – hjpotter92 Mar 15 '18 at 12:34
  • \$\begingroup\$ @hjpotter92 This should be a better addition to vnp's answer \$\endgroup\$ – 409_Conflict Mar 15 '18 at 16:00
13
\$\begingroup\$
  • It crashes if user input for speed_ms is not integer
  • If user presses any key before t = input("GO!! ") his reaction time is saved as 0. This is a major flaw in a code, and should be fixed.
\$\endgroup\$
12
\$\begingroup\$

If it's wet, the friction_coefficient will be lower. Which means that Bd will be lower (which is probably wrong). It's difficult to see that, because of all the unnecessary parentheses. You missed the necessary ones. You probably want

Bd = speed_ms**2 / (19.6 * friction_coefficient)

Rt, Rd, and Bd should get better names, or need a comment. In particular Rt, which probably means reaction time -- there already is a name reaction_time with a perfectly sensible value. That Rt contains a sqrt is suspicious. It needs a comment, if it is correct.

\$\endgroup\$
  • 1
    \$\begingroup\$ That square root looks completely wrong to me. reaction_time is a perfectly sensible time in seconds, Rt is then used as a time in seconds - but it is a value with units "square-root-seconds". \$\endgroup\$ – Martin Bonner supports Monica Mar 15 '18 at 14:20
12
\$\begingroup\$
  • player_1 is read and assigned but never used.

  • If the user misspells wet (e.g. as Wet or as wet), the program assumes that dry was meant - it should ask again until it gets an answer it understands or the player gives up (and consider adding icy for more fun).

  • There's a lot of unnecessary parentheses - they could be trimmed to make the code more readable.

\$\endgroup\$
2
\$\begingroup\$

Don't rely on users following your instruction to enter in lower case. Do conditions = input("Wet or dry conditions? (all lowercase)").lower(). Instead of doing if else, check each possibility separately, and ask again if it's neither (you might want to do 'wet' in conditions rather than `'wet' == conditions in case they put an extra space at the end or something).

Since you're going to divide by a non-integer, you're going to end up with float anyway, so it's probably better to cast speed as a float to begin with, and avoid rounding if they don't enter an integer. You should also do a try block to catch people entering non-numbers.

\$\endgroup\$
  • \$\begingroup\$ If you're going to convert to lower-case anyway (which is a good thing to do), why prompt the user to type in lower-case? \$\endgroup\$ – David Richerby Mar 16 '18 at 17:14
  • \$\begingroup\$ @DavidRicherby 1. I didn't think it was worth changing. 2. Redundancy is good. 3. If a user is unsure what format to use, telling them to use lower case is simpler than saying "Hey, we case everything as lower case, so don't worry about what case". \$\endgroup\$ – Acccumulation Mar 16 '18 at 17:50
2
\$\begingroup\$

Output formatting: Your print calls output spaces between the comma-separated arguments; this causes output like:

Your final reaction time was:  1.6 !

Those extra spaces are ugly. An easy fix is the sep argument for print:

print('Your final reaction time was: ', (Rt), '!\n', sep='')
Your final reaction time was: 1.6!

This will print no spaces between the three parts. Alternatively, you could do some string concatenating, but I prefer the first approach.

I would also drop the colon and include a unit for this measurement:

Your final reaction time was 1.6 seconds!

Slowest gun in the west...

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