alterm4nn / ChronoZoom

ChronoZoom is an interactive timeline for all of history.
http://www.chronozoom.com
160 stars 127 forks source link

CleanCode: Timescale: method CosmosTickSource.getMarkerLabel has unused if #199

Closed a-chumagin closed 11 years ago

a-chumagin commented 11 years ago

Observed:

   this.getMarkerLabel = function (range, time) {
            var text;
            this.getRegime(range.min, range.max);
            var n = Math.max(Math.floor(Math.log(this.delta * Math.pow(10, this.beta) / this.level) * this.log10), -4) - 1;
            if(n > 20) {
                n = 20;
            }
            if(n < -20) {
                n = -20;
            }
            if(n < 0) {
                text = (new Number(-time / this.level)).toFixed(-n);
            } else {
                text = (new Number(-time / this.level)).toFixed(n);
            }
            text += " " + this.regime;
            return text;
        };
  1. if(n < -20) - Condition never be true, because n is more than -3 always. See var n = Math.max(Math.floor(Math.log(this.delta * Math.pow(10, this.beta) / this.level) * this.log10), -4) - 1
  2. function
           if(n < 0) {
                text = (new Number(-time / this.level)).toFixed(-n);
            } else {
                text = (new Number(-time / this.level)).toFixed(n);
            }

Change to text = (-time/this.level).toFixed(Math.abs[n])

  1. "text", "n" - are not clear names. Use more intelligible

Link to trello card: https://trello.com/c/P4QjkuQJ

GiorgiyLutidze commented 11 years ago

fixed in https://github.com/GiorgiyLutidze/ChronoZoom/commit/e254c44188d50d03d02f5be2ed4a0095adaee21e

a-chumagin commented 11 years ago

@GiorgiyLutidze I cannot close bug. There is not unit tests for changes.

a-chumagin commented 11 years ago

There is no unit tests

alterm4nn commented 11 years ago

Code changed significantly.

a-chumagin commented 11 years ago

closed