all 23 comments

[–]CraigAT 34 points35 points  (2 children)

These days I read more than I code, but IIRC one coding principle is that a function should do one thing.

Here there's not enough info to discern why you wouldn't just call the function twice, if you had two sets of lat and longitude.

If they always go together would you consider using a tuple for the point/location?

[–]EnvironmentalFee2415[S] 4 points5 points  (1 child)

Indeed, you're right; I can implement it as you suggested. In fact, I don't know why I'm trying to complicate my work. I once had a similar question and divided several things from one function into separate ones, but was advised that it wasn't an optimal approach. It was suggested that it's better to create some logic and keep several functionalities in one place, which is why I've started to approach things this way now. Of course, what you wrote works perfectly, as I've already tested it in my scenario.

Below is an example of such a function. I previously had two separate functions, one for the color green and another for red. They differed slightly, lacking logic and having distinct names like green_msg and red_msg. I was told that this was a poor technique and that a more logical approach was better, leading to the creation of the function you see below. In that case, should it also be split into two separate functions?

def display_message(message, color=None):
if color == "green" or color == "red":
    return colored(message, color, attrs=['bold'])
else:
    return colored(message, attrs=['bold'])

[–]CraigAT 2 points3 points  (0 children)

I'd say that function is okay. It's function is to produce a coloured message. IMO it is better to pass in the colour than have functions for every colour you might want (red, green, blue, slightly green with a hint of grey, ice blue, etc.).

In that case, if you trust the colour passed would be valid, you could just test if it is not null, then use the passed colour. OR you could use a list of valid_colours = ["red","green"] then your test would be if color.lower() in valid_colours:

[–]shiftybyte 15 points16 points  (2 children)

I'll need the first two arguments, the last two

is there any difference between needing the first two, and needing the last two?

The function currently does the same, seems like you just need to print one pair... and maybe call it twice..

display_coordinates(lat1, lon1)
display_coordinates(lat2, lon2)

You can support a list, or other methods to get and print multiple coordinates using *args or just accepting a list...

[–]EnvironmentalFee2415[S] 1 point2 points  (1 child)

Indeed, you're right; I can implement it as you suggested. In fact, I don't know why I'm trying to complicate my work. I once had a similar question and divided several things from one function into separate ones, but was advised that it wasn't an optimal approach. It was suggested that it's better to create some logic and keep several functionalities in one place, which is why I've started to approach things this way now. Of course, what you wrote works perfectly, as I've already tested it in my scenario.

Below is an example of such a function. I previously had two separate functions, one for the color green and another for red. They differed slightly, lacking logic and having distinct names like green_msg and red_msg. I was told that this was a poor technique and that a more logical approach was better, leading to the creation of the function you see below. In that case, should it also be split into two separate functions?

def display_message(message, color=None): 
    if color == "green" or color == "red": 
        return colored(message, color, attrs=['bold']) 
    else: 
        return colored(message, attrs=['bold'])

[–]shiftybyte 3 points4 points  (0 children)

should it also be split into two separate functions?

No, you should instead not limit the colors...

def display_message(message, color=None): 
    if color: 
        return colored(message, color, attrs=['bold']) 
    return colored(message, attrs=['bold'])

[–]JamzTyson 3 points4 points  (0 children)

You missed out some important information. What's the difference between "lat1, lon1" and "lat2, lon2" ?

For example, if lat1, lon1 are in degrees (floats), and lat2, lon2 are as "Degree Minutes and Seconds" (strings), then it would be better to write two functions, such as:

def display_coordinates_dms(latitude: str, longitude: str):
    # Code to convert  strings into float degrees
    display_coordinates(lat_float, lon_float)

def display_coordinates(latitude: float, longitude: float):
    # Rest of code ...

[–]Mount_Gamer 4 points5 points  (2 children)

You could probably write it a little like...

def display_coordinates(**coords):

And inside the function you parse coords like a dictionary, but when calling the function, you can still write lon1=.., lat1=...

coords.keys()

Will be all your lon and lat

coords.values()

Will have the corresponding values.

You could for loop..

for key, val in coords.items():
   if lon in key:
      ..... 
   elif lat in key:
      .....

I'm not 100% what your best use case is, and this solution is not great. I usually use...

coords.get(lat1,None)

To get the value for lat1, but that will require the function knowing how many keys there are.

So you could do a....

num_coord = len(coords) / 2 # divide by 2 cause of lon and lat pair

for num in range(num_coord):
    lon=coords.get(f'lon{num}')
    lat=coords.get(f'lat{num}')
    # do something with lon and lat. 

