Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

How to do optimization and provide less of code here:

 for _ in _get_valid_json():
            if _['size'] != '':
                available_params = [
                    {'productSku': _['upcID']},
                    {'productUPC': _['upc']},
                    {'productColor': _['color']},
                    {'productSize': _['size']},
                    prod_price,
                    prod_name
                ]
            else:
                available_params = [
                    {'productSku': _['upcID']},
                    {'productUPC': _['upc']},
                    {'productColor': _['color']},
                    prod_price,
                    prod_name
                ]

I think it is not optimal way to use copy of same dict... maybe it can be done more dynamically?

share|improve this question
1  
Welcome on CodeReview.SE! Would you be able to tell us more about what you are trying to achieve ? –  Josay Jun 20 at 13:31
    
@Josay I think it is not optimal way to use copy of same dict... maybe it can be done more dynamically? –  user2734570 Jun 20 at 13:36
add comment

2 Answers 2

up vote 2 down vote accepted

You can append to a list after it has been created.

for _ in _get_valid_json():
    available_params = [
        {'productSku': _['upcID']},
        {'productUPC': _['upc']},
        {'productColor': _['color']}]

    if _['size'] != '':
        available_params.append({'productSize': _['size']})

    available_params.extend([prod_price, prod_name])

Taking a step back, this data structure looks like it will be a huge pain to work with. It is a list of key-value pairs where the ordering is significant (based on the fact that you wanted productSize at that specific location). So, not only do you need to know which value appears in which spot, you also have to know what the key name is to get the value. I don't know the use case here, but it seems like making an actual class would provide a much simpler interface to the user of the data.

share|improve this answer
    
Thanks you! it works great –  user2734570 Jun 20 at 14:16
    
To keep using _ as anything other than an unused loop variable borders on criminal. –  codesparkle Jun 20 at 22:20
    
@codesparkle Agreed. I was focused on creating a better structure for the code and didn't mention that. I up voted jonrsharpe's answer that puts that point front and center. –  unholysampler Jun 21 at 14:51
add comment

_ is used in Python, by convention, to mean "I'm not going to use this". For example:

random_nums = [random.randint(min_, max_) for _ in range(10)]

Here I'm creating a list of ten random numbers - I don't really need to use the index of each one, so I use the name _ to let readers of my code (which, as always, includes future me!) know not to worry about that value.

In your code, by comparison, _ is used for the variable that does everything. This is a terrible idea, given the convention above. Also, _ is pretty low-profile visually, but here it's representing the most important thing.

I don't know exactly what your _get_valid_json returns, so I can't give a really useful variable name, but will use d instead.


It is not clear why available_params is a list containing single-element dictionaries and whatever prod_price and prod_name are (also single-element dictionaries?) Also, there is duplication - why not set up what you always need first, then add to it if needed?

I think the following would make more sense:

for d in _get_valid_json():
    available_params = {'productSku': d['upcID'], 'productUPC': d['upc'],
                        'productColor': d['color']}
    available_params.update(prod_price) # assuming single-element dictionary
    available_params.update(prod_name) # ditto
    if d['size']:
        available_params['productSize'] = d['size']

Note that the empty string '' evaluates False in a boolean context, so you don't need the explicit comparison.

Now you can access the individual parameters much more easily (you don't need to go through the whole list looking for them) and if a function or method takes those parameters as arguments you can unpack the dictionary straight into it:

some_method(**available_params)
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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