1

I have a nodelist object that yields a bunch of node objects on iteration. I need to increment these nodes based on some random condition (like node.x > 17). Here's what I'm doing right now:

for node in nodelist: 
    if node.x > 17: 
        node.x += 1

I can't do map(lambda node: node.x += 1, nodelist) because lambda can't contain assignment. I cannot do nodelist = [node.x + 1 for node in nodelist if...] because a nodelist object consists of more than just its child nodes.

Is there a way to make this shorter/cleaner?

Improve this question
2
  • 6
    What's wrong with it now? It seems pretty straightforward to me. Commented May 11, 2011 at 18:52
  • -1: How can assignment be "cleaner". State change is absolutely central to procedural programming. The assignment statement makes state change as clear as possible. Few things can be as important as the assignment statement. Your examples seem to make the central, critical assignment statements murky and obscure. Commented May 11, 2011 at 20:35

4 Answers 4

Reset to default
7

I think it's already pretty short and clean. Python provides some powerful tools, but sometimes simple is best; this seems like one of those times.

I also think what you have is more readable than any equivalent one-liner (that I can come up with).

Ok, that said, you could do this:

for node in (n for n in nodelist if condition):
    node.x += 1

That could even be compressed into one line. But I honestly prefer the way you have it now.

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

Aye, I'd have to agree that your for loop is the short and clean solution. Unless you're aiming for some "smallest code" contest answer, your code is good. If you were building a list or some such, then a list comprehension might make sense.
I prefer this approach myself.
Thanks. I suppose this "pythonic" concept has gotten to my head, and now I'm insecure about all of my python code ;)
3

You can actually assign in a lambda:

import operator
map(lambda node: operator.iadd(node.x,1), nodelist)

respectively

[operator.iadd(node.x,1) for node in nodelist if ...]

apart from that, your code is not really bad - it is quite concise and easy to read as is.

Improve this answer

3 Comments

This is shorter (in lines), but I doubt many people would find it cleaner.
I fully agree with you - I just want to point out the possibility.
This actually doesn't work if you test it (though seems like it should). Try: class Node(object): pass and then doing nodelist = [Node(), Node()] and then nodelist[0].x = 19; nodelist[1].x=10; to initialize values, then nodelist ([node.x for node in nodelist]) is originally [19,10] and after applying your code [operator.iadd(node.x,1) for node in nodelist if node.x > 17] it outputs [20], but looking back at nodelist its unaltered -- still [19,10]. Best to stick with the simple for loop solution.
0

I vote that You are overdoing this pythonic thing :)

What is your goal: performance? I doubt You will get any.

Readability? Well, for three lines, I am not sure if functional collapse will help in this case, as it obviously will not be so obvious.

Improve this answer

1 Comment

I would even go out on a limb and say that the OP's original code is pythonic.
0

You can actually use lambda:

map(lambda node: node.x>17 and node.x+1 or node.x, nodelist)
Improve this answer

Comments

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.