From 766337dc6157b7262a98400f759cb75d40b24c99 Mon Sep 17 00:00:00 2001 From: Phil Darnowsky Date: Thu, 12 Dec 2019 04:41:42 -0500 Subject: [PATCH] Write peer review for P3 --- peer-review.md | 110 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 peer-review.md diff --git a/peer-review.md b/peer-review.md new file mode 100644 index 0000000..59a36e4 --- /dev/null +++ b/peer-review.md @@ -0,0 +1,110 @@ +## P3 Peer Review + ++ Reviewer's name: Phil Darnowsky ++ Reviwee's name: Jiajia Chen ++ URL to reviewee's Github repo: *https://github.com/audreychenjj-2019/e28* + +*Answer the following questions in regards to the student's project you are reviewing. Include context and code examples in your answers when relevant.* + + +### Are you able to produce any errors or unexpected results? +I was able to produce just one error. Go to the "Portfolio" page and click +"Add". In the symbol field, enter a string that's not the symbol of any stock, +for instance "FOOBAR". Hit Enter, and in the console, you get: + +``` +TypeError: Cannot read property 'symbol' of undefined + at onKeyDownEnter (AutoComplete.vue:123) +``` + +This could presumably be fixed with a quick check at the head of +`onKeyDownEnter`. + +### Were there any parts of the interface that you found confusing or unclear? +In the portfolio page, when you click "Add", it would be nice if the first +input field (in the "Symbol" column) got input focus. + +When adding or editing data in the Portfolio, I would have put a validation +on the numeric fields ("# Shares" and "$ Cost/Share") to ensure non-negative +values. + +### Are there aspects of the code that you feel were not self-evident and would benefit from comments? + +I discuss some things I found unclear or hard to follow below. However, my +preference, when it comes to code that isn't self-evident, is not to comment on +it, but rather to refactor the code so it becomes more self-evident. + +### Are there any parts of the code that you found interesting or taught you something new? + +I like how you encapsulated access to the `localStorage` in a class that hid the +implementation details behind a neat interface. Overall I found your division of +the project into components to be well thought out: they exhibit high coherence +and low coupling, which is what we want. + +### Are there any best practices discussed in course material that you feel were not addressed in the code? +Extracting some methods could clarify your templates a greate deal. For +instance, consider `TopQuotes.vue`, lines 25 through 48. Right now that section +reads: + +``` +
+

+ {{ stock.price }} +

+
+
+

+ {{ + ( + (100 * stock.changes) / + (stock.changes + parseFloat(stock.price)) + ).toFixed(2) + }}% +

+
+``` + +The markup gets buried, visually, by all the logic. But by extracting +appropriate methods this could read: + +``` +
+

+ {{ stock.price }} +

+
+
+

+ {{ changePercent(stock) }}% +

+
+``` + +The structure of the document is now much clearer. + +### Do you have any additional comments not covered in the above questions? +When naming event callbacks, name them for what they do, not what event they are +connected to. For example, in `AutoComplete.vue`, instead of giving the name +`onArrowDown` to the callback bound to `keydown.down`, perhaps call it +`increaseArrowCount`. This leads to more self-documenting code. + +You have much more inline CSS in your templates than I am comfortable with. + +As a general rule, where possible, use `const` instead of `let`. This does a +couple of important things. `const foo` indicates to the reader of your code +that `foo` should never change once set. It indicates the same thing to the JS +interpreter, meaning the interpreter will catch erroneous attempts to mutate +the value of `foo`. It also might serve as a hint to the interpreter that +there's an optimization opportunity by way of inlining the value of `foo`. + +Overall this is a nice piece of work. Well done!