3
\$\begingroup\$

I wrote a working code and was wondering if it could be shortened as I saw a lot of similarities in my loop and if statements. I hope it's possible as I want to make it a bit cleaner.

I had to print a list of license plate numbers of all cars that went over the speed limit, as well as the average speed of each of those cars.

The first line contains the maximum speed (in km/h) for the road that the detectors are placed on. All other lines start with the number of the detector that was passed (1 or 2), followed by the time on which the car passed the detector, and then followed by the license plate number of that car.

example input:

50
1 09:00:00 OZ-15-ZU
2 09:00:04 WS-LP-74
1 09:00:07 97-YC-51
1 09:00:10 NV-71-PU
2 09:00:21 OZ-15-ZU
[...]
2 09:04:58 CN-70-OT

example output:

The following cars went over the speed limit:
GH-23-NN - 68 km/h
NV-71-PU - 56 km/h
CN-70-OT - 70 km/h
18-IP-VU - 59 km/h

input:

50
1 12:00:33 QW-23-56
1 12:01:39 GF-PY-26
2 12:00:43 QW-23-56
1 12:00:46 02-RV-WE
2 12:00:59 02-RV-WE
2 12:01:56 GF-PY-26
1 12:01:15 CF-QW-46
2 12:02:13 WM-20-PF
2 12:01:27 CF-QW-46
1 12:01:28 03-LC-TF
2 12:01:39 03-LC-TF
1 12:01:56 WM-20-PF

output:

The following cars went over the speed limit:
QW-23-56 - 72 km/h
02-RV-WE - 55 km/h
CF-QW-46 - 60 km/h
03-LC-TF - 65 km/h

my code:

H = 3600
M = 60
D = 200
MTS = 3.6

def seconds(time):
    line = time.split(':')
    h = int(line[0])
    m = int(line[1])
    s = int(line[2])
    return h * H + m * M + s

def when(file, max):
    for x in file[1::]:
        line = x.split()
        det = int(line[0])
        if det == 1:
            nump = line[2]
            start = seconds(line[1])
            for x in file[1::]:
                line = x.split()
                det = int(line[0])
                if det == 2:
                    numpe = line[2]
                    sece = seconds(line[1])
                    if nump == numpe:
                        time = D/(sece - start) * MTS
                        if time > max:
                            print('%s -%.0f km/h' % (numpe, time))

def main():
    file = open('traffic_input.txt').read().splitlines()
    max= int(file[0])
    when(file, max)
main()

Can the duplicate lines be replaced so that the code is shortened?

\$\endgroup\$
7
  • 1
    \$\begingroup\$ I am a high schooler, so these answers are honestly a bit too complicated to hand in. I was just hoping for a simplification of what I already had... Not a whole different code. Thank you tho \$\endgroup\$ Jun 30, 2022 at 15:57
  • 2
    \$\begingroup\$ @tryingitout At the same time, if you want to learn you'll have to understand how things could be better :) Pick an answer at a time and try to understand it would be my tip \$\endgroup\$
    – IEatBagels
    Jun 30, 2022 at 20:05
  • \$\begingroup\$ And drop comments to learn how things work if they're unclear. \$\endgroup\$
    – Reinderien
    Jun 30, 2022 at 22:25
  • 3
    \$\begingroup\$ You should undo your edit and leave the previous code and question as it was when you asked it; this means that other people can see what you did and how the answers relate to it. \$\endgroup\$ Jul 1, 2022 at 8:54
  • \$\begingroup\$ Wouldn't you also need to know the distance between the 2 detectors? \$\endgroup\$ Jul 1, 2022 at 20:47

7 Answers 7

7
\$\begingroup\$

Handling time is not trivial

You use a custom function and constants to handle the times recorded. In your case, it stays pretty simple, but in any real world scenario, you would likely handle a full date and time recording or a timestamp, in order to handle cases where the day changes between two records. Without that information, it will lead to bugs when a car crosses each detector on different day.

Again, in real world scenarios, you would probably encounter nasty edge cases like daylight saving times.

Also you can manage in your example case with a custom-rolled solution, it would be better to use the datetime library to handle time.

Don't iterate over the whole data twice

For every record produced by the first detector, you iterate over the whole data to find a match for the second detector. In other words, your algorithm is O(n²), and may get quite slow in a real-world use where there may be tens of thousands of vehicles per day on a given road.

Don't read the whole file at once

When reading the input file, you use read().splitlines() (side-note, this is equivalent to readlines()), meaning you read the whole file all at once and store all of its contents in memory.