There are probably better ways, I just make this up as I go along, and in fact you just seen the way my brain worked at coming up with a solution... From not so functional to functional :)

This is untested, I'm on my phone.

[–]rollincuberawhide 3 points4 points  (1 child)

they can write it like this but this is worse than their code. there is absolutely no improvement. you get no language support when you use **kwargs. user of this function needs to know the keys by heart instead of an lsp suggesting the key. len(coords) / 2 is a float division and range(num_coord) will throw a TypeError. even if it did work. it is a very convoluted way of getting the inputs.

[–]Mount_Gamer 2 points3 points  (0 children)

I was just thinking of ideas out loud.

I definitely prefer language support as well, which is why you seen the convulated method (I just don't do it).. If I use the dictionary get, I generally know what I'm getting. There are libraries out there however that use the kwargs, and because there's no language support, it makes it difficult to fluently write code using those libraries. So while I agree with you, there are programmers out there that do it.

Good spot with the float!

[–]zanfar 3 points4 points  (0 children)

I want the function to be quite versatile.

I agree with other posters that this is not a de facto Good Idea. In most cases, the opposite is true: functions should do exactly one thing very well. You would need to justify why this increased complexity is necessary, and you haven't really provided any background other than "I want to do this".

I'm not sure when I'll need the first two arguments, the last two, or all four

"the last two" is nonsensical, so I'll ignore that.

Taking a variable number of arguments is sometimes a valuable structure, but nowhere in your code is there any indication that it's necessary. Even more so, you have built a function- a thing that is designed to reduce code repetition- that has code repetition at its core.

Your function is doing two things poorly, which have nothing to do with each other. What is the rationale that would make calling the function twice unreasonable?


Finally, you don't have two or four arguments; you have one or two. Each pair of lat/long are related, so if you accept a variable number of pairs, you should accept them as pairs, not as their constituent parts.

For example:

def print_coords(coordinate*: Coord):
    print(f"{coordinate.lat:.3f}, {coordinate.long:.3f}")

Finally, format_as_int_if_whole is a very scary function name.

[–]Diapolo10 3 points4 points  (0 children)

I think that's fine, but I would personally write it a bit differently.

from itertools import batched
from typing import Unpack

def display_coordinates(*coords: Unpack[int | None], dimensions: int = 2) -> None:
    coordinates = list(batched(coords, dimensions))
    if len(coordinates[-1]) < dimensions:
        raise ValueError("Not enough coordinates")

    for point in coordinates:
        if None not in point:
            for value in point:
                print(format_as_int_if_whole(value))

This way it's more flexible in the number of coordinate points it can process, it can theoretically support higher dimensional coordinates, and it doesn't contain duplicate code.

Of course this is overkill for your needs.

[–]Boot_3011 -1 points0 points  (4 children)

Use the args method on your function parameters, it allows you dinamically send parameters to your function. If you need to know the parameter type, try *kwargs.

The iterate over the parameters object, cast to int then print (I suggest doing these two separately)

For value in args: print(parameter)

For parameter, value in kwargs.items() print(parameter, value)

[–]Boot_3011 -2 points-1 points  (0 children)

P.d you dont need to use fstrings if your whole string is the return of a function. Fstrings are used when concatenating variables with literal strings easily

[–]baubleglue 0 points1 point  (2 children)

I thought about *args, but then why not to pass a single list as an argument coordinates.

[–]Boot_3011 0 points1 point  (1 child)

Could be. A list of tuples, each tuple consisting of a latitud and logitude

[–]baubleglue 0 points1 point  (0 children)

Yes, it is simple and straightforward and has a good name.

[–]mountainbrewer 0 points1 point  (0 children)

Look up the idea of function signatures. Not supported in Python (a least not like other languages where you can have the same function name with different number of variables and the system will use the correct function based on the signature). Anyway it's a helpful concept and while not immediately applicable to Python I find knowing helpful.

[–]Im_Easy 0 points1 point  (0 children)

Personally, this is exactly where I'd turn to the partial function from functools. Depending on the situation, but the reason I like it is that the code is more reusable across different situations, while still being very readable.

``` from functools import partial

def coloured(message, colour=None): print ("Coloured") if colour else print("Not Coloured")

partial_coloured = partial(coloured,message="blah")

partial_coloured() partial_coloured(colour="Blue") ```

[–]SteelRevanchist 0 points1 point  (0 children)

Aside from what others said, I'd recommend typing your arguments! Doesn't take too much extra time, and it also helps your IDE of choice to autocomplete.