UMN-LATIS / leaflet-treering

Use link below to test non-released changes.
https://umn-latis.github.io/leaflet-treering/
0 stars 1 forks source link

Associated year bugs #101

Closed thorn573 closed 3 years ago

thorn573 commented 3 years ago

Bug 1: If an annotation was placed next to an start point with no points following it, script would break down.

Bug 2: Calculated year attribute was not renewed each time a change was made.

Both bugs have been remedied.

cmcfadden commented 3 years ago

Seems reasonable.

(this isn't a change suggestion, just a comment) In general, from a code style perspective, getting the point where you've got a whole bunch of conditional cases chained together can make logical reasoning hard. IE:

var myVal = 0;
if(condition1 == 3) {
myVal = 1;
}
else if(condition1 == 2 && condition2 < 5) {
myVal = 2;
}
else if(condition1 > 3 && condition3 < 6) {
myVal = 3;
}
else {
myVal = -1;
}

Instead, you can refactor that logic into its own method and use early returns to make things more explicit:

function resolveConditions(condition1, condition2, condition3) {
if(condition1 == 3) {
return 1;
}

 if(condition1 == 2 && condition2 < 5) {
return 2;
}

if(condition1 > 3 && condition3 < 6) {
return 3;
}

return -1;
}
thorn573 commented 3 years ago

Ah ok, I'll watch for that in the future. Does calling methods within methods cause speed issues? Or does it not matter at the scale this is happening?

cmcfadden commented 3 years ago

Not in a way that would matter at this scale. Technically it's a few extra instructions so I suppose you could quantify it - half a nanosecond or something.