Kritikk på kode!

[alfao232ga]

Hei.

Driver å lærer Python, og håper noen kan ta en titt på denne koden og gi meg kritikk! Hva skal IKKE gjøres! Hva kunne vært annerledes?! Etc.! Jeg ser ikke sånnt nå.

Jeg kan nevne at det henter ut data fra log fila til en tf2 server, eller ment for tf2 da! spill! Og... når folk skriver der !dom eller !nem så får dem opp hvem som dominater dem snart! Eller hvem dem er på vei til å dominate! Bare for lek!

 

Her er koden:

 

 

#!/usr/bin/env python
# -*- coding: utf-8 -*-

from SRCDS import SRCDS
from socket import *
import re
import MySQLdb

db = MySQLdb.connect("localhost", "root", "HEMMELIG!!!!", "tf2")
dbCursor = db.cursor()

dbCursor.execute("TRUNCATE TABLE `dom`")

rcon = SRCDS("192.168.23.10", 27015, "HEMMELIG ^^")	

connectData = ("192.168.23.10", 27502)

udpClientSocket = socket(AF_INET, SOCK_DGRAM)
udpClientSocket.bind(connectData)

dateInfoRegExpRef = re.compile(r'^.*L (\d{2})\/(\d{2})\/(\d{4}) - (\d{2}):(\d{2}):(\d{2}):\s*', re.IGNORECASE)
serverInfoNewRoundRegExp = re.compile(r'\s*World triggered "Round_Start"\s*', re.IGNORECASE)
playerInfoRegExpRef = re.compile(r'"(.*?)<\d*><(STEAM_[\d:]+)><\w*>"\s*', re.IGNORECASE)
actionDisconnectedRegExpRef = re.compile(r'^disconnected', re.IGNORECASE)
actionKilledOrAssistRegExpRef = re.compile(r'(killed|triggered "kill assist" against)\s*"(.*?)<\d*><(STEAM_[\d:]+)><\w*>"\s*')
actionTeamChangeRegExpRef = re.compile(r'^joined team', re.IGNORECASE)
actionSayRegExpRef = re.compile(r'\s*(say|say_team)\s*"(.*)"')

while 1:
line = ''
lineFixed = ''
data = udpClientSocket.recvfrom(1024)
if not data:
	break
