2

I have the following code (simplified):

def send_issue(issue):
    message = bot.send_issue(issue)
    return message

def send_issues(issues):
    return [send_issue(issue) for issue in issues]

As you see, send_issues and send_issue are non-pure functions. Is this considered a good practice (and Pythonic) to call non-pure functions in list comprehensions? The reason I want to do this is that this is convenient. The reason against this is that when you see a list comprehension, you expect that this code just generates the list and nothing more, but that's not the case.

UPD: I actually want to create and return the list contrary this question.

rominf
  • 2,719
  • 3
  • 21
  • 39
  • 2
    Related: https://stackoverflow.com/questions/5753597/is-it-pythonic-to-use-list-comprehensions-for-just-side-effects – Oliver Charlesworth Oct 18 '18 at 12:12
  • 2
    I would go against, if you are not intending to create a list. – Austin Oct 18 '18 at 12:13
  • As long as you're actually using the generated list as a list (meaning you are using indices to retrieve items of the generated list to do further processing I don't have a problem with it. If you intend to iterate over the list only, consider making it a generator expression rather than a list comprehension. If you don't care about the items at all, then you should use a regular `for` loop instead. – blhsing Oct 18 '18 at 12:15
  • I don't see any problem with this. I say go for it; the method names are simple and obvious, which means if I was looking at this code, I would know what it does quite easily. – loganasherjones Oct 18 '18 at 12:19
  • It's not obvious from the code itself that `send_issue` is non-pure. Does it send a message *and* return the contents of the sent message? – chepner Oct 18 '18 at 12:36
  • @chepner, yes it sends a message and returns the sent message, as written in the code. – rominf Oct 18 '18 at 12:43

1 Answers1

4

The question here is - Do you really need to create the list?

If this is so it is okay but not the best design.
It is good practice for a function to do only one thing especially if it has a side effect like I/O.
In your case, the function is creating and sending a message.
To fix this you can create a function that is sending the message and a function which is generating the message.

It is better to write it as.

msgs = [bot.get_message(issue) for issue in issues]
for msg in msgs:
     bot.send(msg)

This is clearer and widens the use of API while keeping the side effect isolated.

If you don't want to create another function you can at least use map since it says - "map this function to every element".

map(lambda issue: bot.send_issue(issue), issues) # returns a list

Also, the function send_issue is not needed because it just wraps the bot.send_issue. Adding such functions is only making the code noisy which is not a good practice.

Petar Velev
  • 2,305
  • 12
  • 24