I personally might reduce some of the conditionals as in some cases they're just checking one item and can basically be placed into the same conditional statement.
function mymodule_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) { // Placed the $op and node type checks in the same conditional. // There's no particular reason they have to be separated since // the rest of the code is dependent upon both being TRUE. if ($op == 'presave' && $node->type == 'property') { $old = node_load($node->nid); if($old->field_usera_ref[0]['uid'] != $node->field_user_ref[0]['uid']){ // Also, it is not necessary to assign the string to $type. You can // just pass the string 'realtor_assign' directly without assigning it. // Since your second parameter in the function signature is $type this // string will automatically be assigned to it. mymodule_tn_complete($node, 'realtor_assign'); } if($old->field_userb_reference[0]['uid'] != $node->field_userb_ref[0]['uid']){ mymodule_tn_complete($node, 'am_assign'); } if($old->field_userc_ref[0]['uid'] != $node->field_userc_ref[0]['uid']){ mymodule_tn_complete($node, 'trep_assign'); } } } function mymodule_tn_complete($node, $type) { switch($type){ case 'realtor_assign': //Do Stuff break; case 'am_assign': //Do other stuff break; case 'trep_assign': // Do different stuff break; } }
Other than those personal preferences from experience, I don't really see anything wrong with it in principal. It looks good. This function is not making any database calls so it should not be putting must strain on the performance. Implementing the hook should not do that. Even if you're not implementing the hook other modules undoubtedly are.