Skip to content
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

Possible improvement to method naming convention #199

Open
krisgesling opened this issue Jun 9, 2021 · 4 comments
Open

Possible improvement to method naming convention #199

krisgesling opened this issue Jun 9, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@krisgesling
Copy link
Contributor

I'm sure I've had this conversation but couldn't find where - so if I'm duplicating please point it out and close this.

I've been thinking about the method names we use in formatting and wondering if we could make them more consistent and descriptive. We currently have two forms:

  1. pronounce_thing()
    I like this because it tells you what it's going to return - a human pronounceable string.
    You could argue that it is not 100% correct because the method doesn't actually pronounce the thing, it returns a string that is pronounceable.

  2. nice_thing()
    This one is a little muddier as it's used for getting either a displayable or a pronounceable string. Some methods handle both cases, others only one.

I'm wondering if it would be better to have everything named to mean either "give me a displayable version of thing" OR "give me a pronounceable version of thing". What these are I don't know. The first option that comes to mind is:

  1. format_thing_for_display()
  2. format_thing_for_speech()

Eg:

  1. format_time_for_display()
  2. format_time_for_speech()

Pro's

  • more consistent and descriptive
  • splitting the formatting of display and speech should reduce the complexity of individual methods

Con's

  • longer names (at least with my first attempt)
  • change inevitably includes some pain

This would be a big task to achieve, so I'm definitely not saying that this is what will happen, it's just come back to mind as I've been looking at a few LF PR's today and I'm interested in hearing other peoples thoughts on it.

It would clearly be a breaking change, and would split some methods eg nice_time() but we can retain all methods for some period of time while it's being deprecated, and nice_time() could call each of the new functions depending on the value of its speech argument.

@krisgesling krisgesling added the enhancement New feature or request label Jun 9, 2021
@chrisveilleux
Copy link
Member

I agree that the current naming conventions are less than ideal. In particular, nice_thing has always been on my list of things to change. As you alluded to, I would like to see some of these functions split up because they do multiple things. This will eliminate the overly long argument list passed to many of these functions. It could even help with naming.

There is more splitting we can do than just display/speech. Each of the arguments in each of these functions should be evaluated to determine if it alters the functionality. The "ordinals" argument in extract_number() comes to mind. Passing more than two or three arguments to a method or function is a code smell. It usually indicates the function/method is doing too much.

Also, a longer name is not a "con" (unless you use an editor without autocomplete 😉 ).

As for change including some pain... no pain, no gain 😄

@ChanceNCounter
Copy link
Contributor

Passing more than two or three arguments to a method or function is a code smell. It usually indicates the function/method is doing too much.

I agree, but we've run into situations a few times where accessibility for new coders became an issue. Also, many of these functions, if split, would consist of a bunch of duplicate code with relatively minor differences.

However, keeping them together has resulted in these ungodly signatures. It's less awful for the user than it is for us, because they rarely want more than one or two of those parameters at a time, but they're still oof and I couldn't tell you how many discussions have contained, "but so many function arguments..."

The easiest solution for users would probably be to add not-a-wrappers for more specific invocations. Have extract_ordinal(n, lang) call extract_number(n, lang, ordinals=True), for instance.

Identifying which functions to split and which to wrap will probably be the most annoying part, but 🤷‍♂️ no pain no gain indeed.

@chrisveilleux
Copy link
Member

I agree, but we've run into situations a few times where accessibility for new coders became an issue. Also, many of these functions, if split, would consist of a bunch of duplicate code with relatively minor differences.

Can you elaborate on what the accessibility issues are? I would think that more well-named and clearly defined functions would be better for new coders than overloaded poorly named functions.

@ChanceNCounter
Copy link
Contributor

"extract_number() does what it sounds like, and can be instructed to do some very useful things," is easier to explain to a brand new coder than, "Here is a list of functions that can extract numbers. Which you'll choose depends on the context."

Again, not great, just a factor. The amount of duplicated code vs. not will be the more important factor. Either way, wrapping that stuff is the worst case (or not, that's subjective =P)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants