0
\$\begingroup\$

I have this block of code which clearly looks like duplicate, but I don't know how to refactor.

    float offsetDifference;
            nfloat newLogoHeight;
            nfloat newHeaderHeight;
            nfloat newLogoWidth;

            if (offset > _previousOffset)
            {
                offsetDifference = offset - _previousOffset;

                newHeaderHeight = (HeaderViewHeightConstraint.Constant - offsetDifference);
                newLogoHeight = (LogoImageViewHeightConstraint.Constant - offsetDifference);
                newLogoWidth = (LogoImageViewWidthConstraint.Constant - offsetDifference);


                HeaderViewHeightConstraint.Constant = newHeaderHeight < headerViewMinHeight ? headerViewMinHeight : newHeaderHeight;
                LogoImageViewHeightConstraint.Constant = newLogoHeight < logoViewMinHeight ? logoViewMinHeight : newLogoHeight;
                LogoImageViewWidthConstraint.Constant = newLogoWidth < logoViewMinWidth ? logoViewMinWidth : newLogoWidth;
            }
            else
            {
                offsetDifference = _previousOffset - offset;

                newHeaderHeight = (HeaderViewHeightConstraint.Constant + offsetDifference);
                newLogoHeight = (LogoImageViewHeightConstraint.Constant + offsetDifference);
                newLogoWidth = (LogoImageViewWidthConstraint.Constant + offsetDifference);

                HeaderViewHeightConstraint.Constant = newHeaderHeight > headerViewMaxHeight ? headerViewMaxHeight : newHeaderHeight;
                LogoImageViewHeightConstraint.Constant = newLogoHeight > logoViewMaxHeight ? logoViewMaxHeight : newLogoHeight;
                LogoImageViewWidthConstraint.Constant = newLogoWidth > logoViewMaxWidth ? logoViewMaxWidth : newLogoWidth;
            }

            //HeaderViewHeightConstraint.Constant = (headerViewMaxHeight - offset) < logoViewMaxHeight ? logoViewMaxHeight : (headerViewMaxHeight - offset);
            //LogoImageViewHeightConstraint.Constant = (logoViewMaxHeight - offset) < 0 ? 0 : (logoViewMaxHeight - offset);
            _previousOffset = offset;

Basically, I receive an offset, and according to the previous offset (wether is smaller or bigger), I reorganize some other values.

The thing is, the calculation is almost the same, and I'd love a method that just does the calculation in which I can pass the parameters, but I don't know how to write the "common code" since all the calculations varies in the + and -, or < and >.

Anyway, you're seeing this as well as I am, but you probably know how to refactor it better than I do. How would you do it ? :)

Note : I've rewritten most of this from my phone, sry for the terrible code formatting, it's really impractical. Feel free to edit

\$\endgroup\$
2
  • 2
    \$\begingroup\$ What is "this"? Please come up with a title that describes the purpose of your code. (See How to Ask.) Also clarify what the inputs and outputs are. \$\endgroup\$ Commented May 28, 2016 at 19:03
  • \$\begingroup\$ Clearly this means the following block of code, and all values are floats as described at the top. \$\endgroup\$
    – Gil Sand
    Commented May 28, 2016 at 19:04

5 Answers 5

1
\$\begingroup\$

If I take newHeaderHeight = HeaderViewHeightConstraint.Constant - offsetDifference and expand offsetDifference I get: newHeaderHeight = HeaderViewHeightConstraint.Constant - (offset - _previousOffset) which is newHeaderHeight = HeaderViewHeightConstraint.Constant - offset + _previousOffset

If I then go to the else statement and expand newHeaderHeight = (HeaderViewHeightConstraint.Constant + offsetDifference); I get newHeaderHeight = HeaderViewHeightConstraint.Constant + (_previousOffset - offset); which is newHeaderHeight = HeaderViewHeightConstraint.Constant + _previousOffset - offset;

They're the same thing. So you can basically remove them from the if/else statement and only write them once.