else:
	line = data[0]

	print line

	if dateInfoRegExpRef.match(line):
		lineFixed = dateInfoRegExpRef.sub('', line, 1)

		serverInfoNewRoundMatch = serverInfoNewRoundRegExp.match(lineFixed)
		playerInfoMatch = playerInfoRegExpRef.match(lineFixed)

		if serverInfoNewRoundMatch:
			dbCursor.execute("TRUNCATE TABLE `dom`")
		elif playerInfoMatch:
			playerSteamId = playerInfoMatch.group(2).strip()
			playerName = playerInfoMatch.group(1).strip()
			lineFixed = playerInfoRegExpRef.sub('', lineFixed, 1)

			actionDisconnectedMatch = actionDisconnectedRegExpRef.match(lineFixed)
			actionKilledOrAssistMatch = actionKilledOrAssistRegExpRef.match(lineFixed)
			actionSayMatch = actionSayRegExpRef.match(lineFixed)
			actionTeamChangeMatch = actionTeamChangeRegExpRef.match(lineFixed)

			if actionDisconnectedMatch:
				dbCursor.execute("delete from `dom` where `playerSteamId` = '%s' or `victimSteamId` = '%s'" %(playerSteamId, playerSteamId))
			elif actionKilledOrAssistMatch:
				lineFixed = actionKilledOrAssistRegExpRef.sub('', lineFixed, 1)
				victimSteamId = actionKilledOrAssistMatch.group(3).strip()
				victimName = actionKilledOrAssistMatch.group(2).strip()

				dbCursor.execute("delete from `dom` where `victimSteamId` = '%s' and `playerSteamId` = '%s'" %(playerSteamId, victimSteamId))

				dbCursor.execute("select 1 from `dom` where `playerSteamId` = '%s' AND `victimSteamId` = '%s'" %(playerSteamId, victimSteamId))
				if dbCursor.fetchone():
					dbCursor.execute("update `dom` set `kills` = `kills` + 1 where `playerSteamId` = '%s' and `victimSteamId` = '%s'" %(playerSteamId, victimSteamId))
				else:
					dbCursor.execute(
					"""
						insert into `dom` 
							(`playerSteamId`, `playerName`, `victimSteamId`, `victimName`, `kills`) 
						values ('%s', '%s', '%s', '%s', 1)
					""" %(playerSteamId, playerName, victimSteamId, victimName))
			elif actionSayMatch:
				playerSaid = actionSayMatch.group(2).strip()

				if playerSaid == "!dom" or playerSaid == "!nem":
					dbCursor.execute("""
						delete
						from `dom`
						where `kills` >= 4
					""")

				if playerSaid == "!dom":
					rcon.say(playerName + " du dominerer snart:")

					dbCursor.execute("""
						select
						`victimName`, `kills`
						from `dom`
						where `playerSteamId` = '%s'
						order by `kills` desc
						limit 3
					""" %(playerSteamId))

					result = dbCursor.fetchall()

					gatherSoonDominated = ""

					i = 0
					for rowData in result:
						victimName, kills = rowData
						killsLeft = 4 - kills
						gatherSoonDominated += "%s (%d)" %(victimName, killsLeft)
						i += 1
						if i != 3:
							gatherSoonDominated += ", "

					if i == 0:
						rcon.say("Ingen!")
					else:
						rcon.say(gatherSoonDominated)
				elif playerSaid == "!nem":
					rcon.say(playerName + " du blir snart dominert av:")

					dbCursor.execute("""
						select
						`playerName`, `kills`
						from `dom`
						where `victimSteamId` = '%s'
						order by `kills` desc
						limit 3
					""" %(playerSteamId))

					result = dbCursor.fetchall()

					gatherSoonDominatedBy = ""

					i = 0
					for rowData in result:
						playerName, kills = rowData
						killsLeft = 4 - kills
						gatherSoonDominatedBy += "%s (%d)" %(playerName, killsLeft)
						i += 1
						if i != 3:
							gatherSoonDominatedBy += ", "

					if i == 0:
						rcon.say("Ingen!")
					else:
						rcon.say(gatherSoonDominatedBy)
			elif actionTeamChangeMatch:
				dbCursor.execute("delete from `dom` where `playerSteamId` = '%s' or `victimSteamId` = '%s'" %(playerSteamId, playerSteamId))

udpClientSocket.close()
dbCursor.close()

 

 

Kan nevne seg at det.. henger seg! F. eks. hvis en runde er over så henger det seg. Har ikke funnet ut åssen jeg løser det enda...

Takk for alle ideer og tanker

[terjeelde]

Første jeg legger merke til er at koden er en eneste stor sammenhengende bit. Det gjør det lite lesbart, og vanskeligere å se hvordan ting henger sammen. Både for deg, og for oss.

 

Veldig vanlig egentlig, spesielt når man er fersk med ett språk. Skriver litt, tester det, skriver mer, og fortsetter så lenge det virker. Det jeg egentlig vil anbefale er at du hugger tak i den koden du har nå, og prøver å knekke den om til mindre stykker med kode.

 

Slik jeg forstår det er alt sentrert rundt linjer du får via UDP, og så gjør du ting basert på det du får. Koden er skrevet slik at du gjør alt på en gang, i hele koden, og det blir litt uoversiktlig.

 

Ofte er det bedre å fokusere på en ting av gangen, og gjøre den bra. Foreslår at du deler koden din i tre:

 

a) En klasse for å holde meldingene. Kan hete TF2Line eller noe, eller en klasse pr. type melding.

 

