6
\$\begingroup\$

I've built a custom control to handle user input in a human-readable format, but save in a machine-format.
It needs to have input validation, and to convert UI-input to a base format.

I'm interested in knowing:

  • Have I implemented this in a proper and idiomatic Angular way?
  • Are there any UI and typescript mistakes?
  • Have I missed anything in the implementation?
  • Is there a better way to implement this sort of behaviour?

In this case, the custom control is used to input latitudes and longitudes in "degree minute decimal-seconds" (47° 22' 0.4959"N) and save them in "decimal degrees" (47.366804405489006).

Each degree (°) can be broken into 60 minutes ('). Each minute can be divided into 60 seconds (").


Main code

coordinate-input.component.html

<input #box
       [placeholder]="placeholder"
       (blur)="triggerLostFocus()"
       (change)="triggerInputChange(box.value)" />

coordinate-input.component.scss

:host {
  display: block;
}

input {
  width: 100%;
}

coordinate-input.component.ts

import { Component, Input, Renderer2, ViewChild, ElementRef, forwardRef } from '@angular/core';
import { ControlValueAccessor, NG_VALUE_ACCESSOR, Validator, ValidationErrors, AbstractControl, NG_VALIDATORS } from '@angular/forms';
import { degreesToDMS, dmsToDegrees, DmsParseResult } from 'app/services/utils';
import { GeographicPositionType } from 'app/models/position';
import { TranslateService } from '@ngx-translate/core';

// This component was built following guidelines/instructions from:
// https://medium.com/@tarik.nzl/angular-2-custom-form-control-with-validation-json-input-2b4cf9bc2d73
@Component({
  selector: 'aby-coordinate-input',
  templateUrl: './coordinate-input.component.html',
  styleUrls: ['./coordinate-input.component.scss'],
  providers: [{
    provide: NG_VALUE_ACCESSOR,
    useExisting: forwardRef(() => CoordinateInputComponent),
    multi: true,
  }, {
    provide: NG_VALIDATORS,
    useExisting: forwardRef(() => CoordinateInputComponent),
    multi: true,
  }],
})
export class CoordinateInputComponent implements ControlValueAccessor, Validator  {
  @Input()
  public type: GeographicPositionType;
  @Input()
  public placeholder: string = null;
  @Input()
  public debug: boolean = false;

  @ViewChild('box')
  private inputElem: ElementRef;
  protected _value: number;
  protected _valueHumanized: string;
  public lastEdition: string;
  private parseResult: DmsParseResult = null;

  // The method used to emit changes back to the form.
  private propagateUiValueChanged = (_: any) => { };
  private propagateTouched = (_: any) => { };
  private validatorInputsChanged = (_: any) => { };

  constructor(
    private renderer: Renderer2,
    private translator: TranslateService,
  ) {
  }

  triggerInputChange(newValue: string) {
    this.checkForChanges(newValue);
  }

  triggerLostFocus() {
    this.propagateTouched(true);
  }

  private checkForChanges(edit: string): void {
    if (this.lastEdition === edit) {
      return;
    }

    const result: DmsParseResult = dmsToDegrees(edit, this.type, this.translator);
    this.parseResult = result;
    if (result.success) {
      this._value = result.parsedValue;
      this._valueHumanized = edit;
    }
    this.lastEdition = edit;
    this.propagateUiValueChanged(this._value);
    this.validatorInputsChanged(this._value);
  }

  // --- BEGIN: Validator: https://angular.io/api/forms/Validator ---
  // "Method that performs synchronous validation against the provided control."
  validate(_control: AbstractControl): ValidationErrors {
    if (this.parseResult == null || this.parseResult.success) {
      return null;
    }
    return {
      parse: this.parseResult.errorMessage,
    };
  }
  // "Registers a callback function to call when the validator inputs change."
  registerOnValidatorChange?(fn: () => void): void {
    this.validatorInputsChanged = fn;
  }
  // --- END: Validator ---

  // --- BEGIN: ControlValueAccessor: https://angular.io/api/forms/ControlValueAccessor ---

  /**
   * Writes a new value to the element.
   * @param obj The new value for the element
   */
  writeValue(obj: any): void {
    const newUserValue = degreesToDMS(obj, this.type);
    this._value = obj;
    // Push value to UI; it will be received back in the callbacks.
    this.inputElem.nativeElement.value = newUserValue;
    this._valueHumanized = newUserValue;
    this.lastEdition = newUserValue;
    this.parseResult = null;
  }
  /**
   * Registers a callback function that is called when the control's value changes in the UI.
   * @param fn The callback function to register.
   */
  registerOnChange(fn: any): void {
    this.propagateUiValueChanged = fn;
  }
  /**
   * Registers a callback function that is called by the forms API on initialization to update the form model on blur.
   * @param fn The callback function to register.
   */
  registerOnTouched(fn: any): void {
    this.propagateTouched = fn;
  }
  /**
   * Function that is called by the forms API when the control status changes to or from 'DISABLED'.
   * Depending on the status, it enables or disables the appropriate DOM element.
   * @param isDisabled The disabled status to set on the element.
   */
  setDisabledState?(isDisabled: boolean): void {
    // Disable the UI.
    this.renderer.setProperty(this.inputElem.nativeElement, 'disabled', isDisabled);
  }
  // --- END: ControlValueAccessor ---
}

Supporting code

position.ts

export enum GeographicPositionType {
  Latitude = 'Latitude',
  Longitude = 'Longitude',
}

utils.ts

import { GeographicPositionType } from 'app/models/position';
import { TranslateService } from '@ngx-translate/core';

export const directionFromSignal = (decimalDegrees: number, positionType: GeographicPositionType): string => {
  switch (positionType) {
    case GeographicPositionType.Latitude:
      return decimalDegrees >= 0 ? 'N' : 'S';
    case GeographicPositionType.Longitude:
      return decimalDegrees >= 0 ? 'E' : 'W';
    default:
      throw new TypeError('Unknown geographic position type.');
  }
};

export const degreesToDMS = (decimalDegrees: number, positionType: GeographicPositionType): string => {
  if (decimalDegrees == null || Number.isNaN(decimalDegrees) || !Number.isFinite(decimalDegrees)) {
    return null;
  }

  const direction: string = directionFromSignal(decimalDegrees, positionType);

  // We need to make this absolute, or when rounding negative numbers,
  // it rounds down and all the math gets incorrect.
  const decimalDegreesAbs = Math.abs(decimalDegrees);

  let degrees = Math.floor(decimalDegreesAbs);
  const decimalMinutes = (decimalDegreesAbs - degrees) * 60;
  let minutes = Math.floor(decimalMinutes);
  let decimalSeconds = (decimalMinutes - minutes) * 60;

  // After rounding the seconds and the minutes might become 60,
  // so we need to account for that.

  // NOTE: we check against 59.9999 because toLocaleString rounds the value,
  // so 59.9999 turns into 60. But rounding before decimalSeconds would mean
  // any value above 59.5 would be considered 60, which we don't want.
  if (decimalSeconds >= 59.9999) {
    minutes++;
    decimalSeconds = 0;
  }
  if (minutes === 60) {
    degrees++;
    minutes = 0;
  }

  const seconds: string = decimalSeconds.toLocaleString(undefined, {
    useGrouping: false, // Specifies whether to use grouping separators.
    minimumFractionDigits: 4,
    maximumFractionDigits: 4,
  });

  // 1° 2' 3.4567"N
  return `${degrees}° ${minutes}' ${seconds}"${direction}`;
};

export interface DmsParseResult {
    success: boolean;
    parsedValue: number;
    errorMessage: string;
}

const parsedOk = (value: number): DmsParseResult => {
  return {
    success: true,
    errorMessage: null,
    parsedValue: value,
  };
};

const parseError = (translator: TranslateService, reason: string, interpolateParams?: Object): DmsParseResult => {
  const translated: string = translator.instant('coordinateConversions.' + reason, interpolateParams);
  return {
    success: false,
    errorMessage: translated,
    parsedValue: null,
  };
};

const dmsRegex: RegExp = /^(\d+)°(\d+)'(\d+\.?\d*)"(\w)$/;

export const normalizeDmsString = (input: string): string => {
  if (input == null) {
    return input;
  }
  let normalized = input;
  normalized = normalized.replace(/\s+/g, '');
  normalized = normalized.replace(/º/g, '°');
  normalized = normalized.replace(/´|‘|’|′/g, '\'');
  normalized = normalized.replace(/''|“|”|″/g, '"');
  return normalized;
};

const handleCleanMatch = (
  translator: TranslateService,
  degrees: number,
  minutes: number,
  decimalSeconds: number,
  signal: string,
  positionType: GeographicPositionType,
  ): DmsParseResult => {
  const value = degrees + minutes / 60 + decimalSeconds / 3600;

  switch (positionType) {
    case GeographicPositionType.Latitude:
      switch (signal) {
        case 'N':
        case 'n':
          return parsedOk(value);
        case 'S':
        case 's':
          return parsedOk(-value);
        default:
          return parseError(translator, 'Signal symbol "{{signal}}" should be N or S.', {signal: signal});
      }
    case GeographicPositionType.Longitude:
      switch (signal) {
        case 'E':
        case 'e':
          return parsedOk(value);
        case 'W':
        case 'w':
          return parsedOk(-value);
        default:
          return parseError(translator, 'Signal symbol "{{signal}}" should be E or W.', {signal: signal});
      }
    default:
      throw new TypeError('Unknown geographic position type.');
  }
};

export const dmsToDegrees = (humanReadable: string, positionType: GeographicPositionType, translator: TranslateService): DmsParseResult => {
  if (humanReadable == null) {
    return parsedOk(null);
  }

  const normalized = normalizeDmsString(humanReadable);
  if (normalized === '') {
    return parsedOk(null);
  }

  const match: RegExpMatchArray = normalized.match(dmsRegex);
  if (match !== null && match.length === 5) {
    const degrees = Number(match[1]);
    const minutes = Number(match[2]);
    const decimalSeconds = Number(match[3]);
    const signal = match[4];

    return handleCleanMatch(translator, degrees, minutes, decimalSeconds, signal, positionType);
  }

  const sampleSignal = positionType === GeographicPositionType.Latitude ? 'N' : 'W';
  return parseError(translator, 'Input must be in format: 12° 34\' 56.789"{{signal}}', {signal: sampleSignal});
};
\$\endgroup\$
3
\$\begingroup\$

A short review;

  • Changing CSS for input seems too broad, just apply CSS changes to a class tied to your control
  • throw new TypeError('Unknown geographic position type.'); seems odd, why throw this if the calling function does not catch it anyway? You need a think on how this control handles exceptions.
  • Similarly, in degreesToDMS you return null , but the calling functions don't deal with returned null values. You need a think about that as well.
  • Naming is well done
  • Commenting is well done
  • Design seems standard to me for a new control
\$\endgroup\$
  • 2
    \$\begingroup\$ Thank you for your time! :) In Angular, component CSS is scoped to the component itself; it never affects neither parent nor children HTML elements. The error is thrown for code-completion; it is never expected to happen, unless some idiot developer (me) forgets to set the type or sets it wrongly; and then an error will be visibly thrown in the console. The null in degreesToDms is passed directly to the UI; the input will have an empty string, which IMO is correct according to an empty or invalid latitude/longitude. \$\endgroup\$ – ANeves Oct 23 '19 at 13:12
2
+200
\$\begingroup\$

In additions to konijn's remarks I would add some other that I would normally add reviewing this code.

Explicit type declarations

In the following code there is no need to declare type:

public debug: boolean = false;

The TypeScritpt compiler will understand the type from the assigned value.

Methods with side effects

You have a method checkForChanges(edit: string). It is not obvious from the method name that it produces side effects (changes the component's state). I would suggest you to rename the method to make it clear. Maybe propagateChanges?

Fields/variables naming

It is probably a matter of taste, but I would suggest you to use names starting with the underscore sign (_value) as less as possible. The only valid case I know is when you want to have a getter with the same name, but it is not the case here.

Use of any declarations

Try to use any as less as possible. TypeScript encourages types and Angular framework is designed in the way that you need any only in case you interact with third party JS libs.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your time. :) I like my types explicit, as much as possible. About the any, I only know the specific type in write(); in the other places, I couldn't figure out what type would be correct (or even if I am meant to pick a type myself); do you have suggestions of idiomatic types, or how I could find out the expected types in e.g. the registerOnTouched(fn: any): void { } method. \$\endgroup\$ – ANeves Oct 23 '19 at 13:32
  • \$\begingroup\$ Sorry, I didn't invest much time going deep into the implementation, maybe there is no other way. As for registerOnTouched the documentation declares it as any. \$\endgroup\$ – Sasha Shpota Oct 23 '19 at 14:49
  • \$\begingroup\$ <shrug> Such is life. Thanks for the doc link. :) \$\endgroup\$ – ANeves Oct 23 '19 at 15:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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