5 minutes of looking at the client only

[Joost]

something wrong with all of these, or a way to do it better. If code was properly discussed, mistakes could've been found and taken out. a lot of these mistakes are all over the client.

Code:

Public Sub SendGetClasses()
Dim Packet As String

    Packet = CGetClasses & END_CHAR
    Call SendData(Packet)
End Sub

^Problem : Dim string, set string, and using string right away in the packet. Seems 2 steps more than required. And it's like this every packet

Code:

For i = 1 To MAX_TRADES
        With Shop(ShopNum).TradeItem(i)
            Packet = Packet & SEP_CHAR & .GiveItem & SEP_CHAR & .GiveValue & SEP_CHAR & .GetItem & SEP_CHAR & .GetValue
        End With
    Next

Code:

Problem: Most shops don't have all trades filled, sending a lot of empty data
    Select Case Err
    
        Case -2147024770
            Call MsgBox("dx7vb.dll is either not found or is not registered, try re-installing directX or adding the file to your system directory.")
            Call DestroyGame

Problem : If dx7vb is not found, Mirage will crash on startup, this will never be called.

Code:

If CanMoveNow Then
                Call CheckMovement ' Check if player is trying to move
                Call CheckAttack   ' Check to see if player is trying to attack
            End If

Problem: Instead of responding to the keyboard being pressed, we need to check every time if a key's being presed 0.o, that's just stupid.

Code:

If GetPlayerInvItemNum(MyIndex, i) > 0 And GetPlayerInvItemNum(MyIndex, i)  ADMIN_DEVELOPER Then
            Call AddText(Text, Color)
        End If
    End If
    Debug.Print Text
End Sub

Problem : I forgot, sorry.

Code:

Public Function isStringLegal(ByVal sInput As String) As Boolean

Problem : If I recall right, checks letter for letter, instead of checking entire string once

Code:

Private Type NpcRec

Problem : Non-hostile NPC's have 10X more data than required, str/def/hp/exps/itemdrop etc

Code:

Public Sub HandleKeypresses(ByVal KeyAscii As Integer)

Problem : What's more likely? /command or admin message? Should check / before admin, changing order basicly.

[genusis]

hmm that's new stuff that DFA added since i stopped helping. You are right about allot of it, but i think the most important thing right now would be fixing the connection errors people gets from the client trying to connect to the server first.

Edit ... why is canattack in a movement If ?

[Joost]

This is a bug fix? Some kids asked me to give some input, so this post really is to show some things that're dead wrong.

[Joost]

Server-side :

Code:

Public Function PasswordOK(ByVal Name As String, ByVal Password As String) As Boolean

Compares Ucase, while a good p/w is bother upper and undercase.

Code:

CanAttackPlayer

Checks if something has 0 hp, while taht's impossible

Code:

Public Sub DestroyServer()

Does the same as LoadServer, unloading evreything, with a succesful DestroyServer, there's no need to Unload everything again on the next start-up

Code:

Function IsMultiIPOnline(ByVal IP As String) As Boolean

Poor coding.

Code:

Public Sub SendDataToMapBut(ByVal Index As Long, ByVal MapNum As Long, ByVal Data As String

Poor coding.
The entire HIGH_INDEX
Will look something like this : 1, 4, 7, 9, looping from 1 to 9. Instead, an array should be made holding those numbers.

Code:

Public Function CanPlayerBlockHit

Nothing wrong with this function, it's just that shields are completely useless.

[Jacob]

So, why don't you try to explain what's wrong with all these?

From what I'm seeing, there's nothing that bad really...

@Genusis: I would guess that the CheckAttack in the CanMoveNow check was a way to stop players from attacking while they were moving.

[Joost]

Pick one and I'll explain what's wrong with it. Not going to bother with others considering this project isn't related to the commnity, it's DFA's project, or so Ive been told.

[genusis]

well since DFA can't work on it right now since he has no internet at all and hasn't even tried to use the library internet it seems to have turned into a community project.

@dugor i can still attack while i move?

[Joost]

If we do this my way, meaning every change will be discussed before added (slow progress) I'll help out.

[GIAKEN]

That's how it should be done anyways.

[genusis]

when i was helping out DFA i made him test it before we released it but then you complained that i was helping out so i stopped and he then began to just release things without testing it hoping it worked.

But I'm in for testing to me it doesn't matter how long it takes as long as it works.

[Joost]

Of course, there're several talented people here. If they are able to comment on each other's code, the project will be stunning.

But of course, another option is to let Genusis do it.

[GIAKEN]

when i was helping out DFA i made him test it before we released it but then you complained that i was helping out so i stopped and he then began to just release things without testing it hoping it worked.

But I'm in for testing to me it doesn't matter how long it takes as long as it works.

Alright stop bringing that shit up over and over.

The point is, whether somebody checks it or not, no letting shitty code in. Now we all need to check it so this doesn't happen anymore.

[genusis]

hey i would not release anything unless i knew it was working =[ making me feel bad.

[genusis]

when i was helping out DFA i made him test it before we released it but then you complained that i was helping out so i stopped and he then began to just release things without testing it hoping it worked.

But I'm in for testing to me it doesn't matter how long it takes as long as it works.

Alright stop bringing that shit up over and over.

The point is, whether somebody checks it or not, no letting shitty code in. Now we all need to check it so this doesn't happen anymore.

Rodger. we should all check each other code regardless of skill.

[Joost]

Added comments to all mistakes I found.

[Matt]

Some of these are not useless. Well, one I seen. Lol.

Some of these are just basic security checks. Such as the one checking to make sure you're not higher than the max_items. A good packet hacker could get higher than that, in an attempt to crash the server. Which, he would crash the server, were that check not there.

[genusis]

Problem : I forgot, sorry.

Code: Select all
Public Function isStringLegal(ByVal sInput As String) As Boolean

the issue is it doesn't properly calculate the string data and is inaccurate.

[doomJoost1]

Some of these are not useless. Well, one I seen. Lol.

Some of these are just basic security checks. Such as the one checking to make sure you're not higher than the max_items. A good packet hacker could get higher than that, in an attempt to crash the server. Which, he would crash the server, were that check not there.

The check is client-side, making it useless. But this is exactly what I want, snippets of code that can be discussed by anyone before it gets added.

[Matt]

Some of these are not useless. Well, one I seen. Lol.

Some of these are just basic security checks. Such as the one checking to make sure you're not higher than the max_items. A good packet hacker could get higher than that, in an attempt to crash the server. Which, he would crash the server, were that check not there.

The check is client-side, making it useless. But this is exactly what I want, snippets of code that can be discussed by anyone before it gets added.

Yeah, you pointed that out on msn.