b) Ett sett med koder for å gjøre de tingene du skal gjøre. Selve Business-Logic-biten.

 

c) En bit med kode for å drive "main event loop". Gjerne 20 linjer eller mindre. En liten snutt som leser fra UDP-socketen, og f.eks først gjør den om til noe som er mer behagelig for deg å jobbe med. Kan godt gjøre om til en instans av TF2Line, og så gi den instansen videre til business-logikk, basert på hva det er du vil gjøre.

 

 

Alt etter smak og behag kan TF2Line håndtere business-logikken også. F.eks med en metode for hver type melding, som "mottar" de forskjellige kommandoene.

 

Kanskje litt knotete ordlagt, men gi lyd om jeg var for knotete og gjøtete i ordleggingen min.

[alfao232ga]

Hvis jeg skal gjøre det du sier, jeg mener jeg har forstått hva du tenker ganske godt!

Hva gjør jeg så med alle disse compila regexp-ene? Vil jo helst ikke ha de slengt inn i tørketrommelen om og om igjen.

 

Mange takk skal du ha .

[terjeelde]

Hvis jeg skal gjøre det du sier, jeg mener jeg har forstått hva du tenker ganske godt!

Hva gjør jeg så med alle disse compila regexp-ene? Vil jo helst ikke ha de slengt inn i tørketrommelen om og om igjen.

 

Greieste er å bare bruke regexpene i koden som parser meldingene om til noe mer Python-aktig. Hvis du har f.eks en TF2Line-klasse, så kan du compile dem som klasse-attributter.

 

class TF2Line:
dateInfoRegExpRef = re.compile(r'^.*L (\d{2})\/(\d{2})\/(\d{4}) - (\d{2}):(\d{2}):(\d{2}):\s*', re.IGNORECASE)
serverInfoNewRoundRegExp = re.compile(r'\s*World triggered "Round_Start"\s*', re.IGNORECASE)
(osv)

 

Da kan du bruke dem fra alle metodene i TF2Line, ved å bruke f.eks: selv.dateInfoRegExpRef.match( data )

De compiles bare når du laster klassen, ikke når du f.eks lager nye instanser av dem, så det blir ikke tregere av den grunn. Faktisk blir det vel teoretisk kanksje litt raskere, siden de blir i lokalt namespacet for klassen.

 

Annen tanke er at disse navnene ble litt lange og tunge. Hva med å f.eks ganske enkelt kalle den re_dateinfo?

 

Mange takk skal du ha .

 

God trening for meg også.

[terjeelde]

God ting å ha i bakhodet når du skriver kode:

 

Gjør minst mulig.

 

En modul, funksjon, loop, eller hva det måtte være, har stort sett godt av å være så kort som mulig, og fokusere på en bittelitten bit av oppgaven.

 

Ikke at det er feil å skrive større ting. Mange av små-prosjektene mine ser ut omtrent som kodesnutten din når jeg er ferdig med "første utkast". Minner meg om ett annet uttrykk:

 

Build one to throw one away.

 

Fokusen på å skrive liten og effektiv kode kommer ofte i veien for å rett og slett få koden skrevet. Spesielt på hobby-prosjekter, hvor jeg gjerne kan bruke ett par uker på å gjøre ett lite valg, rett og slett fordi jeg kan. Ofte får jeg mer gjort hvis jeg hopper litt over dette med å skulle ha alt så perfekt, og bare får koden skrevet.

 

"Build one to throw one away" er derfor også litt fin å ha i bakhodet. Første biten du skriver kan være uorganisert og fæl, men når den virker, så virker den. Da har du også lært hva nå enn du trengte å lære for å få den til å funke, og du er mye bedre kvalifisert til å vite hvordan du bør strukturere koden enn du var når du begynte.

 

