Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions WatchApp Extension/Scenes/GlucoseChartScene.swift
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,11 @@ struct Scaler {
return CGPoint(x: CGFloat(x.timeIntervalSince(startDate)) * xScale, y: CGFloat(y - glucoseMin) * yScale)
}

func rect(for range: WatchDatedRange) -> CGRect {
func rect(for range: WatchDatedRange, minHeight: CGFloat = 2) -> CGRect {
let a = point(range.startDate, range.minValue)
let b = point(range.endDate, range.maxValue)
let size = CGSize(width: b.x - a.x, height: b.y - a.y)
// Enforce a minimum height so that narrow target ranges still show up:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This comment doesn't make sense here, since Scaler is a implementing an API about rectangles. The fact that there's a minHeight implies that the user can specify a minimum height for whatever reason they want.

Two options:

  1. Associate the comment with the default value
  2. Put the comment in the place where it's relevant, which is where rect() is called

The fact that we have a default value is really just a shortcut that's breaking the API anyway. If you really want this to be clean, then the minHeight should have no default value and we should specify "2" in the caller AND comment the caller.

Then the caller is saying "make a rectangle between these two points, but since I know that I'm drawing target ranges- also make sure that the height is 2." That's worthy of a comment, and then the Scaler API is clean.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'd agree with you if this were a generic rectangle-drawing function, but it's not - despite the generic title, it's a function that is always drawing a WatchDatedRange. So it doesn't make sense to imagine that there could be some other possible call to it, that would be drawing something else. That's why I didn't have it in the call when I wrote it originally. I still think it's cleaner that way, with just a minimum height in the CGSize call and a comment to explain it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Just to clarify a bit more, you're saying that the comment is relevant where minHeight is defined in the call, but I disagree - it becomes relevant when you get to the point where it's actually used.

Copy link
Copy Markdown
Owner

@bharat bharat Aug 2, 2018

Choose a reason for hiding this comment

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

Sorry for the confusion and the excessive back and forth over what probably appears to be a very minor detail. I don't think it's minor though - which I'll discuss at the end of this response.

At a high level what I'm saying that putting a comment here is a bad code smell: the code should be able to stand on its own without this level of micro comment. In my prior comment, I was suggesting two different ways that we could address this issue. I think we have 3 options, which I'll walk through.

  1. If you were to provide a minHeight variable, and you were to set it to zero by default (ie, not overly specializing for WatchDatedRange), then you were to override the default value in the caller (eg. scaler.rect(for: range, minHeight: 2)), you would put the comment where you do the override which would be in the caller. The comment would explain that if the minimum height is too small, the range is not visible and you'd probably only need one meta-comment for both calls.

  2. If you want to keep the minHeight default value of 2 as part of the API then you would make the comment as part of the rect API call, eg:

// By default enforce a minimum height so that the range is visible
func rect(for range: WatchDatedRange, minHeight: CGFloat = 2) -> CGRect {
  1. Revert the minHeight variable, embed 2 in the code and make it part of the rect API docs. You make a really good point that this is not a generic rect drawing function - it's specialized for WatchDatedRange, but the hardcoded constant should be part of the API definition, not an implementation detail. So if you go that route, it'd be:
// By default enforce a minimum height of 2 pixels so that the range is visible
func rect(for range: WatchDatedRange) -> CGRect {
  ...
  let size = CGSize(width: b.x - a.x, height: (b.y - a.y, 2))

Any of these options resolves the problem for me, which is that we should always prefer contractual API level comments over implementation detail comments. If we don't hold the line on this, the code rapidly slides into excessive micro comments that take up space and harm maintainability.

let size = CGSize(width: b.x - a.x, height: max(b.y - a.y, minHeight))
return CGRect(origin: CGPoint(x: a.x + size.width / 2, y: a.y + size.height / 2), size: size)
}
}
Expand All @@ -74,15 +75,15 @@ extension HKUnit {
if unitString == "mg/dL" {
return [150.0, 200.0, 250.0, 300.0, 350.0, 400.0]
} else {
return [8.3, 11.1, 13.9, 16.6, 19.4, 22.2]
return [8.0, 11.0, 14.0, 17.0, 20.0, 23.0]
}
}

var lowWatermark: Double {
if unitString == "mg/dL" {
return 50.0
} else {
return 2.8
return 3.0
}
}
}
Expand Down Expand Up @@ -242,23 +243,23 @@ class GlucoseChartScene: SKScene {

targetRanges?.forEach { range in
let sprite = getSprite(forHash: range.hashValue)
sprite.color = UIColor.rangeColor.withAlphaComponent(temporaryOverride != nil ? 0.2 : 0.4)
sprite.color = UIColor.rangeColor.withAlphaComponent(temporaryOverride != nil ? 0.4 : 0.6)
sprite.move(to: scaler.rect(for: range), animated: animated)
inactiveNodes.removeValue(forKey: range.hashValue)
}

// Make temporary overrides visually match what we do in the Loop app. This means that we have
// one darker box which represents the duration of the override, but we have a second lighter box which
// extends to the end of the visible window.
if let range = temporaryOverride {
if let range = temporaryOverride, range.endDate > Date() {
let sprite1 = getSprite(forHash: range.hashValue)
sprite1.color = UIColor.rangeColor.withAlphaComponent(0.2)
sprite1.color = UIColor.rangeColor.withAlphaComponent(0.6)
sprite1.move(to: scaler.rect(for: range), animated: animated)
inactiveNodes.removeValue(forKey: range.hashValue)

let extendedRange = WatchDatedRange(startDate: range.startDate, endDate: Date() + window, minValue: range.minValue, maxValue: range.maxValue)
let sprite2 = getSprite(forHash: extendedRange.hashValue)
sprite2.color = UIColor.rangeColor.withAlphaComponent(0.2)
sprite2.color = UIColor.rangeColor.withAlphaComponent(0.4)
sprite2.move(to: scaler.rect(for: extendedRange), animated: animated)
inactiveNodes.removeValue(forKey: extendedRange.hashValue)
}
Expand Down
Binary file not shown.

This file was deleted.

Binary file not shown.

This file was deleted.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"images" : [
{
"idiom" : "watch",
"filename" : "1-hour-graph-38mm.png",
"screen-width" : "<=145",
"scale" : "2x"
},
{
"idiom" : "watch",
"filename" : "1-hour-graph-42mm.png",
"screen-width" : ">145",
"scale" : "2x"
}
],
"info" : {
"version" : 1,
"author" : "xcode"
}
}
Binary file not shown.

This file was deleted.

Binary file not shown.

This file was deleted.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"images" : [
{
"idiom" : "watch",
"filename" : "2-hour-graph-38mm.png",
"screen-width" : "<=145",
"scale" : "2x"
},
{
"idiom" : "watch",
"filename" : "2-hour-graph-42mm.png",
"screen-width" : ">145",
"scale" : "2x"
}
],
"info" : {
"version" : 1,
"author" : "xcode"
}
}
Binary file not shown.

This file was deleted.

Binary file not shown.

This file was deleted.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"images" : [
{
"idiom" : "watch",
"filename" : "3-hour-graph-38mm.png",
"screen-width" : "<=145",
"scale" : "2x"
},
{
"idiom" : "watch",
"filename" : "3-hour-graph-42mm.png",
"screen-width" : ">145",
"scale" : "2x"
}
],
"info" : {
"version" : 1,
"author" : "xcode"
}
}
12 changes: 3 additions & 9 deletions WatchApp/Base.lproj/Interface.storyboard
Original file line number Diff line number Diff line change
Expand Up @@ -389,23 +389,17 @@
</items>
<menu key="menu" id="Da1-jh-2yL">
<items>
<menuItem title="1 hour" image="1-hour-graph-38mm" id="vL1-NA-WZ1">
<variation key="device=watch38mm" image="1-hour-graph-38mm"/>
<variation key="device=watch42mm" image="1-hour-graph-42mm"/>
<menuItem title="1 hour" image="1-hour-graph" id="vL1-NA-WZ1">
<connections>
<action selector="setChartWindow1Hour" destination="v5b-sO-bb8" id="yp1-H2-0e5"/>
</connections>
</menuItem>
<menuItem title="2 hours" image="2-hour-graph-38mm" id="dPh-7b-Tfv">
<variation key="device=watch38mm" image="2-hour-graph-38mm"/>
<variation key="device=watch42mm" image="2-hour-graph-42mm"/>
<menuItem title="2 hours" image="2-hour-graph" id="dPh-7b-Tfv">
<connections>
<action selector="setChartWindow2Hours" destination="v5b-sO-bb8" id="IQe-Uh-AOb"/>
</connections>
</menuItem>
<menuItem title="3 hours" image="3-hour-graph-38mm" id="fR1-7h-SNe">
<variation key="device=watch38mm" image="3-hour-graph-38mm"/>
<variation key="device=watch42mm" image="3-hour-graph-42mm"/>
<menuItem title="3 hours" image="3-hour-graph" id="fR1-7h-SNe">
<connections>
<action selector="setChartWindow3Hours" destination="v5b-sO-bb8" id="HhZ-89-5Yp"/>
</connections>
Expand Down