cmrincon
Posts: 44
Joined: Thu May 24, 2018 7:39 pm

Help my code to look better

Sun Jun 07, 2020 6:02 pm

Hi! today i read the google style guide (https://google.github.io/styleguide/pyguide.html) and i started to make some of my code to look nice. But this function it still looks awful. Roast my code, all tips all welcome

Code: Select all

def read_config_file(config_file_path):
    """Reads and parse a config file.

Args:
        config_file_path: path to the file to parse.

Returns:
        A dictionary with the mapped settings.
"""

    spells_dictionary = {}
    line_number = 0
    with open(config_file_path, 'r') as config_file:
        for line in config_file:
            line_number += 1
            uncommented_line = line.strip().split('#')
            if len(uncommented_line[0]) == 0:  continue
            splited_uncommented_line =  [x.strip() for x in uncommented_line[0].split(',')]
            if len(splited_uncommented_line) != 2:
                print("ERROR: incorrect line format".format(cfgPath))
                continue
            try:
                spell_id = int(splited_uncommented_line[1])
                spells_dictionary[spell_id] = splited_uncommented_line[0]  #Splited_uncommented_line[0] = spell Name
            except valueError as e:
                 print("ERROR: incorrect line format".format(cfgPath))

    return spellsDict
    

jahboater
Posts: 5927
Joined: Wed Feb 04, 2015 6:38 pm
Location: West Dorset

Re: Help my code to look better

Sun Jun 07, 2020 7:01 pm

Even for something simple like this, I would put the protected code on the next line:

Code: Select all

if len(uncommented_line[0]) == 0:  continue

Code: Select all

if len(uncommented_line[0]) == 0:  
  continue
Is Splited spelled correctly?

Code: Select all

splited_uncommented_line =  [x.strip() for x in uncommented_line[0].split(',')]
Possibly some blank lines separating distinct logic blocks or sections.
Pi4 8GB running PIOS64 Lite

Heater
Posts: 16299
Joined: Tue Jul 17, 2012 3:02 pm

Re: Help my code to look better

Sun Jun 07, 2020 7:43 pm

It's a trick question.

It is not possible to make Python source code look nice. :)

Before trying to make it look nice I would make it at least work. That function returns a non-existent variable spellsDict.

I would remove the confusing comment "#Splited_uncommented_line[0] = spell" which looks like it is dead code that has been commented out.

I find the ugliest part being variable names like "splited_uncommented_line". There must be something shorter and sweeter. Like "words" or "fields"

I don't know if it's required by some Python documentation standard but I would move the comment block at the top to outside the function so as to not break up the code.

Something like:

Code: Select all

"""
Reads and parse a config file.

Args:
    config_file_path: path to the file to parse.

Returns:
    A dictionary with the mapped settings.
"""
def read_config_file(config_file_path):
    spells = {}
    line_number = 0
    with open(config_file_path, 'r') as config_file:
        for line in config_file:
            line_number += 1
            uncommented_line = line.strip().split('#')
            if len(uncommented_line[0]) == 0:  continue
            words =  [x.strip() for x in uncommented_line[0].split(',')]
            if len(words) != 2:
                print("ERROR: incorrect line format".format(cfgPath))
                continue
            try:
                spell_id = int(words[1])
                spells[spell_id] = words[0]
            except valueError as e:
                 print("ERROR: incorrect line format".format(cfgPath))

    return spells
Memory in C++ is a leaky abstraction .

User avatar
kaksi
Posts: 135
Joined: Tue Mar 10, 2015 6:19 am

Re: Help my code to look better

Sun Jun 07, 2020 7:47 pm

It seems to me that line_number is not used.

jahboater
Posts: 5927
Joined: Wed Feb 04, 2015 6:38 pm
Location: West Dorset

Re: Help my code to look better

Sun Jun 07, 2020 7:58 pm

Heater wrote:
Sun Jun 07, 2020 7:43 pm
I find the ugliest part being variable names like "splited_uncommented_line". There must be something shorter and sweeter. Like "words" or "fields"
+1
Heater wrote:
Sun Jun 07, 2020 7:43 pm
I don't know if it's required by some Python documentation standard but I would move the comment block at the top to outside the function so as to not break up the code.
I think a docstring must be just after the function header.
Pi4 8GB running PIOS64 Lite

cmrincon
Posts: 44
Joined: Thu May 24, 2018 7:39 pm

Re: Help my code to look better

Sun Jun 07, 2020 8:32 pm

Thanks you!
Heater wrote: I don't know if it's required by some Python documentation standard but I would move the comment block at the top to outside the function so as to not break up the code.
https://google.github.io/styleguide/pyguide.html Chapter 3.8.3 . Docstring requires it.
Heater wrote: It's a trick question.
It's very difficult for me write a nice code. My code NEVER looks like google git code. The most difficult thing is think a name that describes better a function/attribute/parameter/etc. Also make my code looks nice at first glance is a difficult task.
Heater wrote: Before trying to make it look nice I would make it at least work. That function returns a non-existent variable spellsDict.
Oopsy :oops:
kaksi wrote:It seems to me that line_number is not used.
It's after

Code: Select all

for line in config_file:

User avatar
scruss
Posts: 3322
Joined: Sat Jun 09, 2012 12:25 pm
Location: Toronto, ON
Contact: Website

Re: Help my code to look better

Sun Jun 07, 2020 9:45 pm

Working or not, here's what autopep8 makes of it:

Code: Select all

