3
\$\begingroup\$

I'm trying to implement some simple logic using rails. Code is working fine but its look uglier to me, could any one help me on refractor or implement in a efficient way.

Context:

Data hash assetType key which can be image/clip and Sizes key has array of hashes.

if asset type is image then I'm filtering Sizes array if each hash has TeeShirtSize or not.

Same way all Sizes array of hashes should have Description and be unique.

each statement runs only if Sizes array not empty.

def get_parsed_asset_data(data)
     data['Sizes'].select!{|size| size.key?('TeeShirtSize')} if data['AssetType'] == 'Image' && data['Sizes'].any?
     data['Sizes'].select!{|size| size.key?('Description')} if data['Sizes'].any?
     data['Sizes'].uniq!{|size| size['Description']} if data['Sizes'].any?
     data
  end
\$\endgroup\$
0
0
\$\begingroup\$

I don't think you need to check for any? if its empty, you will get an empty result.

def get_unique_descriptions(sizes, type_not_image)
   # just in case data['Sizes'] can be nil
  (sizes || []).select do |size| 
     # needed condition in every case
     size.key?('Description') && 
          # pass when AssetType != Image
          (  type_not_image ||
             # needed condition when data['AssetType'] == 'Image' 
             size.key?('TeeShirtSize'))   
  end.uniq {|size| size['Description']}  # return only the Sizes, not the whole data Hash
end

descriptions = get_unique_descriptions(data['Sizes'], data['AssetType'] != 'Image')
\$\endgroup\$
2
  • \$\begingroup\$ sizes.select{|size| size.key?('Description') && (not_image ||size.key?('TeeShirtSize'))}.uniq{|size| size['Description']} just one line \$\endgroup\$ – no_lyf_programmer Aug 18 '16 at 22:56
  • \$\begingroup\$ Yes, one line is possible. It did only stretch it to be more explicit. \$\endgroup\$ – slowjack2k Aug 19 '16 at 6:19
0
\$\begingroup\$

First of all, I'd always avoid side-effects. That is, don't modify an object that was passed in; return a new object instead. So no !-methods.

Second, I'd reduce the method to only handle the Sizes array itself. Don't call the method unless there's something to process. That means checking AssetType and data['Sizes'].any? elsewhere.

Third, the name of the method is non-descriptive, in my opinion. It doesn't get anything, since it modifies what you give it, and "parsed asset data" is so generic that it doesn't tell me what the method actually does. Obviously it's something to do with t-shirts and their sizes and descriptions, so that should probably feature in the name.

Then you're left with something like:

def get_unique_tshirt_sizes(sizes) # or some other name
  sizes
    .select { |size| size.key?('TeeShirtSize') && size.key?('Description') }
    .uniq { |size| size['Description'] }
end

This method operates only on the sizes array, and it returns a new array rather than destructively updating an existing one.

You might use it like so:

if data['AssetType'] == 'Image' && data['Sizes'].any?
  sizes = get_unique_tshirts_sizes(data['Sizes'])
end
\$\endgroup\$
2
  • \$\begingroup\$ I got your point, This code works for Image asset type only but issue here is the last two statements(Description and uniqueness ) should execute even if assettype is not an image. \$\endgroup\$ – no_lyf_programmer Aug 17 '16 at 21:08
  • \$\begingroup\$ @tumati Ah, right, makes sense. Though that's even more reason to break out a method like the one above, since that's the two statements that are used for everything (name has to change though). Have a separate method do the image-asset-specific filtering, and then pass stuff through the method above \$\endgroup\$ – Flambino Aug 17 '16 at 21:11

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.