Siema.
Ostatnio z mojego kursu na udemy nauczyłem się obsługiwać pliki JSON, co uznałem za ciekawe rozwiazanie. W ramach ćwiczeń napisałem bardzo prosty program, który z pliku pobiera dane na temat Billboard hot 100 i wypisuje podana liczbę piosenek zgodnie z miejscami w rankingu. Czy jest w nim coś, co można poprawić?
Tutaj link: https://github.com/MaciekSm19/MusicChart
Z góry dziękuję za odpowiedź!
Java for (int i = 0; i < data.length(); i++) {
String title = data.getJSONObject(i).getString("song");
String artist = data.getJSONObject(i).getString("artist");
Integer thisWeek = data.getJSONObject(i).getInt("this_week");
Integer lastWeek;
if (data.getJSONObject(i).get("last_week").equals(null)) lastWeek = null;
else lastWeek = data.getJSONObject(i).getInt("last_week");
Integer peakPosition = data.getJSONObject(i).getInt("peak_position");
Integer weeksOnChart = data.getJSONObject(i).getInt("weeks_on_chart");
songs.add(new Song(title, artist, thisWeek, lastWeek, peakPosition, weeksOnChart));
}
Raczej tak się nie robi. Twoje podejście jest w pełni mechaniczne co nie jest jakąś wielką wadą, ale większość woli pół-automatyczne rozwiązania, gdzie mapowanie klucz -> pole w strukturze dzieje się automatycznie + jakieś andnotacje. Poczytaj jak działa jackson albo gson
Maciek_SK8 napisał(a):
Czy jest w nim coś, co można poprawić?
Tak - JSON jak sama nazwa wskazuje to powstał na potrzeby JSa a nie Javy.
W Javie używaj modeli Javowych a nie JSONa.
Jackson też jest spoko:
https://www.baeldung.com/jackson-object-mapper-tutorial
Wady projektowe:
DatabaseOperations wygląda jakby jej głównym zadaniem było wczytanie JSON'a z zewnętrznego serwisu - jeśli tak, to dobrze żeby zwróciła String. Chyba że powinna tez parsować treść, to powinna zwrócić Song. Zwrócenie JSONObject to najgorsze wyjście.DatabaseOperations jest używane w taki sposób databaseOperations.getJSONObject(databaseOperations.connect()). Co nie ma specjalnego sensu. Czemu nie po prostu String json = databaseOperations.fetchJson()?getTopChartedSongs() w ogóle nie pasuje do klasy ChartOperations. Należałoby ją z tamtąd zabrać i umieścić w innym miejscu.ChartOperations wygląda jakby jego głównym zadaniem było parsowanie jsona i zbudowanie Song. Ja zmieniłbym nazwę tej klasy na SongFactory albo coś w tym stylu.Ja bym to napisał jakoś tak:
public class Main {
public static void main(String[] args) {
SongsResponse allSongs = new SongsProvider().fetch();
System.out.println("The chart was last updated on " + allSongs.publishDate());
System.out.print("How many top charted songs would you like to see? ");
int amountOfSongs = new Scanner(System.in).nextInt();
response.songs()
.stream()
.limit(amountOfSongs)
.map(Main::formatSongListItem)
.forEach(System.out::println);
}
private static String formatSongListItem(Song song) {
return song.thisWeek() + ". " + song.title() + " - " + song.artist();
}
}
record SongsResponse(
String publishDate,
List<Song> songs) {}
record Song(
String title,
String artist,
Integer thisWeek,
Integer lastWeek,
Integer peakPosition,
Integer weeksOnChart) {}
class SongsProvider {
private final HttpClient client;
public SongsProvider() {
this.client = new HttpClient("https://raw.githubusercontent.com/mhollingshead/billboard-hot-100/main/recent.json");
}
public SongsResponse fetch() {
return createResponse(new JSONObject(this.client.fetchJson()));
}
private SongsResponse createResponse(JSONObject jsonObject) {
return new SongsResponse(
jsonObject.getString("date"),
parseSongs(jsonObject.getJSONArray("data")));
}
private List<Song> parseSongs(JSONArray jsonSongs) {
List<Song> songs = new ArrayList<>();
for (int i = 0; i < jsonSongs.length(); i++) {
songs.add(createSong(jsonSongs.getJSONObject(i)));
}
return songs;
}
private Song createSong(JSONObject song) {
return new Song(
song.getString("song"),
song.getString("artist"),
song.getInt("this_week"),
getIntOrNull(song, "last_week"),
song.getInt("peak_position"),
song.getInt("weeks_on_chart"));
}
private Integer getIntOrNull(JSONObject object, String fieldName) {
if (object.isNull(fieldName)) {
return null;
}
return object.getInt(fieldName);
}
}
public class HttpClient {
private final String url;
public HttpClient(String url) {
this.url = url;
}
String fetchJson() {
try {
return readInputStream(connect().getInputStream());
} catch (IOException e) {
throw new RuntimeException(e);
}
}
private static String readInputStream(InputStream inputStream) throws IOException {
BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream));
String line;
StringBuilder stringBuilder = new StringBuilder();
while ((line = reader.readLine()) != null) stringBuilder.append(line);
reader.close();
return stringBuilder.toString();
}
private HttpURLConnection connect() {
try {
URI uri = new URI(this.url);
HttpURLConnection connection = (HttpURLConnection) uri.toURL().openConnection();
if (connection.getResponseCode() != 200) {
throw new RuntimeException();
}
return connection;
} catch (IOException | URISyntaxException exception) {
throw new RuntimeException(exception);
}
}
}
Poprawiłem ten program poprzez użycie Jacksona do importu plików i znacznie zredukowało to ilość linijek kodu 
Kod, który napisałem podzieliłem na klasy i metody. Czy taki podział jest odpowiedni?
Jeszcze raz wrzucam link: https://github.com/MaciekSm19/MusicChart
Maciek_SK8 napisał(a):
Poprawiłem ten program poprzez użycie Jacksona do importu plików i znacznie zredukowało to ilość linijek kodu
Tylko weź pod uwagę co to znaczy. Owszem, linijek jest mniej, ale to dlatego że niejawnie nazwy pól i struktura formatu wejściowego musi być zbieżna z modelem Song. To się fachowo nazywa ciasne przywiązanie (tight-coupling), i ogólnie sprawia że aplikacja jest cięższa do zmiany. Nie jest to oczywiście jakaś tragedia, ale mimo wszystko pod względem elastyczności i jakości kodu to raczej krok wstecz. Prosty refaktor klasy Song może zepsuć działanie aplikacji, a zmiana formatu json'a w API które korzystasz wymusi albo przywrócenie poprzedniego kodu z czytaniem pól albo zmianę pól w klasie Song. Wiadomo, że mniej linijek wygląda czadersko, ale to raczej nie jest dobry kod.
Maciek_SK8 napisał(a):
Kod, który napisałem podzieliłem na klasy i metody. Czy taki podział jest odpowiedni?
Jeszcze raz wrzucam link: https://github.com/MaciekSm19/MusicChart
Klasa JsonDataImporter przyjmuje na wejściu adres URL a na wyjściu wystawia datę publikacji i listę piosenek. Nic w jej interfejsie nie mówi o JSON - JSON to szczegół implementacyjny. Skoro tak to w jej nazwie nie powinno być słowa JSON. Należałoby ją nazwać DataImporter, albo po prostu SongsImporter.
Wyjątek JsonProcessingException to szczegół implementacyjny, nie powinien być częścią sygnatury metody. Należałoby go złapać i rzucić runtime wyjątek catch (JsonProcessingException) { throw new RuntimeException(); }.
Klasa SongResponseFactory zasadzie nic nie robi - możnaby ją spokojnie usunąć.
Niepoprawne wczytanie danych widać że nie jest poprawnie obsłużone, bo nic nie robisz z wyjątkiem tylko System.out.prinln() - to nie jest dobry pomysł, bo zauważ ze jeśli poleci wyjątek, to jsonRoot pozostanie null, i obiekt będzie w niepoprawnym stanie. Późniejsze zawołanie getPublishDate() najpewniej poskutkuje NullPointerException, właśnie dlatego że konstruktor pozostawił obiekt w niepoprawnie zbudowanym stanie. Lepszym wyjściem byłoby po prostu rzucić wyjątek.
Zastanów się - jak program powinien się zachować, jeśli uruchomisz go nie mając połączenia z internetem? 
Nie powinien się wywalić, powinien sprezentować odpowiedni wynik w interfejsie użytkownika, np. napisać tekst "Nie udało się wczytać listy piosenek".
Tylko weź pod uwagę co to znaczy. Owszem, linijek jest mniej, ale to dlatego że niejawnie nazwy pól i struktura formatu wejściowego musi być zbieżna z modelem
Song. To się fachowo nazywa ciasne przywiązanie (tight-coupling), i ogólnie sprawia że aplikacja jest cięższa do zmiany. Nie jest to oczywiście jakaś tragedia, ale mimo wszystko pod względem elastyczności i jakości kodu to raczej krok wstecz. Prosty refaktor klasySongmoże zepsuć działanie aplikacji, a zmiana formatu json'a w API które korzystasz wymusi albo przywrócenie poprzedniego kodu z czytaniem pól albo zmianę pól w klasieSong. Wiadomo, że mniej linijek wygląda czadersko, ale to raczej nie jest dobry kod.
To znaczy Song jest rekordem, który bazuje na nazwach z pliku JSON. Rekord ten mógłby być zmieniany według mnie tylko w okoliczności, kiedy nazwy pól w JSON się zmienią.