First code to repo #1

muhammadadeeltajamul · 2021-07-30T09:17:49Z

Complete code of the assignment

saadyousafarbi

I have gone through an overview of the PR and have requested some changes. There are a couple of things you should do in addition to these changes:

Push the data files as well as part of this PR (it would be easier for me to clone and test out your repo, and check functionality that way),

Add the Problem Statement shared with you to the README.md file along with a Usage Section where you should add the commands to run to generate the monthly, yearly etc reports,

Also go through pep8 conventions here https://www.python.org/dev/peps/pep-0008/, there are many docstring/variables/indentation issues that you will find here, please address them.

If you have any questions, please feel free to ask!

saadyousafarbi · 2021-08-03T04:46:24Z

weatherman.py

+import pandas as pd
+import os
+import argparse
+import sys
+import numpy as np


Imports should be ordered alphabetically.

saadyousafarbi · 2021-08-03T04:46:42Z

weatherman.py

+import numpy as np
+
+
+os.system("clear")


what is the purpose of this?

saadyousafarbi · 2021-08-03T04:47:29Z

weatherman.py

+os.system("clear")
+
+
+class WeatherData:


add a Docstring to the class describing what this class holds.

saadyousafarbi · 2021-08-03T04:48:36Z

weatherman.py

+    def add_month_data(self, month: int, year: int,
+                       month_data: pd.DataFrame):
+        if not (0 < month < 13):
+            print("Invalid month ", month, ". Data not added")


you need to return here. Or else the rest of the code will be executed.

saadyousafarbi · 2021-08-03T04:49:31Z

weatherman.py