The second part is a bit more tricky to refactor because you compare it with min values in one condition and max values in the other. It would however still refactor each ternary operator statement with Math.Min() or Math.Max() (careful not to get them the other way round).

\$\endgroup\$
1
\$\begingroup\$

What about using something like

offsetDifference = Math.Abs(offset - _previousOffset) ;

then

newHeaderHeight = (HeaderViewHeightConstraint.Constant - offsetDifference);
newLogoHeight = (LogoImageViewHeightConstraint.Constant - offsetDifference);
newLogoWidth = (LogoImageViewWidthConstraint.Constant - offsetDifference); 

?

\$\endgroup\$
1
  • \$\begingroup\$ That will always decrease the values, but I'm guessing Zil wants to increase the values when offset is less than _previousOffset. \$\endgroup\$ Commented May 28, 2016 at 21:11
1
\$\begingroup\$

I have a few things to point out. Please note I'm not a C# developer.

Each of the *Constraing.Constant constants are:

  1. changing during runtime of the program, shouldn't constants be... constant?
  2. essentially boolean values (by the looks of it because of < & > operators at bottom of conditionals). I don't know what their initial values are before it hits this section of code, so this might not be true, but they will be boolean after it touches this area of your code.

I think seeing more scope around this code as well as what you are trying to solve would be helpful to make a useful suggestion.

\$\endgroup\$
1
  • \$\begingroup\$ #1 is a good point. For #2, Zil is using the conditional operator. So for int result = conditional ? value1 : value2;, if conditional is true, result gets assigned value1. Otherwise, result gets assigned value2. \$\endgroup\$ Commented May 28, 2016 at 21:40
0
\$\begingroup\$

I'm not a C# expert by any means, but in languages that support this I'd use first class functions. I think you can get something similar using delegates in C#. I think it would look something like this

delegate float MathOperation(float left, float right);
delegate bool Comparison(float left, float right);

static float add(float left, float right)
{
    return left + right;
}
static float subtract(float left, float right)
{
    return left - right;
}

static bool greaterThan(float left, float right)
{
    return left > right;
}
static bool lessThan(float left, float right)
{
    return left < right;
}
float offsetDifference;
nfloat newLogoHeight;
nfloat newHeaderHeight;
nfloat newLogoWidth;

Comparison comp;
MathOperation mop;

if (offset > _previousOffset)
{
    offsetDifference = offset - _previousOffset;
    comp = lessThan;
    mop = subtract;
}
else
{
    offsetDifference = _previousOffset - offest
    comp = greaterThan;
    mop = add;
}

newHeaderHeight = mop(HeaderViewHeightConstraint.Constant, offsetDifference);
newLogoHeight = mop(LogoImageViewHeightConstraint.Constant, offsetDifference);
newLogoWidth = mop(LogoImageViewWidthConstraint.Constant, offsetDifference);

and so on. You'll probably have to have another value that holds the min/max values depending on the branch.

\$\endgroup\$
0
\$\begingroup\$

As D. Jurcau mentioned, you can move that first block of code outside of the if and else and only write it once, and you can handle the min or max checks with Math.Min() and Math.Max(). To combine the second block together you can write a Clamp method that makes sure a value is between two other values, like this:

private static float Clamp(float value, float min, float max)
{
    return Math.Min(Math.Max(value, min), max);
}

Your if/else block can then be replaced with this:

float changeBy = previousOffset - offset;
HeaderViewHeightConstraint.Constant = Clamp(HeaderViewHeightConstraint.Constant + changeBy, headerViewMinHeight, headerViewMaxHeight);
LogoImageViewHeightConstraint.Constant = Clamp(LogoImageViewHeightConstraint.Constant + changeBy, logoViewMinHeight, logoViewMaxHeight);
LogoImageViewWidthConstraint.Constant = Clamp(LogoImageViewWidthConstraint.Constant + changeBy, logoViewMinWidth, logoViewMaxWidth);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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