2
\$\begingroup\$

How can I improve this?

GitHub

import os, platform, logging, logging.handlers, glob
from time import *
import tarfile
import zipfile, zlib

class backup:
    source_s = ''
    destination_d = ''

    def __init__(self):

        if strftime("%p") == 'AM':
            if strftime("%I") in ['12', '01', '02', '03', '04', '05', '06', '07', '08', '09', '10', '11']:
                time_d = 'Morning'
            else:
                pass 
        if strftime("%p") == 'PM':
            if strftime("%I") in ['12', '01', '02', '03']:
                time_d = 'Afternoon'
            elif strftime("%I") in ['04', '05', '06', '07', '08']:
                time_d = 'Evening'
            elif strftime("%I") in ['09', '10', '11']:
                time_d = 'Night'
            else:
                pass 

        print "Date:",strftime("%d %b %Y")
        print "Hi There, Today is ",strftime("%A")," And time is ", strftime("%I:%M:%S:%P"),  ", So on this beautiful ",strftime("%A"),"", time_d," I welcome you to this Backup program."


    def source_destination(self):
        w_s = True
        w_ss = True
        while w_s:
            source = raw_input('Please Enter The Complete Path of Source Directory: ')
            if source == '':
                print "***You have not Specified Anything***"
                print "***Source Directory is required in Order to make backup***"
                continue
            elif os.path.exists(source) == False:
                print "***Source Path Does not Exists***"
                print "***Please check source path and Try Again***"
                continue
            else:
                print "***Specified source path is ACCEPTED***"
                w_s = False
                backup.source_s = source



        while w_ss:
            destination = raw_input('Please Enter Complete Path of Destination Directory:')
            destination = destination.replace(' ','_')

            if destination == '':
                print "***You have not Specified Anything***"
                print "***Destination Directory is required to store the backup files***"

                continue
            elif os.path.exists(destination) == False:
                print "***Destination Path Does not Exists***"
                decision = raw_input('Do you Want me to Create Destination For you (yes or no or just hit enter for yes):')
                if decision == 'yes':
                    print "***Destination Path Will now be created***"
                    os.mkdir(destination)
                    print "***Directory has been created now***"
                    print "***Program will now continue***"
                    backup.destination_d = destination
                    w_ss = False
                elif decision == 'no':
                    print "***As You Wish, because Your wish is my Command***"
                    print "***Program will now Terminate***"
                    print "Bye !!!"
                    exit()
                elif decision == '':
                    print "***Destination Path Will now be created***"
                    mk_d = 'mkdir {0}'.format(destination)
                    os.system(mk_d)
                    print "***Directory has been created now***"
                    print "***Program will now continue***"
                    backup.destination_d = destination
                    w_ss = False
                elif decision not in ['yes','no','']:
                    print "***What you think you are doing, you just have to choose yes or no, is it that HARD?***"
                    print "***Try again now***"
                    continue
            elif os.path.exists(destination) == True:
                if os.path.isdir(destination)== False:
                    print "***Specified location is a file and not a directory, so try again and enter path of a directory***"
                    continue
                else:
                    print "***Specified destination path is ACCEPTED***"
                    w_ss = False
                    backup.destination_d = destination

            else:
                print "***Specified destination path is ACCEPTED***"
                w_ss = False
                backup.destination_d = destination

    def compress(self):
        source = backup.source_s
        destination = backup.destination_d

        w_sss = True
        f_name = raw_input('Please Enter The Desired name for output file(without extension):')

        if f_name == '':
            print "***You have not specified any name so DEFAULT will be used.(i.e 'untitled')***"
            f_name = 'untitled'
        else:
            pass

        while w_sss:
            c_method = raw_input('How you want your backup file to be compressed ?(tar, tar.gz, tar.bz2, or zip):') 

            if c_method == 'tar':
                ff_name = f_name.replace(' ', '_') + '.tar'
                w_sss = False
            elif c_method == 'tar.gz':
                ff_name = f_name.replace(' ', '_') + '.tar.gz'
                w_sss = False
            elif c_method == 'tar.bz2':
                ff_name = f_name.replace(' ', '_') + '.tar.bz2'
                w_sss = False
            elif c_method == 'zip':
                ff_name = f_name.replace(' ', '_') + '.zip'

                w_sss = False

            elif c_method == '':
                print "***You have not selected any method of compression***"
                print "***Please select atleast one method of compression***"

                continue
            else:
                print "***Sorry, The method you specified is not supported yet***"
                print "Please choose from the given options i.e tar, tar.gz, tar.bz2 or zip "

                continue

        suffix = ("/")
        if source.endswith(suffix) == True:
            pass 
        else:
            source = source + os.sep
        if destination.endswith(suffix) == True:
            pass 
        else:
            destination = destination + os.sep

        values = [source, destination, ff_name]

        if c_method == 'tar':
            print "***Compression can take sometime depending upon method you selected and the size of the source, so please be patient***"
            tar = tarfile.open(destination+ff_name, 'w')
            for item in os.listdir(source):
                print "Adding",item,"to archive"
                tar.add(os.path.join(source,item))


            tar.close()
            print "Operation successful"
            exit()
        elif c_method == 'tar.gz':
            print "***Compression can take sometime depending upon method you selected and the size of the source, so please be patient***"
            tar = tarfile.open(destination+ff_name, 'w:gz')
            for item in os.listdir(source):
                print "Adding",item,"to archive"
                tar.add(os.path.join(source,item))


            tar.close()
            print "Operation successful"
            exit()
        elif c_method == 'tar.bz2':
            print "***Compression can take sometime depending upon method you selected and the size of the source, so please be patient***"
            tar = tarfile.open(destination+ff_name, 'w:bz2')
            for item in os.listdir(source):
                print "Adding",item,"to archive"
                tar.add(os.path.join(source,item))

            tar.close()
            print "Operation successful"
            exit()
        else:
            print "***Compression can take sometime depending upon method you selected and the size of the source, so please be patient***"
            zipf = zipfile.ZipFile(destination+ff_name,"w", compression=zipfile.ZIP_DEFLATED)
            def recursive_zip(a, b):
                for item in os.listdir(b):
                    if os.path.isfile(os.path.join(b, item)):
                        print "Adding",item,"to archive"
                        a.write(os.path.join(b, item))
                    elif os.path.isdir(os.path.join(b, item)):
                        recursive_zip(a, os.path.join(b, item))



            recursive_zip(zipf, source)
            zipf.close()
            print "Operation successful"
            exit()




