1
\$\begingroup\$

Basically, if the function parameter is None, I ignore it, else I concatenate all arguments like arg1:arg1_value AND arg2:arg2_value AND arg4:arg4_value considering arg3==None when the function got called.

def _get_query_string_for_get_config(self, host, app_name, instance, config_source, config_name):
    query_string = ""
    if host is not None:
        query_string += "host:{} AND ".format(host)
    if app_name is not None:
        query_string += "app_name:{} AND ".format(app_name)
    if instance is not None:
        query_string += "instance:{} AND ".format(instance)
    if config_source is not None:
        query_string += "config_source:{} AND ".format(config_source)
    if config_name is not None:
        query_string += "config_name:{}".format(config_name)

    return query_string
\$\endgroup\$
3
  • \$\begingroup\$ Consider renaming the method as its name is awfully verbose. \$\endgroup\$ – Richard Neumann Nov 21 '16 at 17:18
  • \$\begingroup\$ Sure. Thanks @RichardNeumann for the input. \$\endgroup\$ – Abhijeet Rastogi Nov 21 '16 at 18:54
  • \$\begingroup\$ Additionally, if you want to specify what the method is doing, consider adding a docstring. \$\endgroup\$ – Richard Neumann Nov 21 '16 at 19:08
5
\$\begingroup\$

Concatenating strings with a common connector (such as ' AND ' here) is most idiomatically done using join. So the idea should be to:

return ' AND '.join(<some list with the right `text:value` formatting>)

The easiest way to get such list should be to pair the text part and the associated value using zip and then use a list comprehension to filter out undesired variables:

parameters = zip(
    ('host', 'app_name', 'instance', 'config_source', 'config_name'),
    (host, app_name, instance, config_source, config_name)
)
query = ['{}:{}'.format(text, value) for text, value in parameters if value is not None]
return ' AND '.join(query)

You can also simplify a bit by turning the list-comprehension into a generator expression and feeding it directly to join.

Lastly, the self parameter suggest that you are using this as a method of a class. But since you are not using self in the method, you should either make it a regular function or a saticmethod:

@staticmethod
def _get_query_string_for_get_config(host, app_name, instance, config_source, config_name):
    parameters = zip(
        ('host', 'app_name', 'instance', 'config_source', 'config_name'),
        (host, app_name, instance, config_source, config_name)
    )
    return ' AND '.join(
        '{}:{}'.format(text, value)
        for text, value in parameters
        if value is not None)
\$\endgroup\$
0
2
\$\begingroup\$

Not sure if you even need a function for this. Just make a dictionary with all these parameters and then you can do:

' AND '.join('{}:{}'.format(key, value) for key, value in my_dict.iteritems())
\$\endgroup\$
3
  • \$\begingroup\$ This will not preserve the order of the arguments. \$\endgroup\$ – Richard Neumann Nov 21 '16 at 17:11
  • \$\begingroup\$ @RichardNeumann yes, but since this is a query, order of and filter does not matter. \$\endgroup\$ – Alex Nov 21 '16 at 18:05
  • \$\begingroup\$ @Richard And if it is really needed (because one variable is almost always False and you want to use short circuit evaluation) you can just make my_dict a collections.OrderedDict. \$\endgroup\$ – Graipher Nov 21 '16 at 18:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.