Jump to content

  •  

Bug Tracker Migration

June 3rd
Good news everyone! The staff has decided that it is time to slowly kill off this Bug Tracker. We will begin the process of slowly migrating from this Bug Tracker over to our Github Issues which can be found here: https://github.com/HerculesWS/Hercules/issues

Over the next couple of days, I will be closing off any opportunity to create new reports. However, I still will keep the opportunity to reply to existing Bug Reports. Doing this will allow us to slowly fix any bug reports we have listed here so that we can easily migrate over to our Issue Tracker.

Update - June 7th 2015: Creating new bug posts has been disabled. Please use our https://github.com/HerculesWS/Hercules/issues tracker to post bugs. Users are still able to reply to existing bug posts.

- Administration

Issue Information

Issue Confirmations

  • Yes (0)No (0)
Photo

command code change review and suggestions

Posted by Hercules Bot on 18 December 2008 - 12:12 PM

Originally posted by theultramage
http://www.eathena.w...er&showbug=2549

http://svn.eathena.w...hangeset/13403/
QUOTE("SketchyPhoenix")
Removed charcommand code while allowing atcommand code to support its functionality.
Charcommands still retain their '#' symbol but now looks for a character name as the first parameter instead of last.
Atcommand configs now support charcommand level configurations by comma seperation (e.g. 60,99)
As a result of this, all atcommands that don't affect multiple users already (@kickall, @doom, @mapexit) are capable of remote usage.


Problems I see in that change...
  1. change of file structure - forces people to re-do all their configs just like that
  2. backwards compatibility - I see you added code to handle the original file format.
    CODE
    -        if( sscanf(line, "%1023[^:]:%1023s", w1, w2) != 2 )
    +            if( (sscanf(line, "%1023[^:]:%1023[^,],%1023s", w1, w2, w3)) != 3  
    +            && ( sscanf(line, "%1023[^:]:%1023s", w1, w2) != 2 && strcmpi(w1, "import") != 0 ) )
    ...
    +                p->level2 = atoi(w3);
    +                p->level2 = cap_value(p->level2, 0, 100);
    But there's a problem, you don't erase 'w3' when the old format is read. Thus the value becomes 0 (in case of initial junk), or the previously read value. Normally you'd erase w3, or use some if-else scheme. Anyways, with the old format the p->level2 value should be assigned the same value as p->level, yes?
  3. @adjcmdlvl is unable to distinguish between the two level values and the user must have enough permissions to set both at once; this may prove restrictive if anyone beside administrators wants to use this command (not sure if such a thing is a good idea, though)
  4. in is_atcommand(), you recycled the atcmd_output[] and atcmd_temp[] buffers for temporary variable storage; use proper local variables instead.
  5. you messed up the indentation in atcommand_commands() (at the end of atcommand.c)
  6. atcommand_commands() might be returning messy output now, basically the same thing twice; maybe some delimiting lines to separate atcommands and charcommands in the output might help.
  7. you did not add a config setting to set the '#' charcommand symbol; also, read the documentation at the beginning of atcommand_athena.conf and see if anything's wrong with it.
  8. you introduced a new parameter, 'charname[24]' to the command processing, used only for logging purposes; I'd re-think the usefulness of this solution.
  9. now that the same code supports both at(@)-commands and char(#)-commands, should its name be left as 'at'(@)? 'gmcommand' is the original name I believe (but maybe changing the naming convention might upset a lot of people)


This post has been edited by theultramage: Dec 18 2008, 04:12 AM

changed status to: Done

atcommand config was rewritten since