Ocena ISS tracker (aplikacja w trakcie rozwoju)

Serio, żadnych uwag co do kodu JS? Zauważcie, że w jednym miejscu robię trik (typowe w JS) tj. number x string = number

Zalążki złego kodu są:

1. zdarza ci się stylować po id

 #mapSection {
 ...
 #dataSection {

problem jest, że id może być jedno, a zwykle wraz z rozwojem aplikacji nawet jak wydawało nam się, że element będzie jeden, to się okazuje, że trzeba zrobić ileś takich samych rzeczy. Więc lepiej stylować po klasach. Kolejny problem, że zwiększasz niepotrzebnie specifity https://developer.mozilla.org/en-US/docs/Web/CSS/Specificity

1. nazewnictwo. Masz funkcję o dość generycznej nazwie parseJson(data) podczas gdy ona działa bardzo konkretnie parsując konkretne dane. I wbrew nazwie ogóle nie działa na JSON, co ciekawe. Czemu więc parseJson (lepszą nazwą byłoby np. parseIssPosition)?

2. nawet jak wejdziemy w implementację funkcji parseJson, to w sumie co ona tam robi? Dwie rzeczy:

   • deserializuje datę
   • kopiuje pozostałe właściwości obiektu

   function parseJson(data) {
    const currentIssPosition = {
        "altitude" : data.altitude,
        "latitude" : data.latitude,
        "longitude" : data.longitude,
        "timestamp" : new Date(data.timestamp * 1000), 
        "velocity" : data.velocity
    };
    return currentIssPosition;     
}

Pytanie, co chcesz zrobić. Jeśli chcesz przekopiować właściwości obiektu, to możesz zrobić to tak {...data} (wtedy kopiuje wszystkie właściwości obiektu) i wtedy ta funkcja skróciłaby się do:

function parseJson(data) {
    return {
        ...data,
        "timestamp" : new Date(data.timestamp * 1000), 
    };
}

(no chyba, że chcesz skopiować tylko wybrane właściwości, to wtedy faktycznie musiałbyś wymienić gdzieś w kodzie każdą właściwość po kolei)

1. masz długie linie w funkcji displayReadings. Do tego można podejść w sposób estetyczny (np. walnąć entery, albo zrobić destructuring, żeby wydobyć już na wstępie właściwości z obiektu currentIssPosition., żeby ciągle tego samego nie pisać), ale wydaje mi się, że najlepiej będzie podejść do tego pod kątem separation of concerns
   np. tu masz długą linijkę:

document.getElementById('longitude').textContent = "długość: " + currentIssPosition.longitude.toFixed(decimalPrecision) + longitudeDirection(currentIssPosition.longitude);

ta linijka zajmuje się 3 rzeczami naraz (aktualizacja DOM, formatowanie stringa, stringifikacja wartości geograficznych).

Zamiast ręcznie dodawać currentIssPosition.longitude.toFixed(decimalPrecision) + longitudeDirection(currentIssPosition.longitude);
albo: currentIssPosition.altitude.toFixed(decimalPrecision) + " km";
możesz zrobić osobne funkcje, które będą od razu to stringifikować i wtedy to się skróci do:

document.getElementById('longitude').textContent = "długość: " + stringifyLongitude(currentIssPosition.longitude)

I wtedy funkcję latitudeDirection można by zlikwidować i zastąpić funkcją stringifyLatitude

function stringifyLatitude(latitude) {
  return latitude.toFixed(decimalPrecision) + ((latitude >= 0) ? "N" : "S");
}

1. kwestia stylu:

"positionX" : map.getBoundingClientRect().x,

twój kod jest poprawny, tym niemniej nie ma potrzeby pisać cudzysłowów wokół kluczy w tym przypadku. Takie coś też by zadziałało:

positionX : map.getBoundingClientRect().x,

(co innego jakbyś miał spacje w kluczu)

1. czemu wywołujesz funkcję map.getBoundingClientRect() po kilka razy?
   Lepiej zrobić tak:

const bounds = map.getBoundingClientRect();

a później się odwoływać po np. bounds.x

• bardziej schludnie
• nie odwołujesz się do DOM co chwila (tutaj nie chcę mówić jakichś kategorycznych sądów, bo nie wiem dokładnie, co robi getBoundingClientRect pod spodem, ale co do zasady, to warto unikać wielokrotnego odwoływania się w JS do elementów DOM, bo może być to niewydajne)

