Kritik an meinem ersten kleinen Pyth/Wxpyth prog erwünscht

Wenn du dir nicht sicher bist, in welchem der anderen Foren du die Frage stellen sollst, dann bist du hier im Forum für allgemeine Fragen sicher richtig.
Antworten
FrancescoS

Hallo,

ich möchte mich gleich anhängen in einer kritischen Code-Begutachtung.
Ich bin ebenfalls ein Neueinsteiger ;-)

(Hoffentlich mache ich micht nicht unbeliebt, indem ich Euch "nötige",
dass Ihr meine Hausaufgaben korrigiert :))

Ich habe (rudimentär) versucht, das Tool "Agent Group Order" in Python und Wxpython
nachzuschreiben (Listing am Ende).

Welche grundlegenden Code (Struktur) Verbesserungen könnte dieses Programm
haben?

BTW: Welchen Editor verwendet ihr?

(ich weiss, (x)emacs oder vim wäre optimal, aber ich aus der (noch) Windows
Welt kann mich an sie schwer eingewöhnen)

Autovervollständigung (bzw. Vorschläge), Identation automatisch erkennen,
Python Hilfe leicht aufrufbar (vom Editor) wäre optimal.

Die normale Python Hilfe, soweit ich weiss, gibt es nur als html Datei.
Bei Active-Python kann ich sie auch als .chm herunterladen.

Am liebsten wäre mir das hlp Format, da ich auf diese Hilfe auch direkt in
Multi-Edit zugreifen kann.

Beste Grüsse,

-----------------------------------------------------------------------------

from wxPython.wx import *
from string import *
import operator

class MyFrame(wxFrame):
def __init__(self, parent, title):

#frame init
wxFrame.__init__(self, parent, -1, title, size=(500, 450))

#create menu
menuBar = wxMenuBar()
menu = wxMenu()
menu.Append(101, "E&xit\tAlt-X", "Exit demo")
EVT_MENU(self, 101, self.OnButtonClose)
menuBar.Append(menu, "&File")
self.SetMenuBar(menuBar)

#panel und buttons
self.panel = wxPanel(self, -1)
self.btn_write = wxButton(self.panel, -1, "Write", wxPoint(280, 140))
self.btn_write.SetDefault()
self.btn_up = wxButton(self.panel, -1, "Up", wxPoint(280, 60))
self.btn_up.SetDefault()
self.btn_down = wxButton(self.panel, -1, "Down", wxPoint(280, 100))
self.btn_down.SetDefault()


def read_and_display_groups (self):

self.groupList = []
self.subs_groupList = []

dat_array = []
#bind Button Methods
EVT_BUTTON(self, self.btn_write.GetId(), self.OnButtonWrite)
EVT_BUTTON(self, self.btn_up.GetId(), self.OnButtonUp)
EVT_BUTTON(self, self.btn_down.GetId(), self.OnButtonDown)

#filelength and entries
input_idx.seek (0, 2)
idx_bytes = input_idx.tell()
idx_entries = input_idx.tell() /16
input_idx.seek (0, 0)

#grouplist
dlg_read_in = wxProgressDialog("Reading dat file", "Status", 3, self.panel)
for line in input_dat.readlines():
self.groupList.append (line)

#idx array
dlg_read_in.Update(2, "Read Idx File")
self.idx_string = input_idx.read ()
dlg_read_in.Destroy()

dlg_process = wxProgressDialog("Process ...", "Status", 100, self.panel)

old = -1
hundredth = idx_bytes / 100
if hundredth == 0:
hundredth = 1
for index in range (0, idx_bytes, 16):
if index / hundredth != old:
old = index / hundredth;
dlg_process.Update (old, str (old) + ' %')

#extract flags
subscribed = 0
if operator.and_ (ord (self.idx_string [index+3]), 0x80):
subscribed = 1
if operator.and_ (ord (self.idx_string [index+3]), 0x04):
pass
if operator.and_ (ord (self.idx_string [index+3]), 0x08):
pass

dat_array.append (self.idx_string [index + 11])

#subscribed grouplist
if subscribed:
self.subs_groupList.append (replace(self.groupList [index/16], "\n", ""))

dlg_process.Destroy()
self.lb1 = wxListBox(self.panel, 60, wxPoint(80, 50), wxSize(180, 250),
self.subs_groupList, wxLB_SINGLE)

