En fråga om bäst kod...

14 svar · 894 visningar · startad av Asa 14 feb. 2005

AsaMedlem sedan dec. 20011 860 inlägg

Frågan14 feb. 2005 10:43#1

Vilken kod är bäst, säkrast och snabbast? Eller finns det något ännu bättre sätt?

<table border="0" cellpadding="3" cellspacing="0" style="border-collapse: collapse" width="100%">
  <%antal=Conn.Execute("Select Count(*) From com_besokare Where mem_id=" & Request.QueryString("id")).Fields(0)
  SQL = "Select B.antal,B.besokar_id,B.datum,B.mem_id,U.ar,U.man,U.dag,U.status,U.id,U.kon,U.anvnamn FROM com_besokare B, com_users U Where B.mem_id=" & Request.QueryString("id") & " and U.id=B.besokar_id Order by B.datum desc limit 20"
  If antal = 0 Then%>
  <tr>
    <td valign="top" colspan="3">Ingen har besökt <%If strKon = 1 Then%>henne<%ElseIf strKon = 2 Then%>honom<%End if%>.</td>
  </tr>
  <%Else
  myArray = Conn.Execute(SQL).GetRows()
  If IsArray(myArray) then
    y=0
    For i = 0 To ubound(myArray,2)
      birthdate= CDate(myArray(4,i)&"-"&myArray(5,i)&"-"&myArray(6,i))%>
      <tr height="24">
        <td style="border-top: 1px solid #000000"><b><%If myArray(7,i) = "1" Then%><img src="<%=URL%>gfx/online.gif" alt="Online"><%Else%><img src="<%=URL%>gfx/offline.gif" alt="Offline"><%End If%> <a href="krypin.asp?id=<%=myArray(8,i)%>"><%=Server.HTMLEncode(myArray(10,i))%></a></b>&nbsp;<%If myArray(9,i) = 1 Then%>F<%Else%>P<%End If%><%=Birth(birthdate)%></td>
        <td style="border-top: 1px solid #000000" align="center">&nbsp;<%If Cint(Session("usr_Id")) = Cint(Request.QueryString("id")) Then%><%=myArray(0,i)%><%ENd If%></td>
        <td style="border-top: 1px solid #000000" align="right"><%=Datum(myArray(2,i))%></td>
      </tr>
    <%y=y+1
    Next
  End If
  End If%>
</table>

eller

<table border="0" cellpadding="3" cellspacing="0" style="border-collapse: collapse" width="100%">
  <%Set RS=Conn.Execute("Select B.antal,B.besokar_id,B.datum,B.mem_id,U.ar,U.man,U.dag,U.status,U.id,U.kon,U.anvnamn FROM com_besokare B, com_users U Where B.mem_id=" & Request.QueryString("id") & " and U.id=B.besokar_id Order by B.datum desc limit 20")
  If RS.EOF Then%>
  <tr>
    <td valign="top" colspan="3">Ingen har besökt <%If strKon = 1 Then%>henne<%ElseIf strKon = 2 Then%>honom<%End if%>.</td>
  </tr>
  <%Else
  Do Until RS.EOF
      birthdate= CDate(RS(4)&"-"&RS(5)&"-"&RS(6))%>
      <tr height="24">
        <td style="border-top: 1px solid #000000"><b><%If RS(7) = "1" Then%><img src="<%=URL%>gfx/online.gif" alt="Online"><%Else%><img src="<%=URL%>gfx/offline.gif" alt="Offline"><%End If%> <a href="krypin.asp?id=<%=RS(8)%>"><%=Server.HTMLEncode(RS(10))%></a></b>&nbsp;<%If RS(9) = 1 Then%>F<%Else%>P<%End If%><%=Birth(birthdate)%></td>
        <td style="border-top: 1px solid #000000" align="center">&nbsp;<%If Cint(Session("usr_Id")) = Cint(Request.QueryString("id")) Then%><%=RS(0)%><%ENd If%></td>
        <td style="border-top: 1px solid #000000" align="right"><%=Datum(RS(2))%></td>
      </tr>
    <%y=y+1
    RS.MoveNext
    Loop
  End If
  RS.Close: Set RS=Nothing%>