Zalążki złego kodu są:

1. zdarza ci się stylować po id

Poprawione. Bo i tak to była wstępna wersja. Inna sprawa, że nie każda aplikacja jest dynamicznie rozwijana pod kątem dodawania nowych elementów. Rozumiem, że bardziej chodzi o "ideę" i wspomniane specifity.

1. nazewnictwo. Masz funkcję o dość generycznej nazwie parseJson(data) podczas gdy ona działa bardzo konkretnie parsując konkretne dane. I wbrew nazwie ogóle nie działa na JSON, co ciekawe. Czemu więc parseJson (lepszą nazwą byłoby np. parseIssPosition)?

Pytanie więc jakiego typu argument dostaje ta funkcja tutaj:

    const URL = 'https://api.wheretheiss.at/v1/satellites/25544';
    fetch (URL)
    .then((response) => response.json())
    .then((data) => displayIssPosition(data))
}

1. nawet jak wejdziemy w implementację funkcji parseJson, to w sumie co ona tam robi? Dwie rzeczy:
   • deserializuje datę
   • kopiuje pozostałe właściwości obiektu

Kopiuj

   function parseJson(data) {
    const currentIssPosition = {
        "altitude" : data.altitude,
        "latitude" : data.latitude,
        "longitude" : data.longitude,
        "timestamp" : new Date(data.timestamp * 1000), 
        "velocity" : data.velocity
    };
    return currentIssPosition;     
}

Pytanie, co chcesz zrobić. Jeśli chcesz przekopiować właściwości obiektu, to możesz zrobić to tak {...data} (wtedy kopiuje wszystkie właściwości obiektu) i wtedy ta funkcja skróciłaby się do:

Kopiuj

function parseJson(data) {
    return {
        ...data,
        "timestamp" : new Date(data.timestamp * 1000), 
    };
}

(no chyba, że chcesz skopiować tylko wybrane właściwości, to wtedy faktycznie musiałbyś wymienić gdzieś w kodzie każdą właściwość po kolei)

I tak jest, bo to dostaję z API:

    "name": "iss",
    "id": 25544,
    "latitude": 50.11496269845,
    "longitude": 118.07900427317,
    "altitude": 408.05526028199,
    "velocity": 27635.971970874,
    "visibility": "daylight",
    "footprint": 4446.1877699772,
    "timestamp": 1364069476,
    "daynum": 2456375.3411574,
    "solar_lat": 1.3327003598631,
    "solar_lon": 238.78610691196,
    "units": "kilometers"
}

Inna sprawa, że ewentualna rozbudowa nie rujnuje całej aplikacji jeżeli chodzi o logikę.

1. masz długie linie w funkcji displayReadings.

Poprawione. Co ciekawe, teraz łatwiej będzie dodać kolejny ficzer typu wyświetl jako mile zamiast km (przeliczenie na inne jednostki).

1. kwestia stylu:

Kopiuj

"positionX" : map.getBoundingClientRect().x,

twój kod jest poprawny, tym niemniej nie ma potrzeby pisać cudzysłowów wokół kluczy w tym przypadku. Takie coś też by zadziałało:
...

1. czemu wywołujesz funkcję map.getBoundingClientRect() po kilka razy?

To chyba z rozpędu zrobiłem. Poprawione.

@LukeJL, dzięki za uwagi, poprawki wdrożyłem.

Oprócz tego poprawiłem trochę interfejs użytkownika. Wszystko jest już (chyba) w miarę wyrównane i (chyba) responsywne. Zachęcam do przetestowania i zgłaszania dalszych uwag.

Sam zastanawiam się nad wydzieleniem funkcji drawOnMap() na scalledLatitude() i scalleLongitude() . Funkcję addISSmarker na razie traktuję jako eksperyment.

Co dalej?

1. Obsługa błędów i wyświetlanie komunikatów dla usera.
2. Przelicznik km <-> mile.
3. Dalsze eksperymenty z pokazywaniem trajektorii będącej w praktyce sinusoidą.

Tak w ogóle to czy wydzielanie tylu funkcji w kodzie JS nie zużyje z dużo zasobów przeglądarki?

Jak widać ładnie pokrywa się z siatką (15N, 0E):