First code to repo by muhammadadeeltajamul · Pull Request #1 · muhammadadeeltajamul/training

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

+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

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


what is the purpose of this?

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

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


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

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

+    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

+        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

+        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

+    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

+        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

+    return
+
+
+def list_all_files(path):


add docstring here.

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

+
+
+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

+### 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

+        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

+        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

+    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

+        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

+    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

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


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

+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

+    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

+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

@@ -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

+    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

+
+    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

+    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

+    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

+    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

+    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

+    """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

+    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

+                                                     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

+        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

+BACKGROUND_COLOR_WHITE = 47
+
+
+month_number_string_mapping = {


Uses CAPS for constants.

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

+    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

+        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 = {

Conversation

muhammadadeeltajamul commented Jul 30, 2021

Uh oh!

saadyousafarbi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saadyousafarbi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saadyousafarbi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!