Music player - Githubissues

tmptrash commented 4 years ago

в общем по коду: ты молодец, что довел это до рабочего состояния. в процессе мы дотюнаем твой код до нужного идеала :) в общем, мне кажется, что для этой задачи кода немного много. я бы сделал его на много короче. что именно, смотри в пунктах ниже. общая рекомендация - старайся минимизировать сайд эффекты. это когда ты вызываешь метод, а он меняет какие-то локальные переменные класса, которые как-то влияют на другие методы. это не очень хорошо. по этому, нужно сокращать количество свойств класса до минимума. в идеале, функии должны быть независимы от внешнего мира на столько, на сколько это возможно. это уменьшает их связанность (как раз с внешним миром).
вот этот коммент не отвечает действительности: Canvas implementation with minimum logic for drawing colored dots.. нужно описать что делает этот модуль и для чего он. как он устроен и какие у него особенности
используй одинарные ковычки. чтобы у всех был один стиль.

давай использовать один стиль для массивов:

this._radioStations = [
        {
            title: "Ambient Radio",
            source: "http://uk2.internet-radio.com:31491/;stream.mp3?_=1"
        },
...
]

заменить на:

this._radioStations = [{
    title: "Ambient Radio",
    source: "http://uk2.internet-radio.com:31491/;stream.mp3?_=1"
}, {
...
]

расчитывай на то, что конфиг прийдет извне через параметер конструктора options.stations. то есть, нужно сохранить this._stations = options.stations вместо _radioStations.
в методе destroy() нужно удалять элементы из DOM, иначе при удалении класса его инстанс. удалиться, а элементы в DOM останутся. присвоение свойствам класса null не удаляет их из DOM. обнулять свойства не имеет смысла. нужно удалять только динамические объекты (память, дом)
Мне кажется ты бы мог значительно упростить себе жизнь, если бы ты создал всю структуру одной строкой. примерно так:
```
el.innerHTML = `<audio>
    <source...>
</audio>`;
```
если тебе понадобятся ссылки на внутренние теги, то можно использовать el.querySelector('source'). а этом же месте будут и стили.
если создаешь this._player = new Player(); в World классе, то нужно его и удалять через this._player.destroy().
я бы вынес preload в localStorage настройку. а он действительно нам нужен? я бы убрал это свойство. может просто стартовать звук сразу, если в localStorage пусто, иначе брать ее оттуда и решать включать или нет. то есть, если пользователь последний раз нажал play, то нужно при следующем старте страницы включить то же самое радио
я бы убрал свойство _isNowPlaying. чтобы проверить что радио играет нужно юзать !audelem.paused вот и все :)
по факту _stationTitlePrefix - константа. давай вынесем ее за класс вот так:
```
const STATION_TITLE_PREFIX = 'Now playing: ';
```