#append index for sorting
for i in range (0, len (self.subs_groupList)):
self.subs_groupList = "%6d" % i + self.subs_groupList + "\n"

#bindings for listbox
EVT_LISTBOX(self, 60, self.EvtListBox)
EVT_LISTBOX_DCLICK(self, 60, self.EvtListBoxDClick)

self.lb1.SetSelection(0)
self.btn_down.SetFocus ()

def OnButtonWrite(self, evt):
output_dat = open ("groups1.dat", 'w')
output_idx = open ("groups1.idx", 'wb')

dat_file_ptr = 0
idx_ptr = 0;
current_entry = 0;

for line in self.subs_groupList:
#write one entry of subscribed list in groups.dat
output_dat.write (line [6:])
i = dat_file_ptr
b_0 = i / 2**16
i = i % 2**16
b_1 = i / 2**8
i = i % 2**8
b_2 = i

#extract index
index = int (line [:6])
temp_str = chr (b_2) + chr (b_1) + chr (b_0) + self.idx_string [index * 16 + 3:index * 16 + 16]

#write one entry of subscribed list in groups.idx
output_idx.write (temp_str);
dat_file_ptr = dat_file_ptr + len (line) - 5; #wegen cr, lf ist ein zeichen?
current_entry += 1

#write rest (is unchanged)
temp_entry = 0
for line in self.groupList:
temp_entry += 1
if temp_entry > current_entry:
output_dat.write (line)

output_idx.write (self.idx_string [current_entry*16:])
output_dat.close ()
output_idx.close ()
wxMessageBox("Written", "Ready", wxOK)

def OnButtonClose(self, evt):
self.Close()

def OnButtonUp(self, evt):
#swaps positon in subs_grouplist and directly in listbox
selected = self.lb1.GetSelection()
selected_string = self.lb1.GetString(selected)
self.lb1.Delete(selected)
i = self.subs_groupList.pop (selected)
self.lb1.InsertItems ([selected_string], selected -1)
self.subs_groupList.insert (selected - 1, i)
self.lb1.SetSelection (selected-1)

#button up/down enable/disable and focus
if selected-1 == 0:
self.btn_up.Enable (FALSE)
self.btn_down.SetFocus ()
else:
self.btn_up.Enable (TRUE)
self.btn_down.Enable (TRUE)

def OnButtonDown(self, evt):
#see above
selected = self.lb1.GetSelection()
selected_string = self.lb1.GetString(selected)
self.lb1.Delete(selected)
i = self.subs_groupList.pop (selected)
self.lb1.InsertItems ([selected_string], selected + 1)
self.subs_groupList.insert (selected + 1, i)
self.lb1.SetSelection (selected+1)

if selected+1 == self.lb1.Number () - 1:
self.btn_down.Enable (FALSE)
self.btn_up.SetFocus ()
else:
self.btn_down.Enable (TRUE)
self.btn_up.Enable (TRUE)

def OnButtonClose(self, evt):
self.Close()

def EvtListBox(self, event):
#directly select in list box
#button up/down enable/disable and focus
if self.lb1.GetSelection() == self.lb1.Number () - 1:
self.btn_down.Enable (FALSE)
else:
self.btn_down.Enable (TRUE)

if self.lb1.GetSelection() == 0:
self.btn_up.Enable (FALSE)
self.btn_down.SetFocus ()
else:
self.btn_up.Enable (TRUE)

if self.lb1.GetSelection() == self.lb1.Number () - 1:
self.btn_up.SetFocus ()

def EvtListBoxDClick(self, event):
pass

app = wxPySimpleApp()
input_dat = open ("groups.dat", 'r')
input_idx = open ("groups.idx", 'rb')
frame = MyFrame(None, "(Freeware) F.St. Utility for Agent")
frame.Show(true)
frame.read_and_display_groups ()
app.MainLoop()



-----------------------------------------------------------------------------
hans
User
Beiträge: 728
Registriert: Sonntag 22. September 2002, 08:32
Wohnort: Sauerland
Kontaktdaten:

erster Eindruck: sieht sauber aus. Einrückung schwach. Du solltest bei Sourcecode immer die Atrtribute [ code ] ...... [ /code ] verwenden. Damit wird auf eine Schriftart mit fester Weite umgeschaltet, z. B. Courier.

