-
Notifications
You must be signed in to change notification settings - Fork 12
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
Base Discrete Maths Package #105
Conversation
Fix same variable use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to start by talking about the comments. Comments provide additional information that the code itself cannot convey. They should add context to what you're doing or explain complex parts of the code when necessary. Consequently, it's important to avoid redundant comments that merely restate what is already evident from the code's readability. Comments should be reserved for situations where the code is not self-explanatory.
Here's some example of some unnecessary comments.
# add a and b
def add(a, b):
return a + b
# imports
from ... import ...
from ... import ...
import ....
# defines my_function
def my_function():
...
On another note, in Python you want to use doscstring when commenting functions, methods, classes or modules.
The second recurring issue I see is the use of abbreviation. Stuff like rpn
, op
, prop
, etc. It's a bad habit. Abbreviation should be avoided as much as possible; there's only a few cases where they are accepted. Change them for full words.
As for the feature itself, it seems to work well. However, it would be nice to have a command to see how to use it. A sort of guide; we don't really know which symbols to use without looking at the code. Some of the errors we get are also not super interesting. For info I did a v v b
It is also strange that I can do stuff like a v b b
or a a v b b
, etc. I feel like it should be an error.
Aside from that well done. It looks like a great PR and everything seems to work well in general.
return all(results) | ||
|
||
# Create a truth table from a proposition | ||
def create_truth_table(proposition: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic that creates the table shouldn't be here. It should be in the cog. However, the logic that evaluates the logical proposition can/should stay here.
The reason why is that this library shouldn't care too much about how the evaluation of a proposition is used. It should only care about evaluating it and returning some results. Who cares about how it's used and what is displayed is the cog. The cog is the link between the logic and the user.
In other words, all the sanity checkers and logic for the evaluation is fine here but the code to draw (this includes making the variables) the table shouldn't be here but in the cog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd change the name for something else that represent a bit more clearly what it does (which is to evaluate logical proposition). Right now, logical_operation
isn't very descriptive.
Also, it's in it's own package but it's alone? Wouldn't the parser module be included? Since this parser is built for this exactly that would be interesting to include it. Imagine if someone says "I want to take this libraries to use it somewhere else" but the parser is missing. It would be a bit awkward. So either move the parser there since it's for that only or move it somewhere else.
Created a discrete maths cog and discrete maths package, along with a simple logical operations module for said package which includes: