Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modularize SanitizedFile#copy_to and SanitizedFile#move_to #1850

Merged
merged 1 commit into from Feb 4, 2016

Conversation

oss92
Copy link
Contributor

@oss92 oss92 commented Jan 27, 2016

I have edited some parts to decrease complexity and code duplication.

reference_scale = scale_x >= scale_y ? scale_x : scale_y
cols = (reference_scale * (cols + 0.5)).round
rows = (reference_scale * (rows + 0.5)).round
reference_scale == scale_x ? cmd.resize("#{cols}") : cmd.resize("x#{rows}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this assignment required?

@thomasfedb
Copy link
Contributor

I like the changes to SanitizedFile. The others I might need a little more convincing of.

@oss92
Copy link
Contributor Author

oss92 commented Jan 28, 2016

As for the Mounter, The init_uploaders method saved a lot of code duplication.

As for MiniMagick, I guess assignment would be required. Maybe change reference_scale = scale_x >= scale_y ? scale_x : scale_y to scale_x = scale_y > scale_x ? scale_y : scale_x if you do not want an extra variable. But it would be less readable. What do you think?

@thomasfedb
Copy link
Contributor

Perhaps something more like this:

scale_x, scale_y = width / cols.to_f, height / rows.to_f
scale = [scale_x, scale_y].max
cols, rows = [cols, rows].map {|dim| dim * scale }

@oss92
Copy link
Contributor Author

oss92 commented Jan 28, 2016

I thought about this, but then you will have to deal with this scale == scale_x ? cmd.resize("#{cols}") : cmd.resize("x#{rows}").

Anyways I updated it.

@thomasfedb
Copy link
Contributor

@oss92 Would you be able to split out the changes to SanitizedFile into a separate PR? I'd be very happy to merge those in without much trouble. The others might take a bit longer.

@oss92
Copy link
Contributor Author

oss92 commented Feb 2, 2016

Done. Updated this pull request and will create another for the other changes if you'd like.

@oss92 oss92 changed the title Less code duplication on multiple modules Modularize SanitizedFile#copy_to and SanitizedFile#move_to Feb 2, 2016
thomasfedb added a commit that referenced this pull request Feb 4, 2016
Refactor file manipulation methods in SanitizedFile
@thomasfedb thomasfedb merged commit aa74e9e into carrierwaveuploader:master Feb 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants