-
Notifications
You must be signed in to change notification settings - Fork 0
Exercise 2: Reviewing a review #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| dependencies { | ||
| implementation("com.google.code.gson:gson:2.13.2") | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| public class App { | ||
| public static void main(String[] args) { | ||
| new Presenter(new LocalDatabaseModel(), new ConsoleView()).run(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import java.util.Scanner; | ||
|
|
||
| public final class ConsoleView implements View { | ||
| private final Scanner scanner = new Scanner(System.in); | ||
|
|
||
| public void show(String text) { | ||
| // Prints `text` on the console | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not very useful? |
||
| System.out.println(text); | ||
| } | ||
|
|
||
| public void run(Presenter presenter, String prompt) { | ||
| while (true) { | ||
| System.out.print(prompt); | ||
| String command = scanner.nextLine(); | ||
| presenter.onInput(command); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import java.io.*; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO wildcard imports are bad |
||
| import java.util.*; | ||
| import java.util.stream.*; | ||
| import com.google.gson.*; | ||
|
|
||
| public final class LocalDatabaseModel implements Model { | ||
| private final JsonObject root; | ||
|
|
||
| public LocalDatabaseModel() { | ||
| try(var reader = new InputStreamReader(Model.class.getResourceAsStream("/dsonames.json"))) { | ||
| root = JsonParser.parseReader(reader).getAsJsonObject(); | ||
| } catch (IOException e) { | ||
| throw new IOError(e); | ||
| } | ||
| } | ||
|
|
||
| public List<String> names() { | ||
| var result = new ArrayList<String>(); | ||
| for (var entry : root.entrySet()) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
| var planet = entry.getValue().getAsJsonObject(); | ||
| result.add(planet.get("name").getAsString()); | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| public Map<String, String> translatedNames(String language) { | ||
| return root.entrySet().stream() | ||
| .map(e -> e.getValue().getAsJsonObject()) | ||
| .filter(p -> p.has(language)) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do some entries not have a translation? Should we document this? |
||
| .collect(Collectors.toMap(p -> p.get("name").getAsString(), p -> p.get(language).getAsString(), (a, b) -> a)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| import java.util.*; | ||
|
|
||
| public interface Model { | ||
| List<String> names(); | ||
| Map<String, String> translatedNames(String language); | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,41 @@ | ||||||
| import java.util.*; | ||||||
| import java.util.stream.*; | ||||||
|
|
||||||
| public final class Presenter { | ||||||
| private final Model model; | ||||||
| private final View view; | ||||||
|
|
||||||
| public Presenter(Model model, View view) { | ||||||
| this.model = model; | ||||||
| this.view = view; | ||||||
| } | ||||||
|
|
||||||
| public void onInput(String command) { | ||||||
| if (command.equals("list")) { | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should support more input languages than English |
||||||
| showList(model.names()); | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| var parts = command.split(" "); | ||||||
| if (parts[0].equals("searcch")) { | ||||||
| showList(model.names().stream().filter(n -> n.contains(parts[0])).toList()); | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return; | ||||||
| } | ||||||
| if (parts[0].equals("translate")) { | ||||||
| var names = model.translatedNames(parts[1]).entrySet(); | ||||||
| showList(names.stream().map(e -> e.getKey() + ": " + e.getValue()).toList()); | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| view.show("Unknown command"); | ||||||
| } | ||||||
|
|
||||||
| public void run() { | ||||||
| view.show("Hi! This app gives you info on star groups."); | ||||||
| view.run(this, "Type 'list', 'search <name>', or 'translate <language>' (or Ctrl+C): "); | ||||||
| } | ||||||
|
|
||||||
| private void showList(List<String> list) { | ||||||
| view.show(list.stream().map(s -> "- " + s).collect(Collectors.joining("\n"))); | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I like bullet points more |
||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| public interface View { | ||
| void show(String text); | ||
| void run(Presenter presenter, String prompt); | ||
| } |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| import java.util.*; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.*; | ||
|
|
||
| final class ExampleTest { | ||
| @Test | ||
| void namesAreLoaded() { | ||
| var model = new LocalDatabaseModel(); | ||
| var names = model.names(); | ||
| assertThat(names, hasItems("Sextans Dwarf Spheroidal Galaxy")); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test the order of names? |
||
| } | ||
|
|
||
| @Test | ||
| void translatedNamesAreLoaded() { | ||
| var model = new LocalDatabaseModel(); | ||
| var names = model.translatedNames("fr"); | ||
| assertThat(names, hasEntry(is("Sextans Dwarf Spheroidal Galaxy"), is("Galaxie naine sphéroïdale du Sextant"))); | ||
| } | ||
|
|
||
| @Test | ||
| void presenterListTest() { | ||
| var view = new FakeView(); | ||
| view.inputs.add("list"); | ||
| var pres = new Presenter(new FakeModel(), view); | ||
| assertThat(view.outputs, empty()); | ||
| pres.run(); | ||
| assertThat(view.outputs, contains("Hi! This app gives you info on star groups.", "- X\n- Y\n- Z")); | ||
| } | ||
|
|
||
| @Test | ||
| void presenterTranslateTest() { | ||
| var view = new FakeView(); | ||
| view.inputs.add("translate fr"); | ||
| var pres = new Presenter(new FakeModel(), view); | ||
| assertThat(view.outputs.isEmpty(), is(true)); | ||
| pres.run(); | ||
| assertThat(view.outputs, contains("Hi! This app gives you info on star groups.", "- X: A\n- Y: fr\n- Z: C")); | ||
| } | ||
|
|
||
| private static final class FakeModel implements Model { | ||
| @Override | ||
| public List<String> names() { | ||
| return List.of("X", "Y", "Z"); | ||
| } | ||
|
|
||
| @Override | ||
| public Map<String, String> translatedNames(String language) { | ||
| var result = new LinkedHashMap<String, String>(); | ||
| result.put("X", "A"); | ||
| result.put("Y", language); | ||
| result.put("Z", "C"); | ||
| return result; | ||
| } | ||
| } | ||
|
|
||
| private static final class FakeView implements View { | ||
| public List<String> inputs = new ArrayList<>(); | ||
| public List<String> outputs = new ArrayList<>(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Public fields are bad |
||
|
|
||
| @Override | ||
| public void show(String text) { | ||
| outputs.add(text); | ||
| } | ||
|
|
||
| @Override | ||
| public void run(Presenter presenter, String prompt) { | ||
| if (!inputs.isEmpty()) { | ||
| presenter.onInput(inputs.get(0)); | ||
| inputs.remove(0); | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be at least some basic Javadoc on public all classes/methods