4
\$\begingroup\$

I am learning programming, and I am thinking how to improve this code in a more pythonic way.

I have a function with number of arguments. However, any caller of the function can specify the arguments, and depending on the arguments given, I will return the dictionary. The given arguments must be only from the listed arguments.

The code that I have below has so much duplication (I remove carPower, carTime and carLease however it will do the same if/else part) How can I improve this?

def creating_result(my_struct, 
                carColour=None,
                carPrice=None,
                carGas=None,
                carBrand=None,
                carSpeed=None,
                carOwner=None,
                carPower=None,
                carTime=None,
                carLease=None,
                carEngine=None,
                carType=None
                ):  

     if carColour and isinstance(carColour, list):
        my_struct["carColour"] = { "thekey" : carColour}
     elif carColour:
        my_struct["carColour"] = carColour

     if carPrice and isinstance(carPrice, list):
        my_struct["carPrice"] = { "thekey" : carPrice}
     elif carPrice:
        my_struct["carPrice"] = carPrice

     if carGas and isinstance(carGas, list):
        my_struct["carGas"] = { "thekey" : carGas}
     elif carGas:
        my_struct["carGas"] = carGas

     if carBrand and isinstance(carBrand, list):
        my_struct["carBrand"] = { "thekey" : carBrand}
     elif carBrand:
        my_struct["carBrand"] = carBrand

     if carSpeed and isinstance(carSpeed, list):
        my_struct["carSpeed"] = { "thekey" : carSpeed}
     elif carSpeed:
        my_struct["carSpeed"] = carSpeed

     if carEngine:
        my_struct["carEngine"] = carEngine

     if carPrice:
        my_struct["carPrice"] = carPrice

     if carType:
        my_struct["carType"] = carType

     print "my_struct is ", my_struct

if __name__ == "__main__":
    # test 1st caller
    my_dict = {}
    creating_result(my_dict, carPrice=1, carColour="red")

    # test 2nd caller
    my_dict = {}
    creating_result(my_dict, carSpeed="200", carType="SUV", carPrice=300)

In case I can not change the list of arguments of the function above, how can I improve the (if/else) of the function above?

\$\endgroup\$
3
\$\begingroup\$

In all programming language, it is better that a function's name is a verb, or start with a verb. In your case, I would use create_result() instead of creating_result().

One this transformation is done, you would think of a better name for this function as it is quite too generic in its meaning. create_car() would be better and more meaningful in your context.

For the parameters you are passing, they should be written following the snake_name convention. Meaning, for example, carColour becomes car_color, carPrice becomes car_price, ... and so on.

More important: Regardless of the purpose of the parameters you are passing to the creating_result() function, their number is quite much which thing should lead to creating a class and use them as instance attributes.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the comment! especially for the camel case and wrapping this as a class. However maybe I should simplify the question, in the case I can not change the list of args, how can I improve the (if/else) from the given arguments above? \$\endgroup\$ – learningprogramming Oct 17 '17 at 18:56
  • \$\begingroup\$ You are welcome. Many if ... else statements are usually "reduced" by creating a dictionary. There are many posts about that issue on this website (Example) \$\endgroup\$ – Billal Begueradj Oct 17 '17 at 19:07
  • \$\begingroup\$ I think, in this case, the if ... else blocks come from some misunderstanding of what to put in a dict and why. So, "creating a dictionary" will actually not help here ;) They should be removed all together, I guess. \$\endgroup\$ – NichtJens Oct 17 '17 at 21:04
2
\$\begingroup\$

Your whole code can be rewritten as:

if __name__ == "__main__":
    # test 1st caller
    my_dict = {}
    my_dict.update(carPrice=1, carColour="red")
    print "my_struct is ", my_struct

    # test 2nd caller
    my_dict = {}
    my_dict.update(carSpeed="200", carType="SUV", carPrice=300)
    print "my_struct is ", my_struct

Which seems to be what you actually want :)

Besides the points raised by @Billal already, I don't understand why you are creating dicts that hold one item under the key "thekey". Can you elaborate on the idea behind that? I feel you might have misunderstood something here.

Thinking about this more, maybe it's even simpler and you only want to initialize the dicts like so:

if __name__ == "__main__":
    # test 1st caller
    my_dict = dict(carPrice=1, carColour="red")
    print "my_struct is ", my_struct

    # test 2nd caller
    my_dict = dict(carSpeed="200", carType="SUV", carPrice=300)
    print "my_struct is ", my_struct
\$\endgroup\$

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.