3
\$\begingroup\$

Description:

Given an image X with known dimensions. We call a method CreateThumbnail to make a thumbnail (or not) with a given max-width and max-height (both optional).

CreateThumbnail call ComputeSize method (bad name?) which is responsable for doing the following logic:

If no max-width/max-height, the width and height should return as is. If only one is provided, then we call a method ResizeWidth on the Dimention2D object to return a new dimension based on the new width size (aspectRation is kept).

Same applies for height.

If both are provided, use the one, that while keeping the aspect ration, won't be bigger than their corresponding maximum.

If the image dimension are smaller than the maximum, no change to the dimensions.

Then, with the new size, the Resize is actually applied (or not) (this is outside the scope of this method).

private Dimention2D ComputeSize(Image image, int maxWidth, int maxHeight)
{
    if (image == null) throw new ArgumentNullException(nameof(image));
    var sourceDimension = new Dimention2D(image.Width, image.Height);

        bool computeWidth = maxWidth > 0 && sourceDimension.Width > maxWidth;
        bool computeHeight = maxHeight > 0 && sourceDimension.Height > maxHeight;

    if (computeWidth && computeHeight)
    {
        if (sourceDimension.Width / maxWidth > sourceDimension.Height / maxHeight)
            computeHeight = false;
        else
            computeWidth = false;
    }

    if (computeWidth)
        return sourceDimension.ResizeWidth(maxWidth);

    else if (computeHeight)
        return sourceDimension.ResizeHeight(maxHeight);

    else return sourceDimension;
}

I'm open to any suggestion. Variables/methods names, the use (or lack of) { and } on if/else, the if/else. and whatever thing you can think of making it prettier.

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

You can replace this:

if (computeWidth)
    return sourceDimension.ResizeWidth(maxWidth);

else if (computeHeight)
    return sourceDimension.ResizeHeight(maxHeight);

else return sourceDimension;

with this:

if (computeWidth)
    return sourceDimension.ResizeWidth(maxWidth);
if (computeHeight)
    return sourceDimension.ResizeHeight(maxHeight);
return sourceDimension;

I would rename the method and input parameters, as it would add more sense to the method name and input parameters.

private Dimention2D  ReSize(Image image, int requireWidth, int requireHeight);
\$\endgroup\$
  • \$\begingroup\$ clearly my naming is not good. This method does not do the actual resize. It determines what's the new size needs to be based on max-width and max-height input parameters. It returns a dimension object with contains Width and Height properties than later is passed to the actual Resize method. \$\endgroup\$ – Bart Calixto Apr 20 '16 at 23:13
  • 1
    \$\begingroup\$ Added a description so people can better help with naming. I agree is not good. \$\endgroup\$ – Bart Calixto Apr 20 '16 at 23:22
1
\$\begingroup\$

You have a typo in your Dimention2D class name. It should probably be Dimension2D:

private Dimension2D ComputeSize(Image image, int maxWidth, int maxHeight)
{

I recommend using braces instead of one liners for anything longer than if (something) return; (same with continue etc.).

    if (image == null)
    {
        throw new ArgumentNullException(nameof(image));
    }

    var sourceDimension = new Dimension2D(image.Width, image.Height);

You had some wrong indentation in the following two lines. Not sure if it was due to copying to your question, or also in your actual code:

    bool computeWidth = maxWidth > 0 && sourceDimension.Width > maxWidth;
    bool computeHeight = maxHeight > 0 && sourceDimension.Height > maxHeight;

Again I recommend braces for better readability:

    if (computeWidth && computeHeight)
    {
        if (sourceDimension.Width / maxWidth > sourceDimension.Height / maxHeight)
        {
            computeHeight = false;
        }
        else
        {
            computeWidth = false;
        }
    }

In the following, when you have conditional returns, you don't need an else. Also it is unclear what your sourceDimension.ResizeWidth(maxWidth); does. Does it change the Dimension2D instance and return itself? Does it create a new instance with the new values?

Since resizing is usually associated with real objects, like for example images, for Dimensions I would use the method name myDimension2D.ScaleToBounds(Dimension2D bounds) instead. Then your algorithm to recalculate the dimensions might actually go into the Dimension2D class/struct.

    if (computeWidth)
    {
        return sourceDimension.ResizeWidth(maxWidth);
    }

    if (computeHeight)
    {
        return sourceDimension.ResizeHeight(maxHeight);
    }

    return sourceDimension;
}
\$\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.