extract methods from Ticket.save_changes() to improve readability

[felix.schwarz@…]

I'd like to improve the readability of Ticket.save_changes() by extracting a few pieces of code into specialized private methods.

[Felix Schwarz <felix.schwarz@…>]

improve readability of Ticket.save_changes by extracting code to update owner on component update

[Felix Schwarz <felix.schwarz@…>]

improve readability of Ticket.save_changes by extracting code to sanitize the cc list

[Felix Schwarz <felix.schwarz@…>]

improve readability of Ticket.save_changes by extracting code to save changes in custom fields

[Remy Blank]

I'm not really convinced that this buys anything. Basically, you trade a comment for a method name. I would be more positive if the extracted methods were re-used elsewhere, but they are single-use methods. So in the current form, I'm rather -1.

[Felix Schwarz <felix.schwarz@…>]

Replying to rblank:

I'm not really convinced that this buys anything. Basically, you trade a comment for a method name. I would be more positive if the extracted methods were re-used elsewhere, but they are single-use methods.

I'd like to make a point why methods are better than comments and why it does not matter if methods are re-used elsewhere:

• When you read code, method calls are easier to skip things that you're not interested in because you don't have to look where the code block ends
• methods make it more clear which input and output is expected, local variable names are less likely to clash
• by moving code into a new method you keep methods short, therefore increase readability and decrease the likelihood of bugs

In a way short methods are the code equivalent to the Un*x philosophy "Write programs that do one thing and do it well".

[Remy Blank]

Sure, I agree with the general idea. But it still bothers me when methods must be named _update_owner_if_component_changed_on_new_ticket() and _fix_cc_separators_and_remove_duplicates() to be understood, so in this specific context, I'd rather keep the code linear.

[Christian Boos]

I agree with Remy, however what would work for me is going in the direction of a more generic save_changes, where the idiosyncrasies related to each field would somehow get abstracted away (see FieldRefactoring, substitute "render" with "save" or "normalize").

[Remy Blank]

After deliberations, we think that there is too little value to split the save_changes() method.