[CF-Devel] RE: [Crossfire-cvs] CVS commit: crossfire/server

[Mark Wedel]

David Hurst wrote:

>
     
      Ahh yes I forgot to mention that =o. Those are the functions njh believed were nop, or simply wrong. He used those comments to make sure people check before I change em to /*'s. (I believe they were used so Mark would notice ;).
     
     
 This really isn't the right approach.  How closely I look at checkins has many
factors - doing such a scheme will not necessarily result in me noticing them. 
Also, code checked in, while it does not need to be perfect, should be a final
version - something in general should not be checked in if it is known that it
will get re-written in a few days.

 If you want my opinion on a particular piece of code, much better to send me
(or the list) mail inquiring about it, and not use CVS as the discussiong
forum.  Using // is also very annoying to people who just want to grab CVS and
compile it and find out it does not compile - that does not convery a good image
to crossfire, and can be very annoying.

 Since it seems to have been sort of asked, I'll give a more in depth code
review.

!     int doesnt_slay = 1;

Rather than use negative terms, I prefer to try and keep things positive
(does_slay).  In general, this makes it easier to understand (!doesnt_slay for
example which results in a double negative, in general an english language no).

      if (hitter->slaying) {
!       if(
!        ((op->race != NULL) && 
!         strstr(hitter->slaying, op->race)
!         ) ||
!        (op->arch && 
!         (op->arch->name != NULL) && 
!         strstr(op->arch->name, hitter->slaying))
!        ) {
!       doesnt_slay = 0;
!       dam *= 3;
!       }

 I generally don't like that style of indentation - I know long lines need to be
broken apart, but single indentation makes something difficult to read.  I would
instead right that as

	if ( ((op->race != NULL) && strstr(hitter->slaying, op->race)) ||
           (op->arch &&  (op->arch->name != NULL) && strstr(op->arch->name,
hitter->slaying)) {
		does_slay=1;
		dam *=3;
	}


 That way the two cases of the logic are on different lines, making it easier to
read the two cases.

!     if(1) { // if (op->resist[attacknum] || 1) { // This is a nop!

 That should really be if the op->resist[attacknum].  I think the change was
sort of misguided - the logic of randomly choosing if the player takes damage if
the result after protection is less than 1 makes some sense.  But if 'dam' as
passed to the function is 0, I don't think it should be getting randomized to be
higher than it was originally.    The point to check for op->resist is that if
op->resist is zero, there is no point doing a bunch of extra work to come out
with the same result.

 The reason not to do that for all damage results is performance.  And
realistically, if the damage results are larger, the rounding makes less a
difference (eg, taking 60 vs 61 points is not a big deal, taking 0 vs 1 is).  A
good compromise might be if the damage is less than 5, then look into seeing if
it should get rounded up or down.

 There also appears to be some serious problems of attacknum (and the ATNR
values) getting mixed up in the AT_ values, eg:

!         switch(attacknum) {
            /* Player has been hit by something */
!         case AT_CONFUSION:
!           confuse_player(op,hitter,dam);

 (rest of them removed) - that code simply put is problem - your comparing
apples to oranges.  Plus, IMO, the if/else if block that was there was more
readable, and performance for the two is almost certainly the same.


 Stylistically, a few other notes:

 There is no need for the block of code that a case statement has to be in { }
braces unless you are declaring a variable in taht scope, eg,

!     case ATNR_DEATH:
!       {
!       deathstrike_player(op, hitter, &dam);
!       } break;


 can just as easily be
	case ATNR_DEATH:
		deathstrike_player(op, hitter, &dam);
		break;

 And if you are going to put the stuff in { } braces, the break statement should
really be on its own line, and not after a }.  In general, the only thing I find
acceptable to put after a } on the line is a comment saying what that brace
matches up with (useful for either long blocks of code or where there are many
layers, so it then makes it easier to see at what point a particular loop/block
may end.