try:
    p = backup()
    p.source_destination()
    p.compress()
except KeyboardInterrupt:
    print "Why are you leaving me "
    reason = raw_input("1. Your program is not good enough 2. I will be back (1 or 2):")
    if(reason == '1'):
        print "Thanks for using my program, I will try to Improve it, so till then, Good Bye !"
        exit();
    elif(reason == '2'):
        print "OK then, See you Soon"
        exit()
    else:
        print "Invalid Input !!!"
        exit()

except EOFError:
    print "Why are you leaving me "
    reason = raw_input("1. Your program is not good enough 2. I will be back (1 or 2):")
    if(reason == '1'):
        print "Thanks for using my program, I will try to Improve it, so till then, Good Bye !"
        exit();
    elif(reason == '2'):
        print "OK then, See you Soon"
        exit()
    else:
        print "Invalid Input !!!"
        exit()
\$\endgroup\$
6
\$\begingroup\$
import os, platform, logging, logging.handlers, glob
from time import *
import tarfile
import zipfile, zlib

class backup:

The python style guide recommends CamelCase for class names

    source_s = ''
    destination_d = ''

Why are these class attributes rather then instance attributes?

    def __init__(self):

        if strftime("%p") == 'AM':
            if strftime("%I") in ['12', '01', '02', '03', '04', '05', '06', '07', '08', '09', '10', '11']:
                time_d = 'Morning'

I recommend not using _d in your variable name, its not clear what it means. Name your variables with something meaningful.

            else:
                pass 
        if strftime("%p") == 'PM':
            if strftime("%I") in ['12', '01', '02', '03']:
                time_d = 'Afternoon'
            elif strftime("%I") in ['04', '05', '06', '07', '08']:
                time_d = 'Evening'
            elif strftime("%I") in ['09', '10', '11']:
                time_d = 'Night'
            else:
                pass 

Don't convert the time to a string, and then test it, it'll be easier if you get the time as numbers

current_time = time.localtime()
if 0 <= current_time.tm_hour < 12:
   time_d = 'Morning'

For a self-contained unit this size, I'd move it into a seperate function that returns time_d

        print "Date:",strftime("%d %b %Y")
        print "Hi There, Today is ",strftime("%A")," And time is ", strftime("%I:%M:%S:%P"),  ", So on this beautiful ",strftime("%A"),"", time_d," I welcome you to this Backup program."


    def source_destination(self):
        w_s = True
        w_ss = True

Bad variables names, they are pretty inscrutable

        while w_s:
            source = raw_input('Please Enter The Complete Path of Source Directory: ')
            if source == '':
                print "***You have not Specified Anything***"
                print "***Source Directory is required in Order to make backup***"
                continue