For hobby-prosjektene mine blir det ofte at jeg smører sammen noe fæl gugge av noe rotete kode, som knapt ser ut, men som virker. Så tar jeg heller å knekker om hele koden, og restrukturerer absolutt alt, før jeg sier meg ferdig.

 

(Jobber på litt annen måte med mer seriøse prosjekter forsåvidt. ulike behov)... hmm, huff jeg babler....

[alfao232ga]

Takk skal du ha! Koslig å lese det du skrever.

"Spesielt på hobby-prosjekter, hvor jeg gjerne kan bruke ett par uker på å gjøre ett lite valg, rett og slett fordi jeg kan." uff... ja... noen ganger kan man føle... ja, ta nå bare det å gå videre. Men fordi jeg KAN ja, må lære meg å tenke sånn.

Jeg skal lese litt mer OOP når det kommer til Python før jeg begynner å endevende koden . Men! Den socket greia! Hvis f. eks. forbindelsen brytes, hvordan kan få den til å hele tiden forsøke å koble seg til på nytt? Tips?

 

Har forresten annet spm. er det muligheter for å kunen skrive til programmet når som helst! Selv om den driver å printer ut? Så kan jeg skrive quit, også stopper det! F. eks!

[terjeelde]

Takk skal du ha! Koslig å lese det du skrever.

 

 

Men! Den socket greia! Hvis f. eks. forbindelsen brytes, hvordan kan få den til å hele tiden forsøke å koble seg til på nytt? Tips?

 

Jeg bruker ett rammeverk som heter Twisted. Det er ett even-driven framework, som blir en litt annen måte å tenke på. Tar litt tid å bli vant til, men ganske behagelig å bruke. Da har jeg f.eks en klasse for en protokoll, og en "fabrikk" som setter opp forbindelsen. Hvis den går ned, så får fabrikken beskjed, og bare lager en ny forbindelse. Enkelte synes Twisted er litt knot å bli vant til, men jeg liker den ganske godt.

 

Er en stund siden jeg har kodet dette for hånd, men hvis ikke jeg husker feil på en sen romjulskveld, så får du en exception når du prøver å lese på forbindelsen etter den har ramlet ned. Du kan jo da ganske enkelt sette den opp igjen.

 

For udpClientSocket-socketen din trenger du forsåvidt ikke bekymre deg. Det er jo UDP, så den kan jo strengt tatt ikke gå ned.

 

Har forresten annet spm. er det muligheter for å kunen skrive til programmet når som helst! Selv om den driver å printer ut? Så kan jeg skrive quit, også stopper det! F. eks!

 

Tja, du kan jo skrive til programmet når som helst, så plukkes det opp når du leser fra en socket. Hvor raskt det kan reagere på det du skriver blir jo da litt styrt av hvor ofte du henter nye meldinger fra socketen.

 

Hvis du skriver programmet ditt rundt en event-loop, så er en grei mulighet å f.eks få skrive-funksjonen til å bare kalle en linje av gangen, og sette en variabel hvis den skal skrive mer.

 

Når du kommer til at du skal lese fra socketen, så leser du ikke direkte, men bruker select() først, for å se om der er data. Når du skal kalle select(), så ser du om skrive-funksjonen har satt variabel som sier den vil skrive mer, og isåfall får du select() til å ikke blokke, men bare rase gjennom. Hvis programmet da har fått en ny kommando om å slutte å skrive, så får du kjørt den før nestegang du kaller print... Hmm, kanskje verdens beste forklaring...

 

prøver litt bedre...

 

recvfrom blokker til den får data, så programmet står der og sover, til det skjer noe på socketen.

 

select kan lytte på flere sockets, og returnere enten når det er noe spennende på en av dem, eller en timeout.

 

select tar tre lister: liste over sockets du kan lese fra, skrive til, eller som du vil sjekke etter andre ting (forbindelse brutt osv)

til slutt kan den ta argument på hvor lenge den skal blokke mens den venter på data.

tilbake gir den tilsvarende tre lister, men denne gangen med bare de socketene hvor det har skjedd noe.

 

