[Bf-codereview] Node Tools r4422 review (issue 7651047)

[ideasman42 at gmail.com]

Reviewers: bf-codereview_blender.org,

Message:
Code review & some suggestions.


https://codereview.appspot.com/7651047/diff/1/node_efficiency_tools.py
File node_efficiency_tools.py (right):

https://codereview.appspot.com/7651047/diff/1/node_efficiency_tools.py#newcode103
node_efficiency_tools.py:103: def set_convenience_variables(context):
suggest to not use globals here and just:
return nodes, links

... so using would look like this (renamed set --> get)

nodes, links = get_convenience_variables()

https://codereview.appspot.com/7651047/diff/1/node_efficiency_tools.py#newcode138
node_efficiency_tools.py:138: def poll(cls, context):
All both functions are the same, use use a mix-in class: eg

class NodeToolBase:
     @classmethod
     def poll(cls, context):
         space = context.space_data
         return space.type == 'NODE_EDITOR' and space.node_tree is not
None

... then use the class like this and dont include a poll function in the
operator subclass.
class MergeNodes(bpy.types.Operator, NodeToolBase):

https://codereview.appspot.com/7651047/diff/1/node_efficiency_tools.py#newcode191
node_efficiency_tools.py:191: for the_list in [selected_mix,
selected_shader, selected_math]:
'the_list' is used for different purposes, would prefer to use a name
that makes sense for each use.

https://codereview.appspot.com/7651047/diff/1/node_efficiency_tools.py#newcode197
node_efficiency_tools.py:197: loc_x = the_list[0][1] + 350.0
*picky* - personal preference to use 2d vectors for all locx,locy in
this script.

# if you dont want to be accessing the nodes original location, this
copies the vector.
xy = node.location.copy()

https://codereview.appspot.com/7651047/diff/1/node_efficiency_tools.py#newcode603
node_efficiency_tools.py:603: node.location = [x,y]
*picky*, you can just do this.
node.location = x, y

https://codereview.appspot.com/7651047/diff/1/node_efficiency_tools.py#newcode799
node_efficiency_tools.py:799: use_node_name = eval(option_split[1])
Using eval() just to pass 3 bools is not really good idea.
For this we have bpy.props.BoolVectorProperty(), an array of bools.

Goes for all other uses of eval() here.

https://codereview.appspot.com/7651047/diff/1/node_efficiency_tools.py#newcode858
node_efficiency_tools.py:858: class AlignNodes(bpy.types.Operator):
*picky* - again, this would benefit from using 2d Vector()'s
(readability)

https://codereview.appspot.com/7651047/diff/1/node_efficiency_tools.py#newcode976
node_efficiency_tools.py:976: class
SelectParentChildren(bpy.types.Operator):
This could be added into blenders C code, seems generally useful and we
have for object-mode, pose-mode.



Please review this at https://codereview.appspot.com/7651047/

Affected files:
   M     node_efficiency_tools.py


Index: node_efficiency_tools.py
===================================================================
--- node_efficiency_tools.py	(revision 4422)
+++ node_efficiency_tools.py	(working copy)
@@ -1493,4 +1493,3 @@

  if __name__ == "__main__":
      register()
-
\ No newline at end of file