Versuch doch mal einen vi für Windump, z. B. bei http://www.softwareonline.org/products.html. Wie bei Windowsprogrammen üblich, Kostet der aber. Ich finde aber, es lohnt sich.

Mach ich doc mal ein Probelauf mit deinem Programm

Hans
hans
User
Beiträge: 728
Registriert: Sonntag 22. September 2002, 08:32
Wohnort: Sauerland
Kontaktdaten:

sieht leider nur sauber aus. Die Einrückung ist nicht vorhanden. Somit wird dieses script vom Pythoninterpreter nicht als Pythonscript akzeptiert.

Hans
Gast

Hallo Hans,

danke erstmals für Deine prompte Antwort,

ich probiere es nochmals (habe vorhin den code einfach mit copy/paste eingefügt):

also code:

Code: Alles auswählen


from wxPython.wx import *
from   string import *
import operator

class MyFrame(wxFrame):
    def __init__(self, parent, title):

        #frame init
        wxFrame.__init__(self, parent, -1, title, size=(500, 450))

        #create menu
        menuBar = wxMenuBar()
        menu = wxMenu()
        menu.Append(101, "E&xit\tAlt-X", "Exit demo")
        EVT_MENU(self, 101, self.OnButtonClose)
        menuBar.Append(menu, "&File")
        self.SetMenuBar(menuBar)

        #panel und buttons
        self.panel = wxPanel(self, -1)
        self.btn_write = wxButton(self.panel, -1, "Write", wxPoint(280, 140))
        self.btn_write.SetDefault()
        self.btn_up = wxButton(self.panel, -1, "Up", wxPoint(280, 60))
        self.btn_up.SetDefault()
        self.btn_down = wxButton(self.panel, -1, "Down", wxPoint(280, 100))
        self.btn_down.SetDefault()


    #process groups.dat and groups.idx
    def read_and_display_groups (self):

        self.groupList = []
        self.subs_groupList = []

        dat_array = []
        #bind Button Methods
        EVT_BUTTON(self, self.btn_write.GetId(), self.OnButtonWrite)
        EVT_BUTTON(self, self.btn_up.GetId(), self.OnButtonUp)
        EVT_BUTTON(self, self.btn_down.GetId(), self.OnButtonDown)

        #filelength and entries
        input_idx.seek (0, 2)
        idx_bytes = input_idx.tell()
        idx_entries = input_idx.tell() /16
        input_idx.seek (0, 0)

        #grouplist
        dlg_read_in = wxProgressDialog("Reading dat file", "Status", 3, self.panel)
        for line in input_dat.readlines():
          self.groupList.append (line)

        #idx array
        dlg_read_in.Update(2, "Read Idx File")
        self.idx_string = input_idx.read ()
        dlg_read_in.Destroy()

        #dlg_process = wxProgressDialog("Process ...", "Status", idx_bytes, self.panel)
        dlg_process = wxProgressDialog("Process ...", "Status", 100, self.panel)

        old = -1
        hundredth = idx_bytes / 100
        if hundredth == 0:
          hundredth = 1
        for index in range (0, idx_bytes, 16):
          if index / hundredth != old:
            old = index / hundredth;
            dlg_process.Update (old, str (old) + ' %')

          #extract flags
          subscribed = 0
          if operator.and_ (ord (self.idx_string [index+3]), 0x80):
            subscribed = 1
          if operator.and_ (ord (self.idx_string [index+3]), 0x04):
            pass
          if operator.and_ (ord (self.idx_string [index+3]), 0x08):
            pass

          dat_array.append (self.idx_string [index + 11])

          #subscribed grouplist
          if subscribed:
            self.subs_groupList.append (replace(self.groupList [index/16], "\n", ""))

        dlg_process.Destroy()
        self.lb1 = wxListBox(self.panel, 60, wxPoint(80, 50), wxSize(180, 250),
            self.subs_groupList, wxLB_SINGLE)

        #append index for sorting
        for i in range (0, len (self.subs_groupList)):
          self.subs_groupList [i] = "%6d" % i + self.subs_groupList [i] + "\n"

        #bindings for listbox
        EVT_LISTBOX(self, 60, self.EvtListBox)
        EVT_LISTBOX_DCLICK(self, 60, self.EvtListBoxDClick)

        self.lb1.SetSelection(0)
        self.btn_down.SetFocus ()

    def OnButtonWrite(self, evt):
        output_dat = open ("groups1.dat", 'w')
        output_idx = open ("groups1.idx", 'wb')

        dat_file_ptr = 0
        idx_ptr = 0;
        current_entry = 0;

        for line in self.subs_groupList:
          #write one entry of subscribed list in groups.dat
          output_dat.write (line [6:])
          i = dat_file_ptr
          b_0 = i / 2**16
          i = i % 2**16
          b_1 = i / 2**8
          i = i % 2**8
          b_2 = i

          #extract index
          index = int (line [:6])
          temp_str = chr (b_2) + chr (b_1) + chr (b_0) + self.idx_string [index * 16 + 3:index * 16 + 16]

          #write one entry of subscribed list in groups.idx
          output_idx.write (temp_str);
          dat_file_ptr = dat_file_ptr + len (line) - 5; #wegen cr, lf ist ein zeichen?
          current_entry += 1

        #write rest (is unchanged)
        temp_entry = 0
        for line in self.groupList:
          temp_entry += 1
          if temp_entry > current_entry:
            output_dat.write (line)

        output_idx.write (self.idx_string [current_entry*16:])
        output_dat.close ()
        output_idx.close ()
        wxMessageBox("Written", "Ready", wxOK)

    def OnButtonClose(self, evt):
        self.Close()

    def OnButtonUp(self, evt):
        #swaps positon in subs_grouplist and directly in listbox
        selected = self.lb1.GetSelection()
        selected_string = self.lb1.GetString(selected)
        self.lb1.Delete(selected)
        i = self.subs_groupList.pop (selected)
        self.lb1.InsertItems ([selected_string], selected -1)
        self.subs_groupList.insert (selected - 1, i)
        self.lb1.SetSelection (selected-1)

        #button up/down enable/disable and focus
        if selected-1 == 0:
          self.btn_up.Enable (FALSE)
          self.btn_down.SetFocus ()
        else:
          self.btn_up.Enable (TRUE)
        self.btn_down.Enable (TRUE)

    def OnButtonDown(self, evt):
        #see above
        selected = self.lb1.GetSelection()
        selected_string = self.lb1.GetString(selected)
        self.lb1.Delete(selected)
        i = self.subs_groupList.pop (selected)
        self.lb1.InsertItems ([selected_string], selected + 1)
        self.subs_groupList.insert (selected + 1, i)
        self.lb1.SetSelection (selected+1)

        if selected+1 == self.lb1.Number () - 1:
          self.btn_down.Enable (FALSE)
          self.btn_up.SetFocus ()
        else:
          self.btn_down.Enable (TRUE)
        self.btn_up.Enable (TRUE)

    def OnButtonClose(self, evt):
        self.Close()

    def EvtListBox(self, event):
        #directly select in list box
        #button up/down enable/disable and focus
        if self.lb1.GetSelection() == self.lb1.Number () - 1:
          self.btn_down.Enable (FALSE)
        else:
          self.btn_down.Enable (TRUE)

        if self.lb1.GetSelection() == 0:
          self.btn_up.Enable (FALSE)
          self.btn_down.SetFocus ()
        else:
          self.btn_up.Enable (TRUE)

        if self.lb1.GetSelection() == self.lb1.Number () - 1:
          self.btn_up.SetFocus ()

    def EvtListBoxDClick(self, event):
        pass

