0
\$\begingroup\$

I am working on my first open source project, and I have read a lot to get the desire coding style and I think I got it. But before go further I want to get reviews about my code. This script is not finished but I want to know if I'm on the right path.

# -*- coding: utf-8 -*-
"""config.py script, this file is part of BSR(Bolivian street racer) game."""
#################################################################
#
# Copyright (C) 2017  Pablo Cesar Chavez Suarez
#
# Autor     : Pablo Cesar Chavez Suarez
#
# Email     : pacehasu@gmail.com
#
# License   : GLPv3
#
# year      : 2017
#
# version   : Blender 2.78
#
# If you use this code, please include this information header.
#
# This file is part of Bolivian street racer from now on called BSR.
#
# BSR is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# BSR is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with BSR.  If not, see <http://www.gnu.org/licenses/>.
#
#################################################################
    from configparser import ConfigParser
    from os.path import dirname, abspath, isfile


    class Config():
        """This class read and write config.ini file."""
        def __init__(self):
            """While initializing checks whether the configuration file exists."""
            install_path = self.readInstallPathFromFile
            pass

        @property
        def installPath(self):
            """Returns the full path to the root directory of the game."""
            return '{}\\'.format(dirname(dirname(abspath(__file__))))

        @property
        def configFilePath(self):
            """Retursn the full path of the configuration file."""
            return '{}'.format(self.installPath + 'config.ini')

        def createEmptyConfigFile(self):
            """Creates the empty configuration file."""
            with open(self.configFilePath,'w', encoding="utf-8") as file:
                #file.closed
                return
        def addFileSection(self, section, path = ''):
            """Adds sections to the configuration file"""
            conf = ConfigParser()
            conf.readfp(open(self.configFilePath, encoding="utf-8"))
            conf.add_section(section)
            conf.set(section, 'path', path)
            with open(self.configFilePath,'w', encoding="utf-8") as file:
                conf.write(file)

        @property
        def readInstallPathFromFile(self):
            """Reads and returns the full path of the root directory
            from the configuration file if it exists,
            otherwise creates and writes the path to then return it."""
            conf = ConfigParser()
            if isfile(self.configFilePath):
                conf.readfp(open(self.configFilePath, encoding="utf-8"))
                if conf.has_section('install'):
                    return '{}'.format(conf.get('install','path'))
                else:
                    self.addFileSection('install')
                    return '{}'.format(conf.get('install','path'))
            else:
                self.createEmptyConfigFile()
                self.addFileSection('install', self.configFilePath)
                conf.readfp(open(self.configFilePath, encoding="utf-8"))
                return '{}'.format(conf.get('install','path'))

        @property
        def scenesPath(self):
            """Returns the scenes full Path"""
            conf = ConfigParser()
            conf.readfp(open(self.configFilePath, encoding="utf-8"))
            if conf.has_section('scenes'):
                return '{}'.format(conf.get('scenes','path'))
            else:
                self.addFileSection('scenes', self.installPath + 'scenes\\')
                conf.readfp(open(self.configFilePath, encoding="utf-8"))
                return '{}'.format(conf.get('scenes','path'))

        @property
        def vehiclesPath(self):
            """Returns the vehicles full Path"""
            conf = ConfigParser()
            conf.readfp(open(self.configFilePath, encoding="utf-8"))
            if conf.has_section('vehicles'):
                return '{}'.format(conf.get('vehicles','path'))
            else:
                self.addFileSection('vehicles', self.installPath + 'vehicles\\')
                conf.readfp(open(self.configFilePath, encoding="utf-8"))
                return '{}'.format(conf.get('vehicles','path'))

        @property
        def tracksPath(self):
            """Returns the tracks full Path"""
            conf = ConfigParser()
            conf.readfp(open(self.configFilePath, encoding="utf-8"))
            if conf.has_section('tracks'):
                return '{}'.format(conf.get('tracks','path'))
            else:
                self.addFileSection('tracks', self.installPath + 'tracks\\')
                conf.readfp(open(self.configFilePath, encoding="utf-8"))
                return '{}'.format(conf.get('tracks','path'))
            pass

    #For purposes of testing
    config = Config()
    print(config.installPath)
    print(config.scenesPath)
    print(config.vehiclesPath)
    print(config.tracksPath)
#output
#c:\game\bsr\
#c:\game\bsr\scenes\
\$\endgroup\$
  • \$\begingroup\$ Welcome to StackExchange Code Review! On this site we review working code. Suggest you take a look at these Guidelines \$\endgroup\$ – Stephen Rauch Mar 9 '17 at 3:32
  • \$\begingroup\$ @StephenRauch this code its fully working, only returns the root path but it working. I'm asking if best practices and design pattern usage its on my script. this not contain purely generic, hypothetical code. \$\endgroup\$ – Strapicarus Mar 9 '17 at 3:52
  • \$\begingroup\$ what does file.closed do? \$\endgroup\$ – Stephen Rauch Mar 9 '17 at 3:54
  • \$\begingroup\$ Closes the file supposedly. i open a file then i closes it. ucan found that in the python documentation \$\endgroup\$ – Strapicarus Mar 9 '17 at 3:57
  • \$\begingroup\$ Your question about file.closed show me that im wrong. file.closed returns true if the file it's closed. Thank you! \$\endgroup\$ – Strapicarus Mar 9 '17 at 4:07
1
\$\begingroup\$

In no particular order:

  1. Your code is unnecessarily indented 4 spaces; start everything top-level at the beginning of the lines.
  2. One newline between methods within a class.
  3. You need no pass at the end of __init__ or tracksPath and possibly elsewhere.
  4. "snake_case", rather than "camelCase", is the norm in Python. See PEP8.
  5. file is a built-in and should not be used as a local variable, like in your with statements.
  6. My personal feeling is that properties should not do a lot of "work", leaving that for regular methods. If you feel compelled to put a verb like "read" in the name, it's probably doing too much "work" for a property. (I admit that I am sometimes the worst offender of this rule.)
  7. I'm not crazy about function/method names being overly-complete sentences, like readInstallPathFromFile; I would probably go with one of install_path, read_install_path or install_path_from_file, depending on any distinctions I was trying to make (are there more places to get the install path than a file?), my mood and phase of the moon.
  8. Use os.path.join to join path components, like you've done in configFilePath.
  9. Don't use str.format needlessly, like you've done in configFilePath -- you string-concatenate the base path with the file name and then format them as themselves. And in a lot of other places--any place with '{}'.format(...). If you're doing that to ensure you've got a string instead of an integer or something, wrap it with str().
  10. You probably don't need the extra formatting in installPath if you use os.path.join; you're adding the trailing backslash so your string-concatenation in configFilePath works.
  11. I don't like the repetitious file opening & parsing. Read your config and keep it in an instance variable. (Unless this is a special case like a huge file or something that could be rewritten without you knowing it.)
  12. I also don't like all the schema construction upon read -- add all the sections when you create the empty file or when you first open the file. Do it by iterating over a list of sections, so you have the section list in one canonical location for reference. (NB: There might be things about ConfigParser that make this not feasible; I've used it and remember it was fiddly in some respects but it's been a few years.)
  13. Run flake8 on your code and fix what it does not like.
\$\endgroup\$
  • \$\begingroup\$ I did not notice the indentation, but it was generated when I copy the code to the question and I will try to improve the code with your suggestions, thanks. \$\endgroup\$ – Strapicarus Mar 10 '17 at 20:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.