Roman Suzi
2 min readDec 1, 2019

--

I agree with almost all of the advises here, except for the docstring for a trivial function and the proposal of providing columns as a list to a function.

Unless you generate an API docs for that or the function is part of some generic and popular library, in which case it will also need explicit types, it makes code less readable to have a docstring. If you need a comment to explain some gotchas, then for me it’s a sign the function will need to be improved at some moment.

As for providing columns to the function, depending on the case it may be bad decision. First of all, it’s no longer gen_table_sql, it’s render_column_list. Second, in this example situation I can foresee you may need more from table_obj, which will lead in the future to passing other parts of the table_obj or even what you pass will no longer be strings, but let’s say an OrderedDict with column information from several table_obj. For clarity, I’d drawn a line between terminal and non-terminal parts of the query. Terminals being strings, non-terminal being other objects (to serialize as SQL query or it’s parts) starting from the sql_query_obj, then SRP principle will be satisfied in a more meaningful way than just arbitrary decomposing into string-processing functions (your final version of the function can be just inlined).

Small notice: Even the gen_ prefix may be better replaced with the full word, whether it’s generate, serialize or what is the convention in your codebase. And _obj may be a noise word.

Also, I notices this:

query = …something…

return query

I found myself doing it a lot, because it makes inserting logging or debug without making diffs larger.

--

--

No responses yet