Skip to main content
Code Review

Return to Answer

Spelling and grammar
Source Link
Toby Speight
Toby Speight
  • 88.3k
  • 14
  • 104
  • 327

First, congrats on writing a Rubik's Cube mode. That is a feat. There are, however, three parts I would love to see improved in a follow-up version:

  1. currentlyCurrently, none of the symbols are private; functions and variables that should not be used outside your module usually have two hyphens, e.g., rubic--paint-diamond. As far as I understood, almost all your functions and values should be considered private, the sole exception areother than the (interactive) commands.
  2. theThe current variant is not consistent in its use of documentation strings. I'd argue that all functions should be documented there.
  3. This is probably the most important remark: your code does not work. You're missing (require 'cl). But since Emacs 27, cl is deprecated. You should (require 'cl-lib) and prefix the CL functions with cl (e.g., replace loop with cl-loop).

Other than that, there are some typos, e.g., rubic-cuve-back or te. Those should probably get fixed.

From a usability point of view, it would be great if the key bindings were listed below the history. That would ease the usage. Also, you should probably not bind g to a reset, at least not without a yes-or-no prompt.

Hope that helps!

First, congrats on writing a Rubik's Cube mode. That is a feat. There are, however, three parts I would love to see improved in a follow-up version:

  1. currently, none of the symbols are private; functions and variables that should not be used outside your module usually have two hyphens, e.g., rubic--paint-diamond. As far as I understood, almost all your functions and values should be considered private, the sole exception are the (interactive) commands.
  2. the current variant is not consistent in its use of documentation strings. I'd argue that all functions should be documented there.
  3. This is probably the most important remark: your code does not work. You're missing (require 'cl). But since Emacs 27, cl is deprecated. You should (require 'cl-lib) and prefix the CL functions with cl (e.g., replace loop with cl-loop).

Other than that, there are some typos, e.g., rubic-cuve-back or te. Those should probably get fixed.

From a usability point of view, it would be great if the key bindings were listed below the history. That would ease the usage. Also, you should probably not bind g to a reset, at least not without a yes-or-no prompt.

Hope that helps!

First, congrats on writing a Rubik's Cube mode. That is a feat. There are, however, three parts I would love to see improved in a follow-up version:

  1. Currently, none of the symbols are private; functions and variables that should not be used outside your module usually have two hyphens, e.g., rubic--paint-diamond. As far as I understood, almost all your functions and values should be considered private, other than the (interactive) commands.
  2. The current variant is not consistent in its use of documentation strings. I'd argue that all functions should be documented there.
  3. This is probably the most important remark: your code does not work. You're missing (require 'cl). But since Emacs 27, cl is deprecated. You should (require 'cl-lib) and prefix the CL functions with cl (e.g., replace loop with cl-loop).

Other than that, there are some typos, e.g., rubic-cuve-back or te. Those should probably get fixed.

From a usability point of view, it would be great if the key bindings were listed below the history. That would ease the usage. Also, you should probably not bind g to a reset, at least not without a yes-or-no prompt.

Hope that helps!

Source Link
Zeta
Zeta
  • 19.7k
  • 2
  • 57
  • 90

First, congrats on writing a Rubik's Cube mode. That is a feat. There are, however, three parts I would love to see improved in a follow-up version:

  1. currently, none of the symbols are private; functions and variables that should not be used outside your module usually have two hyphens, e.g., rubic--paint-diamond. As far as I understood, almost all your functions and values should be considered private, the sole exception are the (interactive) commands.
  2. the current variant is not consistent in its use of documentation strings. I'd argue that all functions should be documented there.
  3. This is probably the most important remark: your code does not work. You're missing (require 'cl). But since Emacs 27, cl is deprecated. You should (require 'cl-lib) and prefix the CL functions with cl (e.g., replace loop with cl-loop).

Other than that, there are some typos, e.g., rubic-cuve-back or te. Those should probably get fixed.

From a usability point of view, it would be great if the key bindings were listed below the history. That would ease the usage. Also, you should probably not bind g to a reset, at least not without a yes-or-no prompt.

Hope that helps!