</table>

spangoMedlem sedan juni 20008 205 inlägg

14 feb. 2005 10:53#2

Det är öppet för SQL-injektion, stoppa aldrig in användardata direkt på det där sättet. Byt ut Request.QueryString("id") mot något i stil med CInt(Request.QueryString("id"))

BrimbaMedlem sedan dec. 19995 874 inlägg

14 feb. 2005 11:08#3

Läs gärna mer om sql-injection och om hur du kan skydda dig på swesecures hemsida i artikeln SQL Injection.

Angående din fråga så är det helt beroende av hur många anrop du har till din sida samt hur många poster du har i ditt recordset. Generellt sett är det snabbare att loopa en array. Men om det märks i ditt fall går inte att säga.
Det viktigaste istället - om du har prestandaproblem - är att indexera din databas på ett korrekt sätt, samt att använda cache. Läs gärna swesecures artikel om index.

Lycka till

AsaMedlem sedan dec. 20011 860 inlägg

14 feb. 2005 11:23#4

Det med sql-injections vet jag. Ska läggas in men undrar om man använder getrowskoden som är ovan.. den har ej recset.close: set recset=nothing.. Det ska väl vara med eller?

OveRRidEMedlem sedan feb. 200112 078 inlägg

14 feb. 2005 12:27#5

Jag skulle nog inte rekommendera att använda .GetRows alls faktiskt. Det finns ett antal anledningar;

1. Du går helt miste om kolumnernas benämningar/namn. Koden blir således mycket svårläst (för att inte säga oläslig).
2. Den prestandaskillnad som det ger är relativt marginell. Det är liksom inte längre värt det, då det finns mycket, mycket annat som bör optimeras innan man ger sig på att optimera saker som listning av data.
3. Ändrar du databasens kolumnordning/kolumnantal etc, är risken till 90% att du kommer få problem i koden, då index'en i din datamatris antagligen påverkas av detta. Ett recordset håller namn till skillnad från din GetRows-metod som bara har index i kolumnordning.

Dessutom; jag ser att du använder index för att referera till kolumner i ditt Recordset. Det finns ingen som helst mening med detta, så sluta med det omedelbart. ;) Där är ungefär som att döpa sina applikationsfiler till 001.asp, 002.asp, 003.asp osv.

fredrikMedlem sedan dec. 19991 072 inlägg

14 feb. 2005 13:02#6

Dessutom; jag ser att du använder index för att referera till kolumner i ditt Recordset.

Njaa... :) För absolut bästa prestanda så hämtas data ut snabbare med index än med kolumn-namnet. Dock så blir läsbarheten som sagt sämre....

...att använda GetRows() ger visserligen mer svårläst kod, men den behöver inte blir svårare att underhålla beroende på hur man bygger sin logik...i en skiktad lösning där enititets-klasser används, så blir det ändå bara ett ställa att ändra på...men då är det ändå inte prestanda-optimerat, så det går kanske på jämt ut då :)

OveRRidEMedlem sedan feb. 200112 078 inlägg

14 feb. 2005 13:12#7

fredrik skrev:

Dessutom; jag ser att du använder index för att referera till kolumner i ditt Recordset.

Njaa... :) För absolut bästa prestanda så hämtas data ut snabbare med index än med kolumn-namnet. Dock så blir läsbarheten som sagt sämre....

Dock; jag tror det finns andra delar som kan optimeras lättare/mer effektivt än just sådana saker i en applikation som denna. Det känns som att jaga myror med hagelgevär, speciellt när man tummar så mycket på läsbarheten i koden och spikar sin databasstruktur så hårt som i detta fallet. Det är liksom inte på grund av att Asa refererar sina kolumnnamn med strängar istället för index som hennes applikation kommer få prestandaproblem, om det nu inträffar. Och om det nu är så att man måste göra sådana optimeringar, då är det nog dags att se sig om efter ny hårdvara/hosting.

