Nothing wrong, yet Not right

ID:155544
 
Jun 2 2011, 7:31 am
 
Attack(mob/M as mob in oview(1))
            if(M.CanAttack==1)                   //otherwise...
                usr << "You attack [M]!"
                oview() << "[usr] attacks [M]!"
                var/damage = src.attack
                view() << "[damage] damage!"
                M.health -= damage
                M.DeathCheck()
                src.LevelCheck()
            if(M.newbie==1)
                usr << "You cannot attack a Newbie."
            if(M.Protect==1)
                usr << "This player is under somesort of protection."
            if(M.inTown==1)
                usr << "Violence will not be tolerated in town"
            else                //if M's HP are at or below 0...
                usr << "You are unable to Attack [M]"


just wrote this up real fast, comments?

I really want to be criticized constructively and want to hear improvements if able.
Jun 2 2011, 7:43 am
You are doing the damage check/damage first followed by a denial check... which is wrong.

You have to do the denial first THEN check if they can attack.

And you have to check if the HP is <= 0. And do you mean if M.CanAttack or src.CanAttack?

In stead of keep repeating the same snippet for all attacks, I suggest you make a procedure:
mob/proc/CanAttack(mob/M)
            if(M.newbie)  //  this checks if the variable is TRUE. FALSE is defined as == 0, "" or null. TRUE is anything else (ex: M.newbie = 2 is true, but when you do == 1, it'll result as a false check)  
                src << "You cannot attack a Newbie."  
                return 0
            if(M.Protect)
                src << "This player is under somesort of protection."  
                return 0
            if(M.inTown)
                src << "Violence will not be tolerated in town"  
                return 0
            if(M.HP <= 0)
                src << "[M.name] is dead! Do not desecrate!"  
                return 0
            if(!src.CanAttack)  //  checks for a FALSE value. So here, we are looking if the value == 0, "" or null
                src << "You cannot attack right now!"  
                return 0
            return 1   //  If none of the above are true, 1 will be returned instead of 0.


Attack(...)
   if(src.CanAttack(M))
        Do damage
I suggest you have a procedure which takes care of the damages, which calls the death and other related procedures - in case you need to tweak it later on
Jun 2 2011, 8:03 am
In response to GhostAnime
thanks, i know how to do that, just didnt think of doing that.

Valid input ;)

Anyone else have words to say?
Jun 2 2011, 9:29 am
As I look at that, I have a few other questions in addition to what GhostAnime pointed out.

What happens if M.CanAttack is 0?

Also, what is going on in LevelCheck? What I imagine is that attacking gives experience points, or something like that, to the attacker, and LevelCheck() processes that gain. But how does LevelCheck() know that an attack has just occurred, or who the target was?

As a matter of aesthetics, I would put the attack message and the damage amount on a single line.
Jun 2 2011, 4:27 pm
In response to Pepper2000
Thanks, applying that now.
Jun 3 2011, 8:02 am
In response to GhostAnime
also


I dont understand this part

Attack(...)
   if(src.CanAttack(M))
        Do damage


Why isnt it? mob then verb then attack?
Jun 3 2011, 9:42 am
In response to Komuroto
I believe because it was an example. He didn't fully write out the attack part because he expects you to know what to do with it.

That being said, I could be completely wrong.
Jun 3 2011, 9:51 am
No big deal but "mob/M as mob in oview(1))" is redundant.
Jun 3 2011, 11:10 am
In response to XeroXen
Xero: You were right on the dot ^_^'

OP: The problem with locating a mob with an argument is the popup that appears. If you want to avoid that, by getting the mob directly in front of you, you can use this: 
Attack()  //  No argument
   var/mob/M = locate() in get_step(src,src.dir)  //  Finds a /mob directly ahead
   if(!M) return  //  No mob found
   if(M.CanAttack...etc...
Jun 3 2011, 3:25 pm
In response to Turles
so...?

how would i phrase it then?
Jun 3 2011, 4:05 pm
In response to Komuroto
mob/M
Jun 3 2011, 4:28 pm
In response to LordAndrew
so no oview needed?
Jun 3 2011, 6:07 pm
In response to Komuroto
get_step() returns the /turf in the direction you specify (in the example shown, it gets the /turf ahead of the person's direction)

oview(source, X) gets the /turf around the source X distance (this includes behind the person). In the snippet, if you replace get_step() with oview(), locate() would return the first /mob it finds; to affect all /mob in the oview(), you would need to use for()