Skip to content

Dev 2#3

Open
franciscoprin wants to merge 5 commits into
masterfrom
dev_2
Open

Dev 2#3
franciscoprin wants to merge 5 commits into
masterfrom
dev_2

Conversation

@franciscoprin

@franciscoprin franciscoprin commented Oct 1, 2020

Copy link
Copy Markdown
Owner

Required enhancements.

@franciscoprin franciscoprin left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Anahis259 or @carlosvivasb could you guys do these changes?

Comment thread phone.py Outdated
Comment on lines +16 to +18
phone_doesnt_start_with_plus = Phone("+13463335555")
phone_greater_than_15_characters = Phone("+13463335555321412342134")
phone_with_special_characters = Phone("!~#$%^&*@|}{")

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Anahis259 please remove this. I am curious, why do you think that you have to add these lines?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chicos yo me encargo de hacer los cambios donde marque "Aquí"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aquí

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya modifique las ultimas 3 correcciones, las subo...

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes made

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new changes made

Comment thread phone.py
Comment on lines +45 to +47
phone_keyboard_mapper ={'a':1, 'b':1,'c':1,'d':2,'e':2,'f':2,'g':3,'h':3,'i':3,'j':4,'k':4,'l':4,'m':5,'n':5,'o':5,'p':6,'q':6,'r':6,'s':7,'t':7,'u':7,'v':8,'w':8,'x':8,'y':9,'z':9,

}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this dictionary to the helper.py module and then inside of the phone.py file do:

from helper import phone_keyboard_mapper

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aquí

Comment thread phone.py
Comment on lines +50 to +53
#"""
#Should convert letter to digit (see image in the README.md)
#clue 1: use a dictionary.
#""" No newline at end of file

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines should be the first one in the method get_number

It is describing what the get_number method is doing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aquí

Comment thread symbol_factory.py
Comment on lines +7 to +12
#"""
#Returns the corresponding symbol's string shape using only |, _, -, /, \,
#Look at the example output in README.md.

#clue 1: use a dictionary to match the hardcode string with the corresponding symbol.
#"""

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar than above, this should be the first lines of code of the method get_string, they are describing what the get_string method is doing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aquí

Comment thread helper.py Outdated
Comment on lines +230 to +234
#def validate_Phone(self) :
#phone_doesnt_start_with_plus = Phone("+13463335555")
#phone_greater_than_15_characters = Phone("+13463335555321412342134")
#phone_with_special_characters = Phone("!~#$%^&*@|}{")
#Return False

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete these lines of code.

Comment thread helper.py Outdated
]
}

print (my_dict[single_simbol])

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete line 227

Comment thread helper.py Outdated
Comment on lines +143 to +157
my_dict ={ "1" : [
" ____ ",
" / | ",
"/_/| | ",
" | | ",
" | | ",
" _| |_ ",
" |______|",
], "2" : [
" _____ ",
" / __ | ",
"/__/ / / ",
" / / ",
" / / ",
" / /____ ",

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are repeating code here:

The list-numbers are already defined above.

For example

number_one = [
  "  ____   ",
  " /    |  ",
  "/_/|  |  ",
  "   |  |  ",
  "   |  |  ",
  "  _|  |_ ",
  " |______|",
]

...
# Rest of the numbers
...

number_nine = [
  "  _______ ",
  " /   _   |",
  "|   |_|  |",
  " \\___    |",
  "     |   |",
  "     |   |",
  "     |___|",
]


my_dict ={ 
"1" : number_one,
...
# follow the parent here and write the rest of the numbers.
....
"9":number_nine  
}

@franciscoprin franciscoprin left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Anahis259, do these changes, please 😄 .
@carlosvivasb, try to understand what changes I am asking to @Anahis259 👀.

Comment thread helper.py Outdated
Comment on lines 143 to 146
my_dict ={
"1" :number_one,, "2" : number_two, "3" : number_three, "4" : number_four, "5" : number_five,"6" : number_six, "7" : number_seven, "8" :number_eight,
, "9" :number_nine,"0" :number_zero
}

@franciscoprin franciscoprin Oct 2, 2020

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Anahis259 , you are almost there, Two more changes:

  1. Break this dictionary into multiple lines
    (fewer lines of code no always mean better code, your main focus is that your code is readable.)
my_dict ={ 
"1" :number_one,
"2" : number_two, 
"3" : number_three, 
....
# The rest of the lines of code.
....
"8" :number_eight,
"9" :number_nine,
"0" :number_zero,
}
  1. Add a meaningful name for the dictionary.
    (my_dict is too vague)
from_num_str_to_num_dict_mapper ={ 
"1" :number_one,
"2" : number_two, 
"3" : number_three, 
....
# The rest of the lines of code.
....
"8" :number_eight,
"9" :number_nine,
"0" :number_zero,
}

@franciscoprin franciscoprin left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a tiny change @Anahis259 in the name is from_num_str_to_num_dict_mapper nope form_num_str_to_num_dict_mapper 😃

Comment thread helper.py Outdated
Comment on lines 143 to 154
form_num_str_to_num_dict_mapper ={
"1" : number_one,
"2" : number_two,
"3" : number_three,
"4" : number_four,
"5" : number_five,
"6" : number_six,
"7" : number_seven,
"8" : number_eight,
"9" : number_nine,
"0" : number_zero,
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Comment thread symbol_factory.py Outdated
Comment on lines +1 to +4
from helper import my_dict
from helper import form_num_str_to_num_dict_mapper
class SymbolFactory:
def get_string(self, single_simbol):
return my_dict[single_simbol]
return form_num_str_to_num_dict_mapper[single_simbol]

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am so happy that you realize that you also have to change the imports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants