There are really great answers here, but i want to show you how do i think when i see comments (and every time i want to add one).
Comments are used normally when you want to clarify your code. But, if
your code is clear enough, the comment will become redundant. And
reading redundant things makes ourselves feels like we are wasting
time, so naturally the comment start to be innecesary: deletable.
I will suppose you understood the given answers by others here, so i will focus on comments:
def is_month_valid(month):
return 0 < month <= 12 ## If month is between 1 and 12, return
If you are in a situation where you want to keep your comment because you find it clearer than your code: Make your code look a bit more like that comment.
For example, you could try thinking:
return 0 < month <= 12 ## If month is between 1 and 12, return
return 1 <= month <= 12
return is_between(month, 1, 12)
return is_the(month).between(1, 12)
return comparable(month).is_between(1, 12) ## Or you could call is_month_valid(comparable(month)) instead and this gives you...
return month.is_between(1, 12) ## If month is between 1 and 12, return
## Also, if month.is_between(1, 12): return... I think the other answers actually answers why this should be discarded. If not, keep reading and think why.
All of them are valid solutions. And if you see, the last one will make the comment really redundant:
def is_month_valid(month):
return month.is_between(1, 12) ## If month is between 1 and 12, return
So, why you will keep this comment? The code is clear, and actually, that comment is making your code look unclean, so you don't have another choice than delete that.
Yay!!... Fixed? ... Is this better? We just focus on readability and nothing else? Not exactly. You have to keep in mind the complexity that each solution has. So:
return 0 < month <= 12 ## If month is between 1 and 12, return
This is a pretty good solution. Why? Is readable to the point it makes the comment redundant. And you don't need nothing else to make it work. But
return 1 <= month <= 12 ## If month is between 1 and 12, return
Is this better? The comment now is more redundant. And you didn't add complexity. This is like a free change. Also, this doesn't allow numbers between (0, 1) as valid months. So yes, this is actually better.
return is_between(month, 1, 12) ## If month is between 1 and 12, return
Well, at least for me, this is less legible than previous ones. And needs you to create another function to make it work. Is this function worth enough? Well, one could point that if you create it you could reuse that function in different places, like in is_between(year, 1, 10000)
. And it allows you to centralize integer validations here... But, you have to decide. That's programming actually: Take decisions and justify them. (Like naming things...)
return is_the(month).between(1, 12) ## If month is between 1 and 12, return
This is actually more redundant and more readable. But, now, you've created a class (another function) with an arguable name, and also another function associated (like a method). However, this maintains the pros from last solution. At this point, this is starting to feel like an unnecessary over engineered solution, but we are just explaining our thinkings.
return comparable(month).is_between(1, 12) ## If month is between 1 and 12, return
Why is_the
an arguably name? Think that some time has passed and you see a function called is_the. My main reaction would be "What the heck is this thing!". It says nothing. You can't know it's purpose without reading it inner code. So, this solution fixes a bit that arguably name. Now month
is comparable to 1 and 12. However, this is still over engineering for a little advantage (helps to dry).
return month.is_between(1, 12) ## If month is between 1 and 12, return
Now, this is better. That code is more redundant and readable than previous solutions. But, what is its price? Now, every time you want to call is_month_valid
you depend on sending a comparable object first: is_month_valid(comparable(month))
. And don't forget you still have to create comparable and is_between too to make this work. But, is this over engineered solution needed? That will depend on how necessary is this in your program: how much this helps to dry, and how probably is this going to change in the future. If it's not stable, could cause more problems than fixes.
Of course, for every solution exists a better one at a particular time. To get them you just have to continue over engineering it and balance their pros and cons. I will not continue doing that because i think is clear now. And looking at the size of your program, i'm sure the last solution is too much, while the second one (1 <= month <= 12
) is simple and clear.
And keep doing the same for every comment you have, or you are tempted to make.
monthlist1 = [1,3,5,7,8,10,12] ## monthlist for months with 31 days.
monthlist2 = [4,6,9,11] ## monthlist for months with 30 days.
monthlist3 = 2 ## month with month with 28 days.
Wouldn't those lines be replaceable with this?
months_with_31_days = [1,3,5,7,8,10,12] ## monthlist for months with 31 days.
months_with_30_days = [4,6,9,11] ## monthlist for months with 30 days.
month_with_28_days = 2 ## month with month with 28 days.
Redundant comments now, they are just messing with your code.
months_with_31_days = [1,3,5,7,8,10,12]
months_with_30_days = [4,6,9,11]
february = 2 ## And month_with_28_days? Isn't that just february?.
Also, like @heather said, i would prefer to make a function days_in_month instead of this. But, my focus here is to show how do i think when facing comments.
for month in monthlist: ## iterate through monthlist.
I know this is going to dissapear, but, is that comment actually needed? Is for (each) month in monthlist:
less clear than iterate through monthlist? I'm sure most programmers are going to recognize what is this line doing by just reading it. In this case, i think the comment isn't needed.
if month == mon: ## Check if the parameter month equals to any month with 31 days.
This is an interesting comment, because you need some context to understand this code. You can't just read that and know what is happening. You need this picture:
def is_day_valid(month, day):
...
for mon in months_with_31_days:
if month == mon: ## Check if the parameter month equals to any month with 31 days.
Now, you can understand why you use mon
instead of month, and why is there another month variable. This situation; when you need some context to understand a line; is undesirable. Luckily, you've put a comment with the required context to understand it, but, is necessary? This was solved by @Gerrit0:
if month in months_with_31_days: ## Check if the parameter month equals to any month with 31 days.
That comment does add noise! Remove it and its perfect. But what can you do if you needed that for?
for month_with_31_days in months_with_31_days:
if month == month_with_31_days: ## Check if the parameter month equals to any month with 31 days.
Now is possible to forget that for
line while reading the if
line if you want. You don't care which month with 31 days it is, but you know if you enter inside that if
block is because month
have 31 days. And you can over engineer solutions like we did previously for getting more readability in that single line... Check for example if days_in(month) == 31:
, that will move the previous for line from is_day_valid
. Try to make functions short and readable. With less lines of code in each function, you have less probability to need more context.
What about this?
date = input("Enter the date in mm/dd/yyyy format: ") ## Input date in the given format.
If you read that line, you know you are asking the user for an input and saving it in a variable called date
. You are actually saying less in that comment. However, redundant and needless. And what about dividing that user interaction into another function. For example:
date = ask_user_for_input_a_date()
Too verbose, and unnecessary in this case. However, i want to point that, when you start to have multiple inputs, you will start asking yourself where to ask those inputs. The same happens with the outputs. One good practice for novice is to divide your main
function like:
def main():
inputs = take_inputs() ## You could validate inputs inside if you want
solution = solve_problem(inputs)
show(solution)
You gather all needed information before starting your problem (group your inputs), then solve the problem completely (group your outputs) and lastly show your solution. If you have problems related to inputs, you know where to start looking for. The same applies to the core logic of your problem and final presentation. This is a nice starting point, and fit well on problems that don't need user interaction while solving it.
However, nice start, seriously. I've started my programming life with the same problem; date validation, and my code was a lot worse than yours :)