Skip to content

Conversation

rdumusc
Copy link

@rdumusc rdumusc commented Oct 29, 2018

No description provided.

@rdumusc
Copy link
Author

rdumusc commented Oct 29, 2018

retest this please

bool QmlStreamer::Impl::_isWithinSizeHintsRange(const uint width,
const uint height) const
{
const auto minWidth = _sizeHints.minWidth;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const auto minWidth = _sizeHints.minWidth;
const auto minWidth = _sizeHints.minWidth == SizeHints::UNSPECIFIED_SIZE
? 0
: _sizeHints.minWidth;
const auto maxWidth = _sizeHints.maxWidth == SizeHints::UNSPECIFIED_SIZE
? std::numeric_limits<uint>()
: _sizeHints.maxWidth;
const auto minHeight = _sizeHints.minHeight == SizeHints::UNSPECIFIED_SIZE
? 0
: _sizeHints.minHeight;
const auto maxHeight = _sizeHints.maxHeight == SizeHints::UNSPECIFIED_SIZE
? std::numeric_limits<uint>()
: _sizeHints.maxHeight;
return minWidth <= width && width <= maxWidth && minHeight <= height &&
height <= maxHeight;

Do you think this is easier to understand?

Copy link
Author

Choose a reason for hiding this comment

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

Doesn't make a big difference to me, but I can apply your suggestion. Maybe have having a single return point is better style?

Copy link
Contributor

Choose a reason for hiding this comment

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

Having everything constant and one return is a better style imo. But it does look a bit repeated. Anyhow, no big deal.

Copy link
Author

Choose a reason for hiding this comment

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

This got me thinking and I started looking if there was a good way to check if a value is withing a range and found (https://stackoverflow.com/questions/9255887/) the following option:

template <typename T>
struct Interval
{
    T low;
    T high;
    bool contains(const T value) const { return low <= value && value <= high; }
};
template <typename T>
Interval<T> interval(const T low, const T high)
{
    return Interval<T>{low, high};
}
// Then you can be more explicit about what you mean:
if (interval(a, b).contains(value))
    // ...

Might be an overkill for this use case but I like how clean this looks... :-)

@rdumusc rdumusc merged commit 75c562f into BlueBrain:master Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants