Josh-Voyles / affordable

Affordible is a simple financial calculator that helps you determine what you can afford.
GNU General Public License v3.0
0 stars 0 forks source link

Enable comma dollar #119

Closed YouKnowJoey closed 2 months ago

YouKnowJoey commented 2 months ago

Resolved #94: Text box can accept strict commas and dollar signs

Josh-Voyles commented 2 months ago

I have a couple of thoughts on this PR. 1st, excellent job on the test case. I like your user of a dictionary to iterate through all possibilities. Well done!

On the other hand, I'm really really trying to keep input validation out of the backend. I'm not even quite comfortable with the float conversion, but it's just a simple type check.

Here's what I would do different: I'd just do the replace() method as you did, but put it in the front end. verify_digits could be used to validate the digits and load them into class variables which could be passed to the af constructors. I would also just keep the VALID_ENTRY as we had it. I know you worked really hard on it, but it works well for only checking a valid number, and we don't have any invalid tests written for the new regex.

Thoughts?

YouKnowJoey commented 2 months ago

@Josh-Voyles Look at the new push, the checks are done in the main_window.py versus the original location in the 'affordability_calculator.py'. I recommend leaving the regex because this defines the grammar for the user inputs. Defining this as 'VALID_ENTRY' insinuates that this should be the user's valid entries.

Josh-Voyles commented 2 months ago

I like the idea of separation between the front and back. (front handles strings, back handles floats/ints).

However, I think there's still more discussion to be had about this implementation.

For example, concerning the regex, I want to mention coupling.

One of the reasons I'm strongly opposed to changing the regex is it couples us to the idea of considering "$" and "," as valid inputs. If later we decide the user should not be allowed to use these characters, we've coupled our application to this idea and it's harder to change.

Maybe "VALID_ENTRY" is not the right word and instead it should be "VALID_FLOAT." However, the current regex should be very close to how Python checks under the hood for numbers.

While the find/replace method is not perfect, it's one line of code that's easy to remove if we decide 'letting users do this is ridiculous.' Also, maybe the user accidentally added a space at the front or back? This is easy to strip out.

A think a more cohesive approach would be this:

if the user enters $4,000, '$' and ',' are simply replaced, '4000' is displayed back to the user in the text box (training the user), '4000' is validated before attempting conversion, then the results are displayed back to the user. 4_000 is also technically a valid number we may want to consider.

We are getting into the realm where we need to start refactoring the code. Think: "Is there a more simple solution to our code base?" I want to prevent patches and ensure the code base is cohesive. A common problem in software engineer is increasing maintenance costs as the code base grows. We are already seeing bugs crop up in our simple application.