Du kan da gjøre noe ala:

 

us = socket( AF_INET, SOCK_DGRAM )
us.bind( ("192.168.23.10", 27502) )
timeout = 3600

while 1:
workableSockets = select( [us], [], [], timeout )

# Sjekk om det var noe å hente på å lese fra socketen
if workableSockets[0]:
	# Vi vet at socketen har data å gi, så recvfrom vil aldri blokke her
	data = us.recvfrom( 10000 )
	quit_function( data )

timeout = print_line()

 

Du kan da lage print_line() slik at den alltid skriver bare en linje. Hvis den har flere linjer den vil skrive, så returnerer du 0 tilbake, og timeout blir satt til 0. Da vil ikke select() blokke, men bare sjekke om det er nytt data å hente. Isåfall leses det, og gis til quit-funksjonen.

 

Etter dette kjøres print_line() igjen, altså nesten med en gang hvis den har mer den vil printe. Den kan da printe videre. Når den har printet alt den vil, så returnerer den f.eks 3600 tilbake, og select() vil blokke til det skjer noe spennende.

[Terrasque]

while 1:

 

ugg

 

running = True

while running:
 Bla bla
 if exit:
running = False

 

Litt mer kontroll, og ser bedre ut

 

Også som de andre har sagt, del opp ting

 

Edit : ta også en titt på queue / threading. Hoved ideen er en thread for incoming, en (eller flere) for logikk, og en for outgoing. Basically:

 

import threading
import Queue

InQueue = Queue.Queue(100) 
# Hundre items i queue bør holde. Bør gi litt DoS protection også
OutQueue = Queue.Queue(100)

# tre threads som behandler logikk
# hvis en bruker lang tid, vil de to andre fortsette å behandle køen.
WORKERS = 3 

class Printer(threading.Thread):
def __init__(self, queue):
	self.__queue = queue
	self.rcon = SRCDS("192.168.23.10", 27015, "HEMMELIG ^^")
	threading.Thread.__init__(self)

def run(self):
	while 1:
		item = self.__queue.get()
		if item is None:
			break
		self.rcon.say(item)

class Logic(threading.Thread):
def __init__(self, inqueue, out):
	self.__inqueue = inqueue
	self.__outqueue = outqueue
	threading.Thread.__init__(self)

def run(self):
	while 1:
		item = self.__inqueue.get()
		if item is None:
			break
		# Logikk delen - Selve logikken bør være en annen plass i koden.
					# Denne bør bare call'e den koden, og så sende resultatet videre.
		self.__outqueue.put(output) #output er det som skal sendes til serveren

for i in range(WORKERS): #Start 3 logikk threads
Logic(InQueue, OutQueue).start()

Printer(OutQueue).start() # Og en skrive thread

while running: #Main loop leser linjer, og sender til logikk threadene
 linje = getline()
 InQueue.put(linje)
 #Mer logikk - som avslutting og lignende

# Legger inn et None objekt i køen for hver thread
# For å fortelle at de skal avslutte.
for i in range(WORKERS):
 InQueue.put(None)
OutQueue.put(None)

 

Med dette så vil lesing, behandling og skriving fungere uavhengig av hverandre, og man kan sende kommandoer mens den holder på å behandle en annen. queue.get vil låse til det er et objekt å hente ut.

Endret 29. desember 2008 av Terrasque

[alfao232ga]

Har lest på threading etc. i det siste ja. Det er noe jeg ikke har forstått!

threading.Thread.__init__(self) i __init__

Forstår den ikke...

[Terrasque]

De class'ene du lager er en subclass av threading.Thread (som betyr at den arver en del derfra). For å initialisere alt det den trenger må du kjøre init ifra orignal class'en, som den gjør

[zotbar1234]

De class'ene du lager er en subclass av threading.Thread (som betyr at den arver en del derfra). For å initialisere alt det den trenger må du kjøre init ifra orignal class'en, som den gjør

 

 