class Player { ...


11. `_currentStation` и `_stationIndex` - это дубляж. тебе хватит хранить только индекс. чтобы получить доступ к текущей станции просто используй в каждом методе, где нужно: `const station = this._stations[this._stationIdx]`.
12. я стараюсь использовать относительно читабельные но коротние имена. давай заменим:
`_player` -> `_playerEl`, `_playButton` -> `_playBtn`, `_nextButton` -> `_nextBtn`, `_stationTitleElement` -> `_titleEl`
13. мне кажется, что метод `_getDefaultRadio()` тебе вообще не нужен. в конструкторе нужно прописать: `this._stationIdx = localStorage.irmaStation || 0`. кстати, давай использовать префикс `irma` в ключах `localStorage`
14. не используй `el.onclick = this._onPlayButton.bind(this);` onclick. используй `addEventListener()` в паре с removeEventListener()
15. по поводу локации. я бы экономил место и сдвинул эти кнопки в правый верхний угол. надпись бы вынес в `tooltip` при наведении на кнопку. это бы секономило кучу места ;) тогда и метод `_updateTitle()` исчезнет. 
16. нашел багу: 
![image](https://user-images.githubusercontent.com/1142545/69878823-9de7d280-12ce-11ea-80a7-acf8f733838c.png). отправляет [сюда](https://developers.google.com/web/updates/2017/06/play-request-was-interrupted): 
17. метод `_pickStation()` можно заменить записью `const station = this._stations[++this._stationIdx] || this._stations[this._stationIdx = 0];`

tmptrash commented 4 years ago

давай упростим значение константы LOCAL_STORAGE_KEY до irma-index
localStorage.getItem(LOCAL_STORAGE_KEY) можно урезать до localStorage[LOCAL_STORAGE_KEY]
что на счет 6, 8, 17 пунктов? ты их пропустил или там что-то не получилось?
смотри какие красивые юникод символы плей/паузы (https://en.m.wikipedia.org/wiki/Media_control_symbols)
а нельзя заменить audio.setAttribute('src',...) на audio.src = ...? И то же с другими свойствами.
помнишь о правиле об именовании функций и о том, что она должна делать что-то одно? _createAudioElement() делает несколько вещей (создает и добавляет в дом), хотя название намекает только на создание.

в этом коде можно убрать фигурные скобки

audio.addEventListener('canplay', () => {
        audio.play();
    }, false);

присмотритесь, в моих кнопках есть чёрный бордюр
есть внегласное правило не использовать операторы сравнения == и !=. Вместо них нужно использовать === и !==. Это связано с проверкой типов. Второй вариант проверяет тип. Первый - нет.

tmptrash commented 4 years ago

Думаю мы уже в одном шаге от результата. Осталась только косметика.

Помнишь, я говорил, что желательно использовать короткие имена? Метод _createHtmlElements() можно переименовать в _createDOM() или _createHtml().
В методе destroy() переменную _stationIndex можно не трогать. Она все равно будет занимать столько же памяти, как и до присвоения null. _stations - нужно, потому что ее GC удалит (так как она указывает на объект)
У меня вопрос. а какая логика за названием LOCAL_STORAGE_KEY, 'irma-index'? Про какой индекс идет речь? Если это радиостанция, то может назовем irma-station? Представь, что это будет читать кто-то левый. Он поймет что такое индекс? Вообще, оригинальная идея была в том, чтобы в этой константе хранить префикс irma. А все, что мы будем хранить в localStorage будет с этим префиксом. Например: localStorage[LS_KEY + 'station']. Изначально я думал, что ключей будет несколько. Если он один тогда давай его станцией назовем и все...
В методе destroy() переменная parentNode уже не нужна. Можно сразу подставлять document.body.removeChild(...)
В методе _createDOM() переменные player и el - это одно и то же. Хватит только одной - player. Помнишь мы говорили, что если переменная содержит ссылку на ветку DOM то к ней добавляется приставка El (playerEl).
Практически все вызовы метода setAttribute('attr', val) можно упростить на el.attr = val.
Ты правильно понял про innerHTML, но не так понял со стилями. Стили нужно положить в тег <style></style> тогда все будет выглядеть на много лучше. И сами функциональные теги на будут захламляться стилями.
Строки 'play', 'next',...'preload' и т.д. я бы вынес в константы вверху модуля. Это явный дубляж.
Ты смотрел на юникод символы? Они не подошли или не работают?
В методе _onPlayButton() когда устанавливаешь паузу или плей символ лучше использовать innerText, потому что при вызове innerHTML включается страшный и ужасный парсер под капотом, который в этой строке пытается найти html теги, а их там точно нет и мы это знаем.
А зачем в методе _onPlayButton() в условном операторе пробелы между строками? Что мы там разделяем? Если убрать setAttribute(), то они не будут нужны, так как везде будет присвоение.
метод _pickStation() можно заменить записью const station = this._stations[++this._stationIdx] || this._stations[this._stationIdx = 0];

tmptrash / irma

Music player #68