Implement Weather functionality - #1
Open
developer-jawad wants to merge 18 commits into
Open
Conversation
…ature in a given month and mean humidity
… function and also make some chnages in main function according to this
…hange to getSingleDateRecord and make a global function with name displayReport
… than comparison there is no value is set in required date because both temperature are same. remove this bug and when i return object from _calculateTemperatureWithField function it return the last file it is read alse remove this bug
m-Hariss
changed the title
m-Hariss
requested changes
May 24, 2024
Collaborator
There was a problem hiding this comment.
Good work @developer-jawad! We can make following improvements:.
- Use meaningful naming conventions.
- Use a bit more smart and cleaner strategy.
- Functions to be as less coupled with each as possible (does not mean avoid dividing tasks into functions, but it should not be dependant of how the one caller calling functions. It just be aware of what its provided with and what is expected from it.).
- Can use different files for different functionality and data.
- Handle exceptions carefully, application must not crash at all.
- Use brief and to the point commit messages (as suggested). Like:
fix: in _calculateInfoFromFile when compare for greater than and less than comparison there is no value is set in required date because both temperature are same. remove this bug and when i return object from _calculateTemperatureWithField function it return the last file it is read alse remove this bug
can be just
fix: temperate comparison in calculating information
or this (in case need a detailed one):fix: resolve issues in _calculateInfoFromFile and _calculateTemperatureWithField - Correct comparison logic in _calculateInfoFromFile: Ensure the required date is set correctly even when both temperatures are the same. - Fix _calculateTemperatureWithField to return the correct file: Ensure the function returns the appropriate file instead of the last file read.
Comment on lines
+56
to
+57
| if len(self.data) == 0: | ||
| raise Exception("No file is found") |
Collaborator
There was a problem hiding this comment.
Can this be out of if/else?
Owner
Author
There was a problem hiding this comment.
no, it is not outside the if condition because when there is not file exists with the name of given month then it will raise an exception
| """ | ||
| def readDataFromSingleFile(self, month): | ||
| try : | ||
| with open(f'weather_reports/Murree_weather_{self.year}_{month}.txt', 'r') as file: |
Collaborator
There was a problem hiding this comment.
Can file path be a constants?
Owner
Author
There was a problem hiding this comment.
no file paths are not constants year and month are dynamic which coming from user
|
|
||
| return required_data | ||
|
|
||
| def _calculateInfoFromFile(self, single_file_record, field, maxNumber): |
Collaborator
There was a problem hiding this comment.
Can we make the classes and functions be independent of whether the data was loaded from file or db or any other source?
Comment on lines
+232
to
+233
| if len(splitted_file_date) >= 2: | ||
| bar_count = int(input("Please Enter Number of Report bar_counts 1-2: ")) |
Collaborator
There was a problem hiding this comment.
What is the purpose of this?
Owner
Author
There was a problem hiding this comment.
it is used to display report in one horizontal bar or 2 horizontal bars
Collaborator
There was a problem hiding this comment.
Lets use Command Line Arguments as in examples mentioned in task?
m-Hariss
requested changes
Jun 23, 2024
Comment on lines
+38
to
+39
| if len(self.data) == 0: | ||
| raise Exception("No file is found") |
Collaborator
There was a problem hiding this comment.
https://github.com/developer-jawad/Weatherman/pull/1/files#r1615940065
We have code duplication, if we keep it inside if/else. Thought?
| """ | ||
| def readDataFromSingleFile(self, month): | ||
| try : | ||
| with open(f'weather_reports/Murree_weather_{self.year}_{month}.txt', 'r') as file: |
Comment on lines
+100
to
+111
| if type_of_calculation == 'H': | ||
| return self._calculateTemperatureWithField('Max TemperatureC', True) | ||
| elif type_of_calculation == 'L': | ||
| return self._calculateTemperatureWithField('Min TemperatureC', False) | ||
| elif type_of_calculation == 'Hu': | ||
| return self._calculateTemperatureWithField('Max Humidity', True) | ||
| elif type_of_calculation == 'avg_L': | ||
| return self._calculateTemperatureWithField('Mean TemperatureC', False) | ||
| elif type_of_calculation == 'avg_H': | ||
| return self._calculateTemperatureWithField('Mean TemperatureC', True) | ||
| elif type_of_calculation == 'avg_Hu': | ||
| return self._calculateTemperatureWithField('Max Humidity', True) |
Collaborator
There was a problem hiding this comment.
What if we use Dict here. Thought?
Comment on lines
+232
to
+233
| if len(splitted_file_date) >= 2: | ||
| bar_count = int(input("Please Enter Number of Report bar_counts 1-2: ")) |
Collaborator
There was a problem hiding this comment.
Lets use Command Line Arguments as in examples mentioned in task?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… than comparison there is no value is set in required date because both temperature are same. remove this bug and when i return object from _calculateTemperatureWithField function it return the last file it is read alse remove this bug