This continue is pointless because the rest of the code is an else block, get rid of this.

            elif os.path.exists(source) == False:
                print "***Source Path Does not Exists***"
                print "***Please check source path and Try Again***"
                continue

Same here

            else:
                print "***Specified source path is ACCEPTED***"
                w_s = False

Use break, somebody may have told you not to use break, they were wrong. Break can be bad for readability, but setting boolean flags is worse.

                backup.source_s = source

Don't start data on your class, store it on self.

        while w_ss:

You set w_ss way above, don't do that. set variables close to their use

            destination = raw_input('Please Enter Complete Path of Destination Directory:')
            destination = destination.replace(' ','_')

Magically replacing parts of the path is suspicous

            if destination == '':
                print "***You have not Specified Anything***"
                print "***Destination Directory is required to store the backup files***"

                continue
            elif os.path.exists(destination) == False:
                print "***Destination Path Does not Exists***"
                decision = raw_input('Do you Want me to Create Destination For you (yes or no or just hit enter for yes):')
                if decision == 'yes':

use if decision == 'yes' or decision == '': to avoid duplicating this

                    print "***Destination Path Will now be created***"
                    os.mkdir(destination)
                    print "***Directory has been created now***"
                    print "***Program will now continue***"

Your script is overly printy. It says a lot more then it does.

                    backup.destination_d = destination
                    w_ss = False

as before, boolean flags are delayed gotos. Don't use them unless you must. Use break.

                elif decision == 'no':
                    print "***As You Wish, because Your wish is my Command***"
                    print "***Program will now Terminate***"
                    print "Bye !!!"
                    exit()

Doing this is generally not considered good form. There is nothing terribly wrong with it, but generally you should terminate your program by returning back to the main loop.

                elif decision == '':
                    print "***Destination Path Will now be created***"
                    mk_d = 'mkdir {0}'.format(destination)
                    os.system(mk_d)

Why are you using system to make a directory? Use the os.mkdir function as you did above

                    print "***Directory has been created now***"
                    print "***Program will now continue***"
                    backup.destination_d = destination
                    w_ss = False
                elif decision not in ['yes','no','']:
                    print "***What you think you are doing, you just have to choose yes or no, is it that HARD?***"
                    print "***Try again now***"
                    continue

Don't insult your users

            elif os.path.exists(destination) == True:

Don't use == True just use if os.path.exists(destination): if os.path.isdir(destination)== False:

Same with False. use if not os.path.isdir(destination)

                    print "***Specified location is a file and not a directory, so try again and enter path of a directory***"
                    continue
                else:
                    print "***Specified destination path is ACCEPTED***"
                    w_ss = False
                    backup.destination_d = destination

            else:

Can this ever happen? You should either find yourself in the destination exists or destination does not exist category

                print "***Specified destination path is ACCEPTED***"
                w_ss = False
                backup.destination_d = destination

This function is too long. You should break it up. I'd want to make this function something like:

   def backup_destination(self):
       while True:
            destination = raw_input()
            if validate_destination(destination):
                self.destination = destination
                return 

See how short it is and how easy it is to see what its doing. All the details are tucked away in other functions.

    def compress(self):
        source = backup.source_s
        destination = backup.destination_d

        w_sss = True

You do realize you can reuse the same variable name across different functions don't you?

        f_name = raw_input('Please Enter The Desired name for output file(without extension):')

        if f_name == '':
            print "***You have not specified any name so DEFAULT will be used.(i.e 'untitled')***"
            f_name = 'untitled'
        else:
            pass

No need for an en empty else clause

        while w_sss:
            c_method = raw_input('How you want your backup file to be compressed ?(tar, tar.gz, tar.bz2, or zip):') 

            if c_method == 'tar':
                ff_name = f_name.replace(' ', '_') + '.tar'
                w_sss = False
            elif c_method == 'tar.gz':
                ff_name = f_name.replace(' ', '_') + '.tar.gz'
                w_sss = False
            elif c_method == 'tar.bz2':
                ff_name = f_name.replace(' ', '_') + '.tar.bz2'
                w_sss = False
            elif c_method == 'zip':
                ff_name = f_name.replace(' ', '_') + '.zip'

                w_sss = False

instead use

    if c_method in ('tar','tar.gz','tar.bz2','zip'):
         ff_name = f_name.replace(' ','_') + '.' + c_method
         break

That reduces duplication

            elif c_method == '':
                print "***You have not selected any method of compression***"
                print "***Please select atleast one method of compression***"

                continue
            else:
                print "***Sorry, The method you specified is not supported yet***"
                print "Please choose from the given options i.e tar, tar.gz, tar.bz2 or zip "

                continue

        suffix = ("/")