+        self.__mean_humidity = " Mean Humidity"
+        return
+
+    def add_month_data(self, month: int, year: int,


add Docstring to method describing what arguments and outputs it gives, and what is its purpose!

saadyousafarbi · 2021-08-03T05:09:40Z

weatherman.py

+        min_temp_column = weather_data.get_column_data(col_name,
+                                                       month, year)
+    max_len = len(max_temp_column)
+    min_len = len(min_temp_column)


what does this variable represent? it is not clear from name.

saadyousafarbi · 2021-08-03T05:10:11Z

weatherman.py

+    len_ = max_len
+    if max_len != min_len:
+        if min_len < max_len:
+            len_ = min_len
+        print_str = "\033[1;31;40m Note: Unequal data of maximum"
+        print_str += " and minimum temperature"
+        print(print_str)


what is this piece of code doing? maybe move this to a separate method!

saadyousafarbi · 2021-08-03T05:10:53Z

weatherman.py

+        suffix_text = str("\033[0;37;40m%.2d " % (i+1))
+        low_print = "\033[0;34;40m" + "+"*abs(int(min_temp_column[i]))
+        high_print = "\033[0;31;40m" + "+"*abs(int(max_temp_column[i]))
+        add_text = "\033[0;34;40m "
+        add_text += (str(min_temp_column[i]) + "C\033[1;37;40m-")
+        add_text += ("\033[0;31;40m" + str(max_temp_column[i]) + "C")
+        text = suffix_text + low_print + high_print + add_text
+        print(text)


better variable names needed here, maybe move this to a seperate method as well.

saadyousafarbi · 2021-08-03T05:14:08Z

weatherman.py

+    return
+
+
+def list_all_files(path):


add docstring here.

saadyousafarbi · 2021-08-03T05:14:49Z

weatherman.py

+
+
+def load_data_from_file(filename):
+    name, ext = os.path.splitext(filename)


add docstring here.

…d README.md and requirements.txt. Added DOCStrings to all methods and classes.

saadyousafarbi

Reviewed the PR. Requested some changes, please have look. Here are some other points to cater:

add a .gitignore file (read about what it does), and add the weather data files to it (and any other extra files not needed as part of code),
add requirements.txt file with all the requirements to run this code,
Docstrings are not in the correct format throughout the code, please revisit the pep8 conventions,
Remove the usage of pandas and numpy, and redesign the code to compute the reports by itself, without help of external packages.

If you have any questions, please free feel to ask!

saadyousafarbi · 2021-08-05T03:56:47Z

README.md

+### Install virtualenv and virtualenvwrapper
+_pip install virtualenv_
+
+_pip install virtualenvwrapper_
+### Add variables to path (Recommended)
+
+export WORKON_HOME=$HOME/.virtualenv
+
+export PROJECT_HOME=$HOME/projects
+### Find virtualenvwrapper and run it
+_which virtualenvwrapper.sh_
+
+_The command will output a path similar to this "/usr/local/bin/virtualenvwrapper.sh"_
+
+Enter the following command to run this file
+
+_source /usr/local/bin/virtualenvwrapper.sh_
+
+### Reload startup file ".bashrc"
+_source ~/.bashrc_
+
+### Create a new virtual environment
+_mkvirtualenv env-name_
+


use venv for virtual envirnoment setup. It is much easier to use
https://docs.python.org/3/library/venv.html

we dont need all the commands related to the virutal envirnoment here, we are only cocerned with our usage. Simplify this to having steps like

command to install virtualenv,

command to activate virtualenv,

command to install requirements of project from requirement.txt

saadyousafarbi · 2021-08-05T04:06:20Z

WeatherData.py

+        add_month_data(self, month, year, month_data)
+        self: object in which you want to add data
+        month: integer range(1-12)
+        year: integer representing year
+        month_data: pandas data frame having data of all days
+
+        Stores month weather information in data structure


This is not the correct format for a docstring. Please check pep8

saadyousafarbi · 2021-08-05T04:06:52Z

WeatherData.py

+        Pass alias to get column name
+        Valid aliases are
+            max_temperature
+            min_temperature
+            max_humidity
+            min_humidity
+            mean_humidity
+
+        Returns column name for other function from alias


check docstring format from pep8

saadyousafarbi · 2021-08-05T04:08:15Z

common.py

+    return month and year from combined string
+
+    month and year should be numbers and separated with /
+    Examples:
+        input string: 12/2006
+            output: 12, 2006
+        input string: jan/2006
+            output: None, None


check docstring format.

saadyousafarbi · 2021-08-05T04:08:56Z

common.py

+        month = int(month)
+        year = int(year)
+    except Exception:
+        print("Invalid input")


add more information to this print statement, include reason why the input is invalid.

saadyousafarbi · 2021-08-05T04:11:50Z

common.py

+    if number == 1:
+        return "Jan"
+    elif number == 2:
+        return "Feb"
+    elif number == 3:
+        return "Mar"
+    elif number == 4:
+        return "Apr"
+    elif number == 5:
+        return "May"
+    elif number == 6:
+        return "Jun"
+    elif number == 7:
+        return "Jul"
+    elif number == 8:
+        return "Aug"
+    elif number == 9:
+        return "Sep"
+    elif number == 10:


have a dict mapping here instead of if/else statements.

saadyousafarbi · 2021-08-05T04:12:10Z

common.py

+    get_column_data_from_alias(weather_data, alias)
+    alias: Alias of column
+    Returns complete column data from alias


saadyousafarbi · 2021-08-05T04:12:53Z

generate_reports.py

+TEXT_STYLE_NORMAL = 0
+TEXT_STYLE_BOLD = 1
+TEXT_STYLE_LIGHT = 2
+TEXT_STYLE_ITALICIZED = 3
+TEXT_STYLE_UNDERLINED = 4
+TEXT_STYLE_BLINK = 5
+
+TEXT_COLOR_BLACK = 30
+TEXT_COLOR_RED = 31
+TEXT_COLOR_GREEN = 32
+TEXT_COLOR_YELLOW = 33
+TEXT_COLOR_BLUE = 34
+TEXT_COLOR_PURPLE = 35
+TEXT_COLOR_CYAN = 36
+TEXT_COLOR_WHITE = 37
+
+BACKGROUND_COLOR_BLACK = 40
+BACKGROUND_COLOR_RED = 41
+BACKGROUND_COLOR_GREEN = 42
+BACKGROUND_COLOR_YELLOW = 43
+BACKGROUND_COLOR_BLUE = 44
+BACKGROUND_COLOR_PURPLE = 45
+BACKGROUND_COLOR_CYAN = 46
+BACKGROUND_COLOR_WHITE = 47


move these to another file, name it constants or settings

why do we have so many of these colors and fonts, are we using all of them?

saadyousafarbi · 2021-08-05T04:13:34Z

generate_reports.py

+    This functions returns a string. Add this string in the start
+    of the string whose color or style you want to change
+
+    change_text_color(style, color, background_color)
+    style: changes style, default style "Normal"
+    color: changes text color, default color "White"
+    background_color: changes background color, default "Black"
+
+    Pass constants in the function, string colors are not allowed


check pep8 docstring format.

saadyousafarbi · 2021-08-05T04:14:29Z

generate_reports.py

+BACKGROUND_COLOR_WHITE = 47
+
+
+def change_text_color(style=0, color=37, background_color=40):


improve function name, not clear what the function is doing from reading the name!

…irements.txt. Added .gitignore. Updated docstrings.

saadyousafarbi

Added some comments, please resolve them!

Why are there 2d arrays?

saadyousafarbi · 2021-08-10T09:18:42Z

common.py

@@ -0,0 +1,105 @@
+def get_month_year_from_string(string):
+    """Returns month and year from combined string
+    month and year should be numbers and separated with /


/ -> slash

saadyousafarbi · 2021-08-10T09:19:43Z

common.py

+    mapping_dictionary = {
+        "jan": 1,
+        "feb": 2,
+        "mar": 3,
+        "apr": 4,
+        "may": 5,
+        "jun": 6,
+        "jul": 7,
+        "aug": 8,
+        "sep": 9,
+        "oct": 10,
+        "nov": 11,
+        "dec": 12
+    }
+    return mapping_dictionary.get(string, None)


move this mapping to constants file.

saadyousafarbi · 2021-08-10T09:20:10Z

common.py

+
+    string = str(string).lower()
+    # Map month name to number
+    mapping_dictionary = {


what does this variable represent? name should be more descriptive!

saadyousafarbi · 2021-08-10T09:21:47Z

common.py

+    mapping_dictionary = {
+        1: "Jan",
+        2: "Feb",
+        3: "Mar",
+        4: "Apr",
+        5: "May",
+        6: "Jun",
+        7: "Jul",
+        8: "Aug",
+        9: "Sep",
+        10: "Oct",
+        11: "Nov",
+        12: "Dec"
+    }


move this to constants.
You can use a single dict for the mapping, instead of having 2 different dicts.

saadyousafarbi · 2021-08-10T09:22:06Z

common.py

+    return mapping_dictionary.get(string, None)
+
+
+def get_month_string_from_number(number: int):


change number to a better variable name!

saadyousafarbi · 2021-08-10T09:42:40Z

generate_reports.py

+    return color_string
+
+
+def arg_max(list_):


name of function needs to be more descriptive!

input name needs to be more descriptive!

why is there an underscore after the input variable name?

saadyousafarbi · 2021-08-10T09:44:13Z

generate_reports.py

+    if not list_:
+        return None
+    max_index = 0
+    max_value = list_[max_index]
+    for i in range(0, len(list_)):
+        if list_[i] > max_value:
+            max_value = list_[i]
+            max_index = i
+    return max_index


what is happening in this method? Can we not do this computation with lists, instead of needing a separate function?

saadyousafarbi · 2021-08-10T09:44:41Z

generate_reports.py

+    """Returns the index of minimum element of list
+
+    Arguments:
+        list_:
+            list whose minimum index is required
+
+    Returns:
+        index: int:
+            The index whose value is minimum
+    """
+
+    if not list_:
+        return None
+    min_index = 0
+    min_value = list_[min_index]
+    for i in range(0, len(list_)):


same concerns as for the method arg_min

saadyousafarbi · 2021-08-10T09:45:48Z

generate_reports.py

+    return min_index
+
+
+def get_boundary_element(list_, max_=True, index=0):


why are there underscores after the variables here?

saadyousafarbi · 2021-08-10T09:47:18Z

generate_reports.py

+                                                     month, year)
+        if max_temp_column:
+            max_index = arg_max(max_temp_column)
+            max_temp_list.append([max_temp_column[max_index],


what is happening here?

AhtishamShahid

beyond other suggested changes I would like to see if you can add unit tests for the project.

AhtishamShahid · 2021-08-13T10:46:36Z

WeatherData.py

+        self.__max_humidity = "Max Humidity"
+        self.__min_humidity = " Min Humidity"
+        self.__mean_humidity = " Mean Humidity"
+        return


I don't think we need return here

AhtishamShahid · 2021-08-13T10:50:23Z

constants.py

+BACKGROUND_COLOR_WHITE = 47
+
+
+month_number_string_mapping = {


Uses CAPS for constants.

AhtishamShahid · 2021-08-13T10:53:21Z

generate_reports.py

+    load_year_data(weather_data, path, year)
+
+    # Checking data of all 12 months
+    for i in range(0, 12):


month in range(1,13) can be used , month variable month = i +1 can be removed

AhtishamShahid · 2021-08-13T10:56:59Z

generate_reports.py

+        max_temp_column = get_column_data_from_alias(weather_data,
+                                                     "max_temperature",
+                                                     month, year)
+        if max_temp_column:


Line 141 to 145 look similar to 151 to 157 see if it can be extracted in functions.

First code to repo

5b518e1

saadyousafarbi suggested changes Aug 3, 2021

View reviewed changes

Code changed as required. Splitted the code into multiple files. Adde…

38cabbf

…d README.md and requirements.txt. Added DOCStrings to all methods and classes.

saadyousafarbi suggested changes Aug 5, 2021

View reviewed changes

adeel added 3 commits August 6, 2021 17:30

Completed changes. Removed numpy and pandas requirement. Updated requ…

0a0f5c9

…irements.txt. Added .gitignore. Updated docstrings.

Updated .gitignore

28591e4

Added .zip file

854864e

saadyousafarbi suggested changes Aug 10, 2021

View reviewed changes

Changes to improve readability

1851b39

AhtishamShahid reviewed Aug 13, 2021

View reviewed changes

Followed constant variable name

6bed126



		def load_data_from_file(filename):
		name, ext = os.path.splitext(filename)

		BACKGROUND_COLOR_WHITE = 47


		def change_text_color(style=0, color=37, background_color=40):

		return mapping_dictionary.get(string, None)


		def get_month_string_from_number(number: int):

		return min_index


		def get_boundary_element(list_, max_=True, index=0):

		import numpy as np


		os.system("clear")

		BACKGROUND_COLOR_WHITE = 47


		month_number_string_mapping = {

First code to repo #1

Are you sure you want to change the base?

First code to repo #1

Conversation

muhammadadeeltajamul commented Jul 30, 2021

saadyousafarbi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saadyousafarbi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saadyousafarbi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AhtishamShahid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment