-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code to Change Words to LowerCase #188
base: master
Are you sure you want to change the base?
Conversation
new Locale(language) class JavaCharactertoLowerCaseExample1 {
} String myString="YAŞAT Patel"; Log.v("mainlist", "en source: " +myString.toLowerCase(enLocale)); |
S23/mepripri/ToLowerCase.py
Outdated
@@ -0,0 +1,49 @@ | |||
import csv | |||
|
|||
def splitLang(l): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is great for getting the first part of the language input, but we should make l
a little more descriptive in terms of the variable name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, i will work on the variable name.
S23/mepripri/ToLowerCase.py
Outdated
lang=splitLang(lang) | ||
if lang=='tr' or lang=='az': | ||
for i in s: | ||
if ord(i)==73: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this may work, I think the use of hard coded numbers is not ideal. Maybe if the function merely searches for 'I' instead of using ord() that would be a little clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that also perfect that will be a direct search.
S23/mepripri/ToLowerCase.py
Outdated
str+=chr(962) | ||
else: | ||
str=str+s.lower() | ||
elif lang=='zh' or lang=='th' or lang=='ja' or lang=='en': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to specify these four languages, as anything else would be just lower cased. So this could just be an else:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, i also have a non-language condition there. To solve this I can add that condition in elif and make the rest as else. This will be more better.
S23/mepripri/ToLowerCase.py
Outdated
str=str+s.lower() | ||
elif lang=='zh' or lang=='th' or lang=='ja' or lang=='en': | ||
str+=s.lower() | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this as any other language is just lower cased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is similar to the above one I guess
S23/mepripri/ToLowerCase.py
Outdated
first=1 | ||
else: | ||
str+=s[i].lower() | ||
elif i==1 and first==1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the elif
at line 28 is doing. This may be omitted as it doesn't appear to do anything as of this moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This elif condition is actually working as a flag. I will try to look into the code again, if this can be avoided.
@@ -1,18 +1,29 @@ | |||
HELLO en hello | |||
WORLD en-US world | |||
WORLD eng-US Invalid Language |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eng
would likely produce an invalid language type, but it could also just be lower cased since any other language that is not of the ones specified is just lowered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is added because this doesn't fit in the language requirement. If any language is greater than or less than 2 letters will be an invalid language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this code looks good and you added a decent number of test cases. There is room for improvement in terms of readability and some unnecessary lines. Well done!
I have made the changes to the code. @thofstrand, Can you please review them again |
import csv | ||
|
||
def splitLang(lang): | ||
lang=lang.split('-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, but I don't really like the change of type here from str to list.
if lang=='tr' or lang=='az': | ||
for i in s: | ||
if i=='I': | ||
str+=chr(305) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number.
str+=i.lower() | ||
elif lang=='ga': | ||
if (s[0]=='n' or s[0]=='t') and s[1] in ['A','E','I','O','U','Á','É','Í','Ó','Ú']: | ||
str+=s[0].lower()+'-'+s[1:].lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s[0] already lowercase here?
else: | ||
str+=s.lower() | ||
elif lang=='el': | ||
if ord(s[-1])==931: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic numbers — unreadable.
str+=chr(962) | ||
else: | ||
str=str+s.lower() | ||
elif len(lang)!=2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check out the BCP-47 spec. Language codes can have 2 or 3 letters.
else: | ||
str=str+s.lower() | ||
elif len(lang)!=2: | ||
str+='Invalid Language' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should raise an exception here, not return an error string.
No description provided.