The parens do nothing

        if source.endswith(suffix) == True:
            pass 
        else:
            source = source + os.sep

Why are you using "/" above and os.sep here? Use os.sep everywhere

        if destination.endswith(suffix) == True:
            pass

Don't have empty if blocks, invert the logic of the if statement else: destination = destination + os.sep

You do the same thing to destination and source, write a function to do it

        values = [source, destination, ff_name]

Unused variable, delete it

        if c_method == 'tar':
            print "***Compression can take sometime depending upon method you selected and the size of the source, so please be patient***"

You have the same print in every if block, copy it outside of the if block

            tar = tarfile.open(destination+ff_name, 'w')

use os.path.join to combine pieces of a path. It will take care of the os.sep you were dealing with earlier

            for item in os.listdir(source):
                print "Adding",item,"to archive"
                tar.add(os.path.join(source,item))


            tar.close()
            print "Operation successful"
            exit()

again, exiting midscript is considered bad form. (in most cases)

        elif c_method == 'tar.gz':
            print "***Compression can take sometime depending upon method you selected and the size of the source, so please be patient***"
            tar = tarfile.open(destination+ff_name, 'w:gz')
            for item in os.listdir(source):
                print "Adding",item,"to archive"
                tar.add(os.path.join(source,item))


            tar.close()
            print "Operation successful"
            exit()

See how this is exactly the same as the first case, but with a different mode, write a function that takes the mode as a parameter

        elif c_method == 'tar.bz2':
            print "***Compression can take sometime depending upon method you selected and the size of the source, so please be patient***"
            tar = tarfile.open(destination+ff_name, 'w:bz2')
            for item in os.listdir(source):
                print "Adding",item,"to archive"
                tar.add(os.path.join(source,item))

            tar.close()
            print "Operation successful"
            exit()

And again!

        else:
            print "***Compression can take sometime depending upon method you selected and the size of the source, so please be patient***"
            zipf = zipfile.ZipFile(destination+ff_name,"w", compression=zipfile.ZIP_DEFLATED)
            def recursive_zip(a, b):
                for item in os.listdir(b):
                    if os.path.isfile(os.path.join(b, item)):
                        print "Adding",item,"to archive"
                        a.write(os.path.join(b, item))
                    elif os.path.isdir(os.path.join(b, item)):
                        recursive_zip(a, os.path.join(b, item))



            recursive_zip(zipf, source)

Why are you supporting recursive for zip, but nobody else?

            zipf.close()
            print "Operation successful"
            exit()

Zip is different, but similiar to the others. I'd make it so they share code, but that's ab it trickier.

try:
    p = backup()

Don't use one letter variable names, unless the abbreviation is really common

    p.source_destination()
    p.compress()
except KeyboardInterrupt:
    print "Why are you leaving me "
    reason = raw_input("1. Your program is not good enough 2. I will be back (1 or 2):")
    if(reason == '1'):
        print "Thanks for using my program, I will try to Improve it, so till then, Good Bye !"
        exit();
    elif(reason == '2'):
        print "OK then, See you Soon"
        exit()
    else:
        print "Invalid Input !!!"
        exit()

except EOFError:
    print "Why are you leaving me "
    reason = raw_input("1. Your program is not good enough 2. I will be back (1 or 2):")
    if(reason == '1'):
        print "Thanks for using my program, I will try to Improve it, so till then, Good Bye !"
        exit();
    elif(reason == '2'):
        print "OK then, See you Soon"
        exit()
    else:
        print "Invalid Input !!!"
        exit()

use except (KeyboardInterrupt, EOFError): to catch multiple exceptions

Generally

  1. You have a lot of useless continue statements that don't do anything
  2. You use boolean flag variables, you should use break
  3. Your variable names are cryptic
  4. You duplicate a lot of logic. You should strive to have each piece of logic once
  5. Your functions tend to be long and complicated, they should be broken up into smaller functions
  6. Your compare boolean values with == True and == False DON'T!
\$\endgroup\$
  • \$\begingroup\$ Thanks man, I am gonna rewrite this script by following your suggestions. Can you recommend me some good book/tutorial/paper so that I can be a good programmer? \$\endgroup\$ – hsinxh Nov 11 '11 at 16:02
  • \$\begingroup\$ @Harbhag: I'd suggest Dive Into Python. Reading the Standard Library sources can be helpful as well. \$\endgroup\$ – André Paramés Jan 31 '13 at 19:05

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.