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

List of weak/unclear variable names to improve (some may need discussion) #923

Open
4 tasks
knod opened this issue Oct 27, 2018 · 9 comments
Open
4 tasks

Comments

@knod
Copy link
Collaborator

knod commented Oct 27, 2018

Include the variable name and the file path in which it appears. You can also add notes explaining yourself. Feel free to add them below or in new comments! Please don't get rid of or change something that someone else put down, though you can certainly add to the conversation surrounding it.

  • 1. activeID GraphTimeButtons.js
  • 2. client Everywhere. There are two clients. One for a client's account and one for the client data. Even without that, I'm not sure client is a great name for that data as it grows. The latter is complicated by the fact that it has the current and future props. Options:
    1. account or accountData for... account data.
    2. answers for form data. Then it can be all form data, not just client data. E.g. whether the client wants to fill in extra expense amounts. Is that something we want? current and future don't really fit into that model.
    3. responses (similar, though with problems)
    4. benefitData or dataForBenefits 😕
    5. household for client data, since it really is the household we're collecting info about. This may change if we start to calculate individual benefits, such as with MassHealth. We'd have to talk to SMEs about that.
  • 3. predictions Predictions.js and it's associated stuff. Not sure about this. We may need to keep it as it is since it indicates the future. It's partly a 'plain language' thing. Maybe report would work, but it's not as specific.
  • 4. timeClient or whatever it is. I have to go. I'll find it later.

This may not be the best format for doing this. What would be better? Something with a reddit-like commenting system? We can discuss.

@knod knod changed the title List of weak/unclear variable names to improve (may need discussion) List of weak/unclear variable names to improve (some may need discussion) Oct 27, 2018
@knod
Copy link
Collaborator Author

knod commented Oct 28, 2018

  • 5. income - often actually should be earnings

or earned

@dylanesque
Copy link
Collaborator

dylanesque commented Oct 31, 2018

I combed through the files in the utils folder today. Most of it looks good or was touched upon in Michelle's comments above. One thing did stick out to me, though:

[] clientPartial from getSideEffects.js is pretty confusing.

@knod
Copy link
Collaborator Author

knod commented Oct 31, 2018

Great find! That's actually another attempt at timeClient or whatever it is.

@knod
Copy link
Collaborator Author

knod commented Nov 2, 2018

This looks like a duplicate, but it's not quite:

  • 7. client

In my opinion client is very confusing. It sounds like it would have earned, hasSNAP, etc. straight in it, but it's actually current data and future data and those have that info in them. Can we find a better way to express these? Should it be split into separate variables?

Also, see #952.

@knod
Copy link
Collaborator Author

knod commented Nov 2, 2018

  • 8. activeBenefits (and benefits in some places) - They include income, so a better name might be resources

@knod
Copy link
Collaborator Author

knod commented Nov 2, 2018

  • 9. income is sometimes used when we mean earned income or pay

knod added a commit to knod/cliff-effects-1 that referenced this issue Nov 2, 2018
knod added a commit that referenced this issue Nov 3, 2018
Fixes #9 of #923, switch `income` to `earned` where needed
@knod
Copy link
Collaborator Author

knod commented Nov 6, 2018

From #968 (#968 (review)):

  • .interval to idSuffix or something

@knod
Copy link
Collaborator Author

knod commented Nov 15, 2018

  • toFancyMoneyStr really isn't descriptive. It turns a number into a currency string, though right now it only makes US dollars. Not sure how to dance with that one.

@knod
Copy link
Collaborator Author

knod commented Nov 15, 2018

  • toMoneyStr also isn't descriptive. I'm not sure what else to call it. It takes a number, adds to decimal points if needed, otherwise rounds a number to the first two decimal points, then returns a string of that. It's basically just a wrapper for .toFixed(2), but means we don't have to remember that functionality everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants