Roast My Code (Color Switcher)

Coding Python Support
3 
for n in AN:
    if n.select:
        if n and n.bl_idname == "ShaderNodeBsdfAnisotropic":

You don’t need “if n”. Having it there is confusing, because it makes me wonder if n’s __bool__ method is overridden, or if n ever becomes None which doesn’t make sense because you’re accessing n.select in the previous line, etc etc.

If n was ever evaluated to False, something else is very, very wrong.

You can deduplicate your code by putting the strings of node types in a set,

node_bl_idnames = {
    "ShaderNodeBsdfAnisotropic",
    "ShaderNodeBsdfDiffuse",
    "ShaderNodeEmission", }  # ..and all the others.

… then do a check that covers all of the nodes.

for n in AN:
    if n.select and n.bl_idname in node_bl_idnames:
        n.inputs[0].default_value = hex_to_rgb(hex)
        material.diffuse_color = hex_to_rgb(hex)

        # (Nothing more to do now, actually. Drink_tea() maybe)

Multiple returns should return different values, otherwise it’s confusing to the reader.
If I were pedantic, I’d find it strange that a free function is returning a value that has no meaning outside of an operator method. It would make more sense to return a True/False value, which you check in your operator, then let it return a set with a value.

Note that returning {'FINISHED'} indicates a successful operation, causes the view layer to be updated and pushes an undo step for undo-enabled operators, while returning {'CANCELLED'} does nothing. If your operator uses undo, this has an impact.

A common approach is to use a changed variable.

changed = False

for a in b:
    if x or y:
        if z:
            changed = True

if not changed:
    ...
else:
    ...

Using a counter technically works, but it doesn’t show your intent. The pattern above is fairly common.

2 Likes