Hvorfor ikke:

 

class Printer(threading.Thread):
def __init__(self, queue):
	self.__queue = queue
	self.rcon = SRCDS("192.168.23.10", 27015, "HEMMELIG ^^")
	super(Printer, self).__init__(self)

 

? (jeg skulle forresten virkelig ønske at det fantes super(self).foo() som kalte neste metoden foo i MRO-rekkefølgen -- det burde ikke være nødvendig å oppgi 2 argumenter, når det første er det samme som self.__class__).

[terjeelde]

class Printer(threading.Thread):
def __init__(self, queue):
	self.__queue = queue
	self.rcon = SRCDS("192.168.23.10", 27015, "HEMMELIG ^^")
	super(Printer, self).__init__(self)

 

? (jeg skulle forresten virkelig ønske at det fantes super(self).foo() som kalte neste metoden foo i MRO-rekkefølgen -- det burde ikke være nødvendig å oppgi 2 argumenter, når det første er det samme som self.__class__).

 

Vanlige er å gjøre:

class Printer( Thread ):
def __init__( self, queue ):
	Thread.__init__( self )

 

Da slipper du å bruke super osv. Siden du uansett må navngi klassen du skriver kan du like gjerne navngi superklassen direkte, for å få litt ryddigere kode.

 

Med new-style-classes kan du også gjøre det omtrent som du vil:

 

def mysuper( self ):
return super( self.__class__, self )

class A( object ):
def __init__( self ):
	print "A"

class B( A ):
def __init__( self, bar ):
	print "B - bar: %s" % bar
	mysuper(self).__init__()

b = B('foo')

 

Funker fint, og gir output:

 

B - bar: foo
A

[steingrim]

Vanlige er å gjøre:

class Printer( Thread ):
def __init__( self, queue ):
	Thread.__init__( self )

Da slipper du å bruke super osv.

Det er en stund siden super() ble vanlig. super() takler multippel arv, det gjør ikke direkte kall på parent ctor.

[zotbar1234]

Vanlige er å gjøre:

class Printer( Thread ):
def __init__( self, queue ):
	Thread.__init__( self )

 

Da slipper du å bruke super osv.

 

Er det problematisk å bruke super()?

 

Siden du uansett må navngi klassen du skriver kan du like gjerne navngi superklassen direkte, for å få litt ryddigere kode.

 

Noe av poenget mitt var nettopp at det burde finnes en metode som fant selv ut hva neste metoden med samme navn i MRO-rekkefølgen er og kalle den. Common Lisp har call-next-method, som, om jeg ikke husker feil, gjør nettopp det. Ja, det kan løses på egenhånd (vi bruker faktisk et lignende hack), men jeg vil ha ting fungerende uten å måtte mekke på dem. Ja, jeg er veldig lat

 

Det burde ikke være nødvendig å måtte navngi seg selv eller superklassen når man ønsker å si "initialiser de gjenværende del-objektene [gjennon deres respektive ctorer]".

[terjeelde]

Er det problematisk å bruke super()?

 

Nei, ikke egentlig.

 

Noe av poenget mitt var nettopp at det burde finnes en metode som fant selv ut hva neste metoden med samme navn i MRO-rekkefølgen er og kalle den. Common Lisp har call-next-method, som, om jeg ikke husker feil, gjør nettopp det. Ja, det kan løses på egenhånd (vi bruker faktisk et lignende hack), men jeg vil ha ting fungerende uten å måtte mekke på dem. Ja, jeg er veldig lat

 

Det burde ikke være nødvendig å måtte navngi seg selv eller superklassen når man ønsker å si "initialiser de gjenværende del-objektene [gjennon deres respektive ctorer]".

 

Derfor forslaget mitt om mysuper(), som ikke navngir noe som helst, men self finner frem til superklassen av nåværende klasse, uansett hvilke klasse du bruker den i.