fredrik skrev:

...att använda GetRows() ger visserligen mer svårläst kod, men den behöver inte blir svårare att underhålla beroende på hur man bygger sin logik...i en skiktad lösning där enititets-klasser används, så blir det ändå bara ett ställa att ändra på...men då är det ändå inte prestanda-optimerat, så det går kanske på jämt ut då :)

Man får ju återigen se på vilket context vi ligger inom i det här fallet också. Jag tvivlar på att Asa's applikation följer någon slags N-tiermodell, än mindre använder/är uppbyggt med klasser eller objekt i så stor utsträckning.

AsaMedlem sedan dec. 20011 860 inlägg

14 feb. 2005 13:32#8

Dessutom; jag ser att du använder index för att referera till kolumner i ditt Recordset..

Vaddå?

spangoMedlem sedan juni 20008 205 inlägg

14 feb. 2005 13:46#9

Du använder GetRows, vilket är ett bekvämt sätt att förminska läsbarheten av koden på. Det lilla du vinner i prestanda på det är förmodligen inte värt det.

AsaMedlem sedan dec. 20011 860 inlägg

14 feb. 2005 14:04#10

Så jag kan använda kod 2 istället? Isåfall känns det skönt för Getrows är lite småjobbigt tycker jag.

OveRRidEMedlem sedan feb. 200112 078 inlägg

14 feb. 2005 14:20#11

Det är iallafall min (och spango's?) åsikt.

Dock, använd namn instället för index (nummer) när du refererar till kolumner i ditt recordset (rs). Exempelrad:

'gör inte såhär
CDate(RS(1)&"-"&RS(5)&"-"&RS(6))

'gör såhär
CDate(RS("DatumKolumn1")&"-"&RS("DatumKolumn5")&"-"&RS("DatumKolumn6"))

Förstår du?

/red. Sen är det aldrig fel med lite mellanslag i koden heller, typ;

CDate(RS("DatumKolumn1") & "-" & RS("DatumKolumn5") & "-" & RS("DatumKolumn6"))

spangoMedlem sedan juni 20008 205 inlägg

14 feb. 2005 14:34#12

OveRRidE skrev:

Det är iallafall min (och spango's?) åsikt.

Jomenvisst, här tycker vi lika för en gångs skull ;)
Sen kan man påpeka att datum borde lagras som datum i en kolumn i stället för som heltal i tre kolumner, men det är en annan sak...

OveRRidEMedlem sedan feb. 200112 078 inlägg

14 feb. 2005 14:37#13

Det där med datumen håller jag med dig om, men det kändes som ett annat problem. :)

AsaMedlem sedan dec. 20011 860 inlägg

14 feb. 2005 14:59#14

Jag har så med datumet av vissa skäl. Jag trodde det vad bättre att ha så: CDate(RS(1)&"-"&RS(5)&"-"&RS(6))

Med siffror. Var nån som sa det till mig förrut! Så det är inte sant alltså?

OveRRidEMedlem sedan feb. 200112 078 inlägg

14 feb. 2005 16:31#15

Nja, en datumkolumn (iallafall i t.ex. SQL-server) ser ju till att hålla reda på inte bara datum, utan även kalenderdatum, vilket innebär att du t.ex. inte kan föra in datumet 2004-02-31. Har du det uppdelat i tre kolumner som bara fattar heltal så kan ju ju i princip få in datum som 2005-40-67, och den månaden finns ju inte på vår planet, så.. ;)

Dessutom slipper du hålla på att formatera datumet varje gång du skall använda det. Vidare; det brukar anses som en dödssynd att stränghantera datum på det sättet. :OO

Jag kan liksom inte se någon riktig fördel med det.