app = wxPySimpleApp()
input_dat = open ("groups.dat", 'r')
input_idx = open ("groups.idx", 'rb')
frame = MyFrame(None, "(Freeware) F.St. Utility for Agent")
frame.Show(true)
frame.read_and_display_groups ()
app.MainLoop()
Dookie
Python-Forum Veteran
Beiträge: 2010
Registriert: Freitag 11. Oktober 2002, 18:00
Wohnort: Salzburg
Kontaktdaten:

Hallo Francesco,

schaut ja ganz schön aus, allerdings hätte ich noch ein paar Anmerkungen.
Du verwendest unterschiedliche Eintückungstiefen, ich hoffe Du mischt nicht Leerzeichen(Space) und Tabulatoren(Tabs). Ansonst verwende am besten immer 4 Leerzeichen zum Einrücken. Gute Editoren lassen sich so einstellen, daß sie immer Leerzeichen verwenden.

Auch der Teil:

Code: Alles auswählen

        if self.lb1.GetSelection() == self.lb1.Number () - 1:
          self.btn_down.Enable (FALSE)
        else:
          self.btn_down.Enable (TRUE)
lässt sich einfacher als:

Code: Alles auswählen

          self.btn_down.Enable (self.lb1.GetSelection() != self.lb1.Number () - 1)
