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

[Brian Costlow]

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.



On Wed, Jul 9, 2008 at 12:27 PM, Eric Lake <ericlake at gmail.com> wrote:

> Using:
>
>        self.disks = [disk for disk in self.server.Win32_LogicalDisk()]
>         self.localDisks = [ disk for disk in self.disks if disk.DriveType
> == 3]
>
> would then get the disk information every time I call the class though
> wouldn't it? What I am trying to write is a general purpose class that
> I can use to get all kinds of information from my servers. I have
> removed the input and output file stuff. here is the class as it
> stands currently:
>
> import string
> import wmi
>
> class SysInfo(object):
>    def __init__(self, hostname):
>        self.host = hostname
>         self.server = wmi.WMI(self.host)
>
>    def GetLocalDrives(self):
>        driveList = []
>        for disk in self.server.Win32_LogicalDisk():
>            if disk.DriveType == 3:
>                 driveList.append(str(disk.Name))
>        return driveList
>
>     def GetTotalDiskSpace(self, drive):
>        self.drive = drive
>        for disk in self.server.Win32_LogicalDisk():
>            if disk.Name == self.drive:
>                total = long(disk.Size) /1073741824.0
>        return total
>
>    def GetFreeDiskSpace(self, drive):
>        self.drive = drive
>        for disk in self.server.Win32_LogicalDisk():
>            if disk.Name == self.drive:
>                free = long(disk.FreeSpace) /1073741824.0
>        return free
>
>    def GetUsedDiskSpace(self, drive):
>        self.drive = drive
>         total = self.GetTotalDiskSpace(self.drive)
>        free = self.GetFreeDiskSpace(self.drive)
>         used = (total - free)
>        return used
>
>     def GetCpuList(self):
>        cpulist = []
>        cpudict = {}
>        for cpu in self.server.Win32_Processor():
>            name = string.strip(str(cpu.Name))
>            deviceid = str(cpu.DeviceID)
>            cpudict = {deviceid:name}
>            cpulist.append(cpudict)
>        return cpulist
>
>    def GetNumCpu(self):
>        cpus = self.GetCpuList()
>        return len(cpus)
>
>    def GetNodeName(self):
>        # This is for when you are looking at a clustered env and
>        # want to know who the active node is.
>        for os in self.server.Win32_OperatingSystem():
>            activeNode = os.CSName
>        return str(activeNode)
>
>
> On Wed, Jul 9, 2008 at 12:06 PM, Brian Costlow <brian.costlow at gmail.com>
> wrote:
> > Eric,
> >
> > A couple of other things I noticed.
> >
> > You use one of these two constructs in all the methods.
> >
> > for disk in server.Win32_LogicalDisk():
> >
> > for disk in server.Win32_LogicalDisk():
> >     if disk.DriveType == 3:
> >
> > Since it doesn't seem these would change for this script between the
> object
> > creation and calling the methods, I'd refactor them into the init as
> well.
> >
> > class SysInfo(object):
> >     def __init__(self, hostname):
> >         self.host = hostname
> >         server = wmi.WMI(self.host)
> >         self.disks = [disk for disk in server.Win32_LogicalDisk()]
> >         self.localDisks [ disk for disk in self.disks if disk.DriveType
> ==
> > 3]
> >
> > Then you can iterate the lists in your methods.
> >
> >     def GetStorage(self):
> >         for disk in self.localDisks:
> >
> > Also, it won't matter much in a one-pager, but if you were doing anything
> > bigger, this might bite you. It looks like you're creating a global
> variable
> > for the outfile, then referring to that implicitly from inside an object
> > method. Typically, Id try to open/close that resource inside the method
> that
> > uses it. If it had to come from outside, Id pass it explicitly just so
> the
> > code was clearer.
> >
>
>
>
> --
> Thanks,
>
> Eric Lake
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/mailman/private/centraloh/attachments/20080709/0c5ed22f/attachment.htm>