Som06 report 1 #2
Conversation
|
Changes made to existing code |
|
@sadan A |
There was a problem hiding this comment.
Please improve your variable and function naming conventions. I also see lot's of unnecessary comments, please remove them as well.
Personally, I believe functions and variable names should be named like so it's easy to understand the code by reading it but sometimes it's also good to add comments on sections where there is some calculation involved. Or to avoid inline comment you can add docstrings in functions to explain how the function actually works.
weather.py
Outdated
| exit | ||
|
|
||
|
|
||
| def yearcalc(y): |
There was a problem hiding this comment.
function names are always underscore "_" separated. e.g., year_calculator or yearly_report
There was a problem hiding this comment.
changed the function names as per the above
weather.py
Outdated
| mx_tmp = 0 | ||
| mx_hum = 0 | ||
| mn_tmp = 100 | ||
| os.chdir(r'C:\Som\Personal Projects\weatherdata') |
There was a problem hiding this comment.
This directory structure is not valid for Linux based systems. So if I clone this application I can't run this locally as this directory does not exist. Please find a better and generic way to read files from directory.
There was a problem hiding this comment.
changed with pathlib module which is generic for all OS
weather.py
Outdated
| if f.__contains__(y): | ||
| months.append(f) | ||
| for month in months: | ||
| with open (f'C:\\Som\\Personal Projects\\weatherdata\\{month}','r') as data_file: |
There was a problem hiding this comment.
Again, please check directory structure.
There was a problem hiding this comment.
changed with pathlib module
weather.py
Outdated
| with open (f'C:\\Som\\Personal Projects\\weatherdata\\{month}','r') as data_file: | ||
| #with open (r'C:\Som\Personal Projects\weatherdata\lahore_weather_1996_Dec.txt','r') as data_file: | ||
| csv_data = csv.reader(data_file, delimiter=',') | ||
| csv_data.__next__() |
There was a problem hiding this comment.
Again, avoid abuse of dunder methods. Check other ways to skip lines while reading a file.
There was a problem hiding this comment.
This one, i am not sure how to achieve. i will explore more
weather.py
Outdated
|
|
||
|
|
||
| months =[] | ||
| data_folder = Path("C:/Som/Personal Projects/weatherdata/") |
There was a problem hiding this comment.
This will still not work on my local machine if I try to run.
There was a problem hiding this comment.
Yes, that's correct. i have to give a generic folder name only like weatherdata and pathlib is resolve it for all kind of OS i guess
There was a problem hiding this comment.
Did you also look into os.path https://docs.python.org/3/library/os.path.html
weather.py
Outdated
| count = 0 | ||
| str_mo = ''.join(mo) | ||
| dt = datetime.strptime(str_mo, "%Y%m") | ||
| c_dt = str(dt.strftime("%Y_%b")) |
There was a problem hiding this comment.
what does strftime() returns?
There was a problem hiding this comment.
Then why are you converting str into str agaian?
There was a problem hiding this comment.
I think, i have done it mistakenly.
weather.py
Outdated
| count = 0 | ||
| str_mo = ''.join(mo) | ||
| dt = datetime.strptime(str_mo, "%Y%m") | ||
| c_dt = str(dt.strftime("%Y_%b")) |
There was a problem hiding this comment.
Then why are you converting str into str agaian?
| @@ -8,47 +8,42 @@ | |||
|
|
|||
There was a problem hiding this comment.
Global variables and specifically constants must always be named in all caps, DATA_FOLDER
weather.py
Outdated
| str_mo = ''.join(mo) | ||
| dt = datetime.strptime(str_mo, "%Y%m") | ||
| str_month = ''.join(month) | ||
| dt = datetime.strptime(str_month, "%Y%m") | ||
| c_dt = str(dt.strftime("%Y_%b")) | ||
| os.chdir(data_folder) |
There was a problem hiding this comment.
this is one way of doing this, but I don't see a reason to change the directory in the middle of code execution. Please check method scandir() and then you can do something like:
with os.scandir(DATA_FOLDER) as data_files:
for file in data_files:
pass
| data_f.readline() | ||
| reader = csv.DictReader(data_f, restkey=None, restval=None) | ||
| for line in reader: | ||
| if line["Max TemperatureC"] is None or line["Max TemperatureC"] == '': |
There was a problem hiding this comment.
What is the difference between the two checks?
a = None
b = ""
if a:
print("a")
if b:
print("b")What do you think the output of above code.
There was a problem hiding this comment.
Ideally both are same. but in my code if i use only one check i am getting the error for the others. That's why i used both check in this condition.
There was a problem hiding this comment.
So, what I was trying to display here is the way of writing this if statement. you can simplify this like
if line["Max TemperatureC"]:
pass| if int(line["Max Humidity"]) >= max_hum: | ||
| max_hum = int(line["Max Humidity"]) | ||
| maxhum_dt = line["PKT"] | ||
| else: |
There was a problem hiding this comment.
What iss the purpose of this else?
There was a problem hiding this comment.
This is not required. forgot to remove.
| c_mintmp_dt = string_to_date(mintmp_dt) | ||
| c_maxhum_dt = string_to_date(maxhum_dt) | ||
|
|
||
| return [max_tmp,c_maxtmp_dt,min_tmp,c_mintmp_dt,max_hum,c_maxhum_dt] |
There was a problem hiding this comment.
This is okay but not good. Now I have to go back and forth in yearly_report function to see what are you passing in the arguments.
There was a problem hiding this comment.
I didn't get that. Do you mean the arguments like maxhum_dt, mintmp_dt etc..?
There was a problem hiding this comment.
I think you must not return these in an array.
No description provided.