schreiben.

Auch machst Du manchmal zwischen Funktions-/Methodennamen und Klammerauf ein Leerzeichen und dann wieder nicht, was das Lesen des Codes etwas erschwert. Ich selber bevorzuge die Schreibweise ohne Leerzeichen, aber es gibt auch andere Programmierer, die, die Klammer als Parametertuplebegrenzung sehen und ein Leerzeichen bevorzugen. IMHO sollte beides in Ordnung sein, aber halt nicht beides in einem Code.

Jedenfalls für Dein erstes Python-Programm echt gut.

Gruß

Dookie
FrancescoS

Hallo Dookie,

danke für das Lob,

Mein erstes Python Programm ist eigentlich nicht wirklich,
aber erstes bisschen größeres.

Ich habe auch Teile von Surce code "geklaut" über das WxPyhton Demo von mehreren
Programmen und die Python Sources an sich durchsucht, um möglichst schnell
zu einem Ergebnis zu kommen.

Zur Einrückung:
ich vewende (wie auch in C/C++) eigentlich nur Spaces und eine Einrückung von
2 Zeichen. Ich hasse es, wenn wer 4 oder gar 8 einrückt :)
Die 4 Zeichen kommen nur vom Copy/Paste.

Das mit den Klammer fällt mir erst jetzt auf, ja das erschwert sicher die
Lesbarkeit.

Viele Grüsse
hans
User
Beiträge: 728
Registriert: Sonntag 22. September 2002, 08:32
Wohnort: Sauerland
Kontaktdaten:

Ich habe mir das auch mal angesehen (extra wxPython installiert).

Ich mag es nicht, wenn die GUI komplett im Hauptprogramm entworfen wird. Mag daran Liegen, dass ich normanl mit Delphi arbeite und auf Linux mit QT angefangen habe. Da werden die Daten für das Fenster immer in einer eigenen Datei (Klasse) gespeichert und man leitet diese nur noch ab. Bei komplizierten Fensteraufbauten erhöht das die Lesbarkeit.

Mir ist nicht klar, wofür die Indexdatei benötigt wird und wie diese arbeitet (habe mir aber auch noch nicht die Mühe gemacht, das verstehen zu wollen). Kleine Datenmengen kann man mühelos sequentiell sortieren und bei großen Mengen würde ich wenn eben möglich über eine Datenbank gehen. In einer SQL-Abfrage kann man dann ganz schnell mit order by feldname eine Sortierung erzwingen.

Hans
Gast

hans hat geschrieben:Ich habe mir das auch mal angesehen (extra wxPython installiert).
Hallo Hans,

hast Du extra wegen meiner Frage wxPython installiert? :oops:
In einem anderen Thread habe ich gesehen, dass ich diese Frage besser
im GUI Teil des Forums posten sollte, aber ich wollte nur eine Kritik des Programmstils an sich.
hans hat geschrieben: Ich mag es nicht, wenn die GUI komplett im Hauptprogramm entworfen wird. Mag daran Liegen, dass ich normanl mit Delphi arbeite und auf Linux mit QT angefangen habe. Da werden die Daten für das Fenster immer in einer eigenen Datei (Klasse) gespeichert und man leitet diese nur noch ab. Bei komplizierten Fensteraufbauten erhöht das die Lesbarkeit.
Äh, habe ich mir noch nicht angeschaut, aber ich gebe Dir völlig recht.
Ich möchte in den nächsten Tagen sowieso mal den Boa-Constructor
anschauen. BTW: (nein, ich stelle die Frage jetzt im GUI Forum)
hans hat geschrieben: Mir ist nicht klar, wofür die Indexdatei benötigt wird und wie diese arbeitet (habe mir aber auch noch nicht die Mühe gemacht, das verstehen zu wollen).
Ist auch nicht so wichtig; Forte Agent speichert die Eigenschaften der Gruppenliste etwas umständlich in der index Datei ab.
Antworten