Again, the input file can get quite large, and loading the whole file in memory is wasteful, you can instead read and process a line at a time with readline()

Don't test for happy path only

I can't tell for sure what data you tested your code on, but in the provided data, there is only well-behaved data, with each car being recorded exactly once by each detector. What happens if a detector fails and doesn't record a vehicle? What happens if the same vehicle passes multiple times over the detectors?

In the first case, your code drops the input, which is likely the desired option (but reporting a detector failure could be an alternative)

In the second case, it can lead to false negatives.

Separation of concern

Your when function both processes the data and prints the output. What if you want to log the speed limit offenses to a file or a database instead?

You should separate the logic from the presentation.

Document your code

You code has no comments, no docstrings, and non-descriptive names. As such, it is pretty hard to understand the logic of each section.

Some of the issues I pointed out will make the code longer and the logic more complex when addressed, so while I can still make sense of your code for the time being, it won't be the case anymore if you try to improve it's functionality.

Name shadowing

You use max as a variable name. However, max is a built-in function of python, that will no longer be accessible in your code.

This can easily be avoided with a better variable name, such as speed_limit.

Use a __main__ guard

Wrapping top-level code in a main function is good practice, the only thing missing is wrapping the call to main in a main guard:

if __name__ == '__main__':
    main()

This will allow to import your code as a module later on without processing the file specified in main.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Don't read the whole file at once is a bit mixed. I also prefer the "streamed" approach, but there are some circumstances under which a file that fits entirely in memory can be more quickly processed with separate, complete I/O and processing passes than streaming them together. \$\endgroup\$
    – Reinderien
    Jun 30, 2022 at 22:28
  • \$\begingroup\$ "Don't read the whole file at once" that would have to go with streaming the output and evicting cars seen twice from the current processing set, otherwise the memory issue remains. that comes with some trade-offs in terms of detecting invalid data (like a second pass on the second detector) (and the input file doesn't appear to be sorted, which also makes it complicated) \$\endgroup\$
    – njzk2
    Jul 2, 2022 at 19:11
6
\$\begingroup\$

I don't think I've seen this in other comments, but as someone who is learning to code, get into the habit of using descriptive variables everywhere.

As a direct example, you use a variable name D. I think you really mean distance_between_sensors, but here's the problem with D. It makes me guess your intent and is unclear.

There are numerous other examples within your code, like sece and numpe that have very little meaning to anyone but yourself. A big thing about code that is rarely taught in school is that code is not just instructions for the machine, but how you communicate to other software developers what your software is supposed to do.

At the end of the day, most people can stumble through writing code in most any language, but it takes a lot of practice and diligence to be able to write code in a way that others (yourself included!) months or years down the road can easily understand. Learning this early makes your life and everyone else's much easier.

\$\endgroup\$
3
\$\begingroup\$

Here's how I would write it:

from datetime import time

H = 3600
M = 60
D = 200
MTS = 3.6

def seconds(t):
    ts = time.fromisoformat(t)
    return ts.hour * H + ts.minute * M + ts.second

def when(lines, max_speed):
    dets = {1: {}, 2: {}}
    over_limit = []
    for line in lines:
        data = line.split()
        det = int(data[0])
        nump = data[2]
        time = seconds(data[1])
        dets[det][nump] = time
    for nump, time_start in dets[1].items():
        try:
            time_end = dets[2][nump]
        except KeyError:
            pass
        else:
            time = D/(time_end - time_start) * MTS
            if time > max_speed:
                over_limit.append(f'{nump} - {time:0.3} km/h')
    return over_limit

def main():
    with open('traffic_input.txt') as file:
        lines = file.read().splitlines()
    max_speed = int(lines[0])
    over_limit = when(lines[1:], max_speed)
    for l in over_limit:
        print(l)

if __name__=="__main__":
    main()

How, for a bit of explanation:

Simple changes in main and seconds: datetime module offers built in parser from ISO time format, so you can get the times without manual parsing.

In main, you opened a file but never closed it. It's not a good idea and will cause you problems later on. Best way to handle files is by using ContextManager (the with ... as ...) part, which makes sure the file gets closed after leaving the with block.

I've added if __name__=="__main__": guard to the call to main, so that it will not be executed on module import (note that you can still call main of the imported module manually should you want to do so).

Don't call your variables with built-in names like max - you are shadowing whatever it was originally, which means you can no longer access the old value and risk breaking other stuff and really hard to debug errors.

I've change the moment in code where the first line is removed, because it didn't make sense for me to have it located inside the when function. I also made when return the data, instead of printing it - functions that print instead of returning are extremely unfriendly for reusing them later - gather the data you want and print is when you're absolutely done with its handling.

Now, to the meat of you function:

This repetition in your code is actually also reflected during execution - you have 2 nested for loops that do in huge part same thing. If you change your logic slightly, you can gather the data you need in a single pass through the list, and then extract relevant entries in a 2nd pass through the gathered data.

Basicly, this piece of code:

for x in file[1::]:
        line = x.split()
        det = int(line[0])
        if det == 1:
            nump = line[2]
            start = seconds(line[1])
# Here the repetition starts
            for x in file[1::]:
                line = x.split()
                det = int(line[0])
                if det == 2:
                    numpe = line[2]
                    sece = seconds(line[1])

Is calculating exact same information multiple times when you could do it only once per each entry and find matching entries for each detector later.

I've changed it do a two dictionaries - one for each detector, that has keys for each license plate and holds the timestamp values. Now you can grab all the license plates encountered on detector 1, find its matching entry from detector 2 (because they are dictionaries, we can use the nump as a key and get the value directly without searching) and get the times straight away. This, imho results in a much cleaner code and one with better time complexity.

\$\endgroup\$
3
  • \$\begingroup\$ I know you're trying to be helpful, but don't just give a solution. This was clearly a homework problem, OP likely has just submitted your code as their own to their class and learned nothing. Have a look at meta.stackoverflow.com/a/334823/4308438 for a more in depth reasoning \$\endgroup\$ Jul 1, 2022 at 8:30
  • \$\begingroup\$ @lennartVH01 Are you sure it's homework? Based on the question, he might be working for the state highway police ;) \$\endgroup\$
    – KBurchfiel
    Jul 1, 2022 at 16:06
  • \$\begingroup\$ Don't use cryptic variable names like dets and nump. Use detectors and number_plate (or license_plate). Also, I suggest using the license plate number as top key in your dict so that you have detections[license_plate][detector_id] = timestamp. Then you can directly loop over each car. \$\endgroup\$
    – md2perpe
    Jul 2, 2022 at 15:26
3
\$\begingroup\$

Good advice in the other answers.

Thinking about your loops:

for x in file[1::]:

and the fact that this expression is re-evaluated on every single inner loop iteration:

time = D/(sece - start) * MTS

There is a way to get rid of all of the explicit loops, and have that expression evaluated in a vectorised manner. Pandas will do this, and will simplify all of your parsing and processing. Your seconds function will go away, your loops will go away, your H, M and MTS variables will all go away.

Basically, you need to:

  • Parse the file into a dataframe
  • Group by the two detectors
  • Inner-join (merge()) the left and right groups
  • Subtract the times and divide the distance by the times
  • Format and print the result

Consider sorting your output - Pandas makes this easy.

Suggested

import numpy as np
import pandas as pd

DISTANCE_KM = 0.2

with open('traffic_input.txt') as f:
    speed_limit = float(next(f))
    df = pd.read_csv(
        f, delim_whitespace=True, header=None,
        names=('detector', 'time', 'plate'),
    )
df['time'] = pd.to_timedelta(df.time)

(first_id, first), (second_id, second) = df.groupby('detector')
compared = first.merge(second, on='plate', suffixes=('_first', '_second'))
hours = (
    compared.time_second - compared.time_first
) / np.timedelta64(1, 'h')
compared['speed'] = (DISTANCE_KM / hours).abs()

illegal = compared.loc[
    compared.speed > speed_limit,
    ['plate', 'speed'],
].sort_values(by='speed', ascending=False)

print('The following cars went over the speed limit:')
print(illegal.to_string(
    index=False,
    formatters={'speed': '{:.0f} km/h'.format},
))

Output

The following cars went over the speed limit:
   plate   speed
QW-23-56 72 km/h
03-LC-TF 65 km/h
CF-QW-46 60 km/h
02-RV-WE 55 km/h
\$\endgroup\$
2
\$\begingroup\$

Under the assumption that, for a given plate, detection 1 is before detection 2 in the file, you only need to read and parse every line once.

If the line was from detector 1, we want to remember it and store it in a conveniently data container, a dictionary or dict for short.

If the line was from detector 2, we want to check if the plate is in our memory of detector 1 and then calculate speed and then do stuff...

For example:

TRAFFIC_FILE = 'traffic_input.txt'
DISTANCE     = 0.2 # [km]
SEC_IN_HOUR  = 3600
SEC_IN_MIN   = 60

def time_str_to_seconds(time):
    hours, minutes, seconds = time.split(':')
    seconds = SEC_IN_HOUR * int(hours) + SEC_IN_MIN * int(minutes) + int(seconds)
    return seconds

def calculate_speed(seconds):
    time_in_hours = float(seconds) / SEC_IN_HOUR   # [hour]
    speed = DISTANCE / time_in_hours               # [km/hour]
    return speed

def process_traffic_file(filename):

    with open(filename) as f:
    
        max_speed = int(f.readline())
        
        first_detections = {}  # dictionary to store first detection times
                               # key will be the number plate as a string
                               # value will be the time as a string
        
        for line in f:
            detector, time, plate = line.split()
            
            if detector == '1':
                # store first detection time
                first_detections[plate] = time

            elif detector == '2':
                if plate in first_detections.keys():
                
                    start_time = time_str_to_seconds(first_detections[plate])
                    end_time   = time_str_to_seconds(time)
                    delta_time = end_time - start_time
                    
                    speed = int(calculate_speed(delta_time))
                    if speed > max_speed:
                        print(f"{plate} - {speed} km/h")

                    # plate can be removed from first_detections
                    del first_detections[plate]
            else:
                pass # or do some error handling/message

if __name__ == '__main__':
    process_traffic_file(TRAFFIC_FILE)
\$\endgroup\$
2
  • \$\begingroup\$ You suggested an alternative solution, but the aim is to review the existing code \$\endgroup\$ Jul 1, 2022 at 10:49
  • 1
    \$\begingroup\$ @BillalBegueradj you are right, i changed my answer \$\endgroup\$
    – Jan Kuiken
    Jul 1, 2022 at 11:35
1
\$\begingroup\$

Your problem piqued my interest, so I wrote my own implementation with an aim for professionally maintainable code. This means losing out on a lot of performance for getting better maintainability. I'm doing many iterations through same data instead of catching stuff in the first pass, and the code ends up larger.

So, if you're interested not in the performance aspect, but the maintainability, see my reimplementation on Github, called Speeders. (Stole project structure from literate-wordle).

A few points are worth discussing as they diverge from your code:

  • Splitting concerns between parsing text/matching events/speed calc/speed limit check via separate files
  • Never stated the actual distance of detectors as a factor in problem statement, I had to glean the 200 (assumed meters) distance from your code
  • I assumed detector IDs could be different, raising ValueError on detector_id not in [1,2] for visibility of that potential bug.
  • Solved out-of-order detector hits (roads can be traveled both ways, so detector 2 hit before detector 1 isn't a bug, does your implementation support this?)
  • Explicit tests for each feature: it takes more time, but a bit of regression testing is helpful over time
  • Tooling around the code (linters, formatter, type-checking etc) help with consistency
  • Added a command line to process this, because it's always good fun.

As far as your code is concerned, you're solving the problem, and that's the biggest hurdle. You asked for performance oriented improvements, and indeed I'm not answering that.

But If you wanted to look at what drives professional code, starting from what you wrote, the next question I'd ask myself is "would I be ready to work with this code (change, maintain, extend it) for months at a time, together with a team", which lead to all sorts of code-readability challenges, clarifying code's assumptions (see above detector count + speed).

Hope my random evening writing python helps you, even if my answer is to be writen off firmly as "Off Topic".

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

Your approach works (at least for the valid cases), as demonstrated by your code.

However, it is a "naive" approach (nothing wrong with that, in many cases it can be sufficient, or at least a good start for something more efficient).

For each entry in the file that shows a car passing the first detector, you go through the whole file to find the passing of the second detector for this car.

A more efficient approach would be to store the information of which car passes each detector in a data structure that would allow you to retrieve them easily. For example:


    detection_infos = default_dict(dict)
    for x in file[1::]:
        line = x.split()
        detector = int(line[0])
        detected_time = seconds(line[1])
        plate = line[2]
        detection_info[plate][detector] = detected_time

    # then
    for plate, detectors in detection_infos.items():
        duration = detectors[2] - detectors[1]
        speed = MPS_TO_KMH * DETECTORS_DISTANCE / duration
        if speed > max_speed:
            print('%s -%.0f km/h' % (plate, speed))

This approach separates parsing the file from interpreting the results, by creating an intermediate data structure (albeit a very simple one, here)

It reduces the complexity of individual parts, and allows the coder to segregate responsibility and test those individually (Does the file parsing work? Given a valid data structure, does the speed detection work?).

This remains a simple approach, and if, for example, the data set was becoming very big, it would not scale, and would need to be re-visited.

Input validation is left as an exercise to the reader (consider what happens if the car didn't go through one of the detectors, or passed twice, or passes in reverse order?)

\$\endgroup\$

Your Answer

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

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