[CentralOH] Help with learning how to use a class (please don't laugh :) )

[Sam Corder]

I'd suggest not having two different return types coming back from a method.  You could either raise an exception for an invalid drive or return a value like -1.  My personal preference is for exceptions but there are varying schools of thoughts when you should return error codes and when you should raise exceptions.  

    def GetTotalDiskSpace(self, drive):
        self.drive = drive
        for disk in self.server.Win32_LogicalDisk():
            if disk.Name == drive:
                return = long(disk.Size) /1073741824.0
                
        return -1

Or with exceptions:

    def GetTotalDiskSpace(self, drive):
        self.drive = drive
        for disk in self.server.Win32_LogicalDisk():
            if disk.Name == drive:
                return = long(disk.Size) /1073741824.0
                
        raise ValueError('Invalid Drive')

A couple other pointers.  You don't even need a total variable here since your if statement will only execute once.  You don't need to store drive in an instance variable unless you are going to use it in another method that doesn't receive the drive parameter.  In that case you could end up with some weird behavior if you don't call the different methods in exactly the right order.

-Sam

----- Original Message -----
From: Eric Lake <ericlake at gmail.com>
To: Brian Costlow <brian.costlow at gmail.com>
Cc: centraloh at python.org
Sent: Wednesday, July 9, 2008 2:04:22 PM GMT-0500 Auto-Detected
Subject: Re: [CentralOH] Help with learning how to use a class (please don't laugh :) )

Good point. I think that doing the following would catch that. I'm
still very new to the OOP stuff so I'm not sure how to go about
refactoring it.

    def GetTotalDiskSpace(self, drive):
        self.drive = drive
        try:
            for disk in self.server.Win32_LogicalDisk():
                if disk.Name == self.drive:
                    total = long(disk.Size) /1073741824.0
            return total
        except UnboundLocalError:
            return 'Drive Not Found'

    def GetFreeDiskSpace(self, drive):
        self.drive = drive
        try:
            for disk in self.server.Win32_LogicalDisk():
                if disk.Name == self.drive:
                    free = long(disk.FreeSpace) /1073741824.0
            return total
        except UnboundLocalError:
            return 'Drive Not Found'

On Wed, Jul 9, 2008 at 1:56 PM, Brian Costlow <brian.costlow at gmail.com> wrote:
> You have a point.
>
> In the original code, the class definition was inline with the script
> calling it, and every time you instantiated the object, you then called
> methods that
> called Win32_LogicalDisk.
>
> If you're going to use the class such that sometimes the calling code does
>
> foo = SysInfo(somehost)
> foo.GetCpuList()
>
> and never uses the disk info methods, then my suggestion is 'doing work'
> that you don't need to do.
>
> On the other hand, you still have code that repeats itself, that you should
> think about refactoring in some way.
>
> I didn't run this (my windows box isn't handy) but GetTotalDiskSpace and
> GetFreeDiskSpace look like they will die if you feed it a non-existing
> drive, since you will end up returning a name you haven't defined.
>
>
>

-- 
Thanks,

Eric Lake
_______________________________________________
CentralOH mailing list
CentralOH at python.org
http://mail.python.org/mailman/listinfo/centraloh