Elisp #574
Elisp #574
Conversation
- Initial test modified to be similar to Clojure version - update-quality modified to read from make-item directly. Helper methods removed
|
First review was in #572 . Work in progress |
elisp/gilded-rose.el
Outdated
| (list :name name :sell-in sell-in :quality quality)) | ||
|
|
||
| (defun update-quality (item) | ||
| (let* ((quality (plist-get item :quality)) |
There was a problem hiding this comment.
the 3 lets are already a simplification/improvement of the original code. Is it possible to access the value from the struct each time? replace quality with (plist-get item :quality) everywhere?
elisp/gilded-rose.el
Outdated
| "Create an item with NAME, SELL-IN, and QUALITY." | ||
| (list :name name :sell-in sell-in :quality quality)) | ||
|
|
||
| (defun update-quality (item) |
There was a problem hiding this comment.
update quality takes a list of items at least and iterates them.
elisp/gilded-rose.el
Outdated
| (let* ((quality (plist-get item :quality)) | ||
| (sell-in (plist-get item :sell-in)) | ||
| (name (plist-get item :name))) | ||
| (cond |
There was a problem hiding this comment.
This is a solution, not the original problem. The code is already a nice switch statement. The original has deeply nested IFs. All languages should be as similar as possible to avoid confusion for participants. Please see https://github.com/emilybache/GildedRose-Refactoring-Kata/blob/main/clojure/src/gilded/core.clj
| - Test can be run interactively using the command ~ert~ or with ~(ert-run-tests-interactively t)~ | ||
|
|
||
| * Todo | ||
| - Add TestText fixture |
There was a problem hiding this comment.
Yes a TextTestFixture main method would be nice but is not strictly necessary.
- removed the cond statement and replaced with nested ifs - removed the let statement. Data is now accesses directly - update-quality now takes a list of items
|
Thank you. Code has been amended. Please see latest commit |
|
Thank you we are moving forward. There is still one thing. Your Gilded Rose update-quality code. Let me try to elaborate. The original code, e.g. https://github.com/emilybache/GildedRose-Refactoring-Kata/blob/main/clojure/src/gilded/core.clj is bad on purpose. We need a port that looks very similar. Your code is something like that Also you use min/max. The original code is different: And inside it has duplication for the checks >= 0 and <= 50. Further the code needs to check for the exact strings "Backstage passes to a TAFKAL80ETC concert" and "Sulfuras, Hand of Ragnaros" |
|
Thank you. I appreciate the support and guidance |
- logic of if statments changed to if not - removed min/max - Strings for Sulfuras and Backstage pass made to exactly match specification - Code structure is based more on the clojure version
|
Changes have been made. This has really pushed me to learn elisp better. Please let me know if anything else is needed |
elisp/gilded-rose.el
Outdated
|
|
||
| (defun update-quality (items) | ||
| (dolist (item items) | ||
| (if (and (not (string= (plist-get (car item) :name) "Aged Brie")) |
There was a problem hiding this comment.
I do not now elisp. Is (car item) necessary when you already do (dolist (item items)) so item is an item which we access with plist-get, right?
There was a problem hiding this comment.
That's a good point. I thought car was needed to get the name as the first element of an item. I'll remove it and run my tests against the code
There was a problem hiding this comment.
@codecop you were right. The car is not needed
elisp/gilded-rose.el
Outdated
|
|
||
| (when (> (plist-get (car item) :quality) 0) | ||
| (when (not (string= (plist-get (car item) :name) "Sulfuras, Hand of Ragnaros")) | ||
| (setf (plist-get (car item) :quality) (1- (plist-get (car item) :quality))))) |
There was a problem hiding this comment.
Is (1- .) a function which decrements by one? Has to be
There was a problem hiding this comment.
Yes it is:
1- is a built-in function in ‘C source code’.
(1- NUMBER)
Return NUMBER minus one. NUMBER may be a number or a marker.
Markers are converted to integers.
Other relevant functions are documented in the number group.
This function does not change global state, including the match
data.
This function has a ‘byte-compile’ property ‘byte-compile-one-arg’.
See the manual for details.
Removed unnecessary car from update-quality
|
Thank you and well done seeing it done. |
Thank you for your help |
READ ME BEFORE SUBMITTING A PR
Please do not submit a PR with your solution to the Gilded Rose Kata. This repo is intended to be used as a starting point for the kata.
Please provide your PR description below this line
Amended elisp code. Please see latest commit for more details