def read_config_file(config_file_path):
    """Reads and parse a config file.

Args:
        config_file_path: path to the file to parse.

Returns:
        A dictionary with the mapped settings.
"""

    spells_dictionary = {}
    line_number = 0
    with open(config_file_path, 'r') as config_file:
        for line in config_file:
            line_number += 1
            uncommented_line = line.strip().split('#')
            if len(uncommented_line[0]) == 0:
                continue
            splited_uncommented_line = [x.strip()
                                        for x in uncommented_line[0].split(',')]
            if len(splited_uncommented_line) != 2:
                print("ERROR: incorrect line format".format(cfgPath))
                continue
            try:
                spell_id = int(splited_uncommented_line[1])
                # Splited_uncommented_line[0] = spell Name
                spells_dictionary[spell_id] = splited_uncommented_line[0]
            except valueError as e:
                print("ERROR: incorrect line format".format(cfgPath))

    return spellsDict
Python is not a language noted for its beauty, but in line with its "there can only be one way to do it" philosophy, autopep8 applies PEP 8. You can install it with sudo apt install python3-autopep8
‘Remember the Golden Rule of Selling: “Do not resort to violence.”’ — McGlashan.
Pronouns: he/him

User avatar
Paeryn
Posts: 3008
Joined: Wed Nov 23, 2011 1:10 am
Location: Sheffield, England

Re: Help my code to look better

Sun Jun 07, 2020 10:25 pm

cmrincon wrote:
Sun Jun 07, 2020 8:32 pm
kaksi wrote:It seems to me that line_number is not used.
It's after

Code: Select all

for line in config_file:
The only line that uses line_number is, as you said, that single line that increments it. So whilst technically it is used because the increment operator uses it, it isn't used for anything and the function would produce the exact same result if you removed the variable and those two lines.
She who travels light — forgot something.

cmrincon
Posts: 44
Joined: Thu May 24, 2018 7:39 pm

Re: Help my code to look better

Mon Jun 08, 2020 8:30 am

Here is how my code looks after your tips:

Code: Select all

def read_config_file(config_file_path):
    """Reads and parse a config file.

Args:
        config_file_path: path to the file to parse.

Returns:
        A dictionary with the mapped settings.
"""

    spells_dictionary = {}
    line_number = 0

    with open(config_file_path, 'r') as config_file:
        for line in config_file:
            line_number += 1
            uncommented_line = line.strip().split('#')
            
            if len(uncommented_line[0]) == 0:  
                continue
            
            fields =  [x.strip() for x in uncommented_line[0].split(',')]
            
            if len(fields) != 2:
                print("ERROR: incorrect line format. Line {} of file {}".format(line_number, config_file_path))
                continue
            
            try:
                spell_id = int(fields[1])
                spells_dictionary[spell_id] = fields[0]  #fields[0] is the spell name
            
            except valueError as e:
                 print("ERROR: incorrect line format. Line {} of file {}".format(line_number, config_file_path))
                 continue

    return spells_dictionary
        
Now the code looks better!

I think that the function is too long and can be splitted in two diferent functions: read_config_file() and parse_line(), that will cause i would need to raise a few exceptions in parse_line, and catch them in read_config_file() in order to print the error and jump to the next line.

All the errors are not managed the same way, so i would need 3 different exceptions: ValueError, and 2 custom exceptions. The good thing is that ValueError and 1 of my custom exceptions would have the same except block. The bad thing is that all this exceptions will make my code more complex.

Will this make my code easier to read?

User avatar
rpiMike
Posts: 1432
Joined: Fri Aug 10, 2012 12:38 pm
Location: Cumbria, UK

Re: Help my code to look better

Mon Jun 08, 2020 8:50 am

Personally I don’t like underscores in variable names or function names:

config_file_path -> configFilePath

User avatar
bensimmo
Posts: 4712
Joined: Sun Dec 28, 2014 3:02 pm
Location: East Yorkshire

Re: Help my code to look better

Mon Jun 08, 2020 9:12 am

rpiMike wrote:
Mon Jun 08, 2020 8:50 am
Personally I don’t like underscores in variable names or function names:

config_file_path -> configFilePath
The Python style guide is to use underscores for functions and variables, so they should work to that.

https://www.python.org/dev/peps/pep-000 ... able-names

jahboater
Posts: 5927
Joined: Wed Feb 04, 2015 6:38 pm
Location: West Dorset

Re: Help my code to look better

Mon Jun 08, 2020 9:32 am

bensimmo wrote:
Mon Jun 08, 2020 9:12 am
rpiMike wrote:
Mon Jun 08, 2020 8:50 am
Personally I don’t like underscores in variable names or function names:

config_file_path -> configFilePath
The Python style guide is to use underscores for functions and variables, so they should work to that.

https://www.python.org/dev/peps/pep-000 ... able-names
Yes.
In English there are spaces between words, and capital letters are only used for proper nouns and the start of sentences.
The Python standard is the closer approximation.
Camel case might be slightly shorter, but readability comes first.
Pi4 8GB running PIOS64 Lite

jamesh
Raspberry Pi Engineer & Forum Moderator
Raspberry Pi Engineer & Forum Moderator
Posts: 26833
Joined: Sat Jul 30, 2011 7:41 pm

Re: Help my code to look better

Mon Jun 08, 2020 11:30 am

I prefer camel case as well, just as readable in my opinion.

But the best code is the stuff that the person maintaining the code can read easily. I cannot read Linux kernel code easily, because the coding standard doesn't match how I think of code. Others have no problem.

Not easy!
Principal Software Engineer at Raspberry Pi (Trading) Ltd.
Contrary to popular belief, humorous signatures are allowed.
I've been saying "Mucho" to my Spanish friend a lot more lately. It means a lot to him.

Return to “Python”