JPHamlett
2010-08-21 20:45:50
KEY
+ : This is a positive mark that shows something you did well and should continue doing.
- : This is something that your script either seemed to lack or you for lack of a better word did poorly at showing.
No mark means that is neither good or bad it is just a suggestion that can be taken into consideration.
FEEDBACK
You use a lot of global variables I'm not gonna mark you down for that but just
fyi don't use that many of them cause they are very looked down upon by the rest of the community.
In you antiban you use a boolean but never use it anywhere in the script
so its kinda pointless also you only return true on two occasions now im not sure if that what you wanted to do
but to me it just dosn't make much sence.
This code
is kinda pointless as it is now what i would actually do is that I would have it return a integer and do something like this
Now that is just my personal preference and would save you some lines of coding later on.
In your deposit funtion not a very big deal but its what the "pros" do
you have
you could do
+ I like your failsafes
MAIN LOOP
- In this code
if the script does
then it wont free the dtms/bitmaps what I would do is
then what I would do is in you BYE procedure put a free in there some where just so it doesn't do any memery leaks
- Lots of global variables -- efficiency
- Lots of bitmaps -- efficiency
code]
procedure W(Info: string);
begin
if (not(Debug)) then
Exit;
Writeln('' + Info + '');
end;
- wasted function. could be:
- '' + Loc +'' = Loc. Stop that.
- all unnecessary. there's nothing else in that function. and.. really? 6 DTMs? that's a little haphazard.
- Don't call OpenTheBank in OpenTheBank. Shorten that function.
Tries := 0; set automatically
A for-to loop probably would have been more efficient in DepositInventory
- Be more consistent in WithdrawEss. Either close the bank and exit the function or break the loop and close the bank.
- Mouse(623, 108, 15, 15, True); Random click? Could be better.
- Why Exit Craft? There's nothing else in it.
+ Nice Progress Report.
- That's not necessary since you have if (not LoggedIn) in each function. Just log out on mistakes instead of passing it this far.
- DTMs/BMPs will never be freed.
+ Opening: Wow, nice opening. It explains nicely the instructions [which is really the important part] and the credits =].
Procedure Load; Amazing job getting all of those DTM's. However you could have just taken all of those variables that it uses from the global variables, and then just put them in the procedure variables. Another way would be to use casings. Procedure(Load : String); case Load of // begin =]. Ofcourse the way you did it I really find no problem with. I assume you knew what you were doing when you named the Banker DTM's .
+ Procedure W(Info : String); I love this. This is exactly what I like to see. Not only is it going to be useful for your debugging, it also shows me that your taking the time to make debugging easier for yourself and others. Great job.
Function Antiban : Boolean; Great job on actually putting Antiban into your script. You get props, but you have it set as a boolean. Now theres nothing wrong with this, but I don't see anything that comes from it. Also, you should make it FindNormalRandoms everytime this function is called rather then having a 7.5% chance whenever the function is called. Overall its decent ^^.
+ Procedure ProgressReport : Forward; So you know how to use forwarding, thats really good. Your taking a lot of steps into debugging, which is very good to see.
+ Procedure Bye(Explain, Loc: String); This is very, very good. Goes perfectly into Debugging. It also calculates your exp and levels gained =]. I would have put logout at the end of the function so it takes care of everything one shot. Great job.
Function AccurateBankonMMFinding: Boolean; Alright I like the name =] Niceness. There are a bit too many if..then's floating around though, so if I were you id probably store the dtm's into a TIntegerArray and then call them out using a For..To..Do loop. This way it would look much cleaner. Also, maybe use a failsafe like FindSymbol(), or try a go at TPA's. This function is nice though, and i love the concept.
+ Function DDTMs(PathNum: Integer): Integer; Its a beautiful thing when I see DDTM's being setup. While I would have used various Arrays to setup the subpoint variables and then a for..to..do loop to set up everything, I dont expect you to do that xD. Great job.
+ Function OpenTheBank : Boolean; Wow, nice function. Its definitely has failsafes to say the least ^^. Once again alot of If.Then's were used, so I would have probably used a while..do loop instead of all the checking for things and working backwards :P. Good good job though.
+ Function DepositInventory : Boolean; I love the fact your using Bitmaps, i have rarely seen it being used. Although you might want to FreeBitmap(); before you exit/finish your function. Overall its nice, I would have used DTMs since im more familiar with them and they seem extremely reliable, but hell, if you can use bitmaps like that your good bro.
+ Function WithdrawEss: Boolean; Love it, Everything checks out good, however I would use functions like WithdrawItem(); as a failsafe =]
+ Function FindRuins(var fx, fy: Integer): Boolean; Autocolor, plus you check the uptext =]. I would change the WriteLN's into W(); Good function.
+ Procedure SmartRun(Energy: Integer); Nice Job. Everything you need in a simple -self made- procedure =] excellent.
+ Function UniversalWalking(Num, MSR, MER, MRad, MRandX, MRandY: Integer): Boolean; Whoa that looks nice. I would take out all the findnormalrandoms, but overall it looks pretty nice. Great job using the variables for the procedure itself. I would have added in TPA's as well, but its fine the way it is.
+ Function BankToRuins(I: Integer): Boolean; Im really starting to like your variables that your putting into the functions/procedures . Everything looks alligned and together in the function, I like that you used i as point numbers, great job.
+ Function RuinsToBank(I: Integer): Boolean; Same thing except vice-versa I see. Nice Job. I would say you could combine these 2 walking functions into 1 big one, but it looks more organized with the script this way =].
+ Function FindAltar(var fx, fy: Integer): Boolean; AutoColor + Uptext Check. Clever, maybe a nice use of TPA's could have been demonstrated here.
+ Function FindPortal(var fx, fy: Integer): Boolean; An AutoColor that comes with a uptext check =] I like. Good job, once again I would replace the writeln() with W(); I would have used TPA's for this one but hey, it works =].
+ Function Craft: Boolean; Everything ties in here nicely. I dont like the static click you do with the mouse too much, but I guess its there for a good re-positioning. Great use of the Player arrays ^^.
+ Procedure ProgressReport; Couldn't ask for much more, It tells you the total amount of things the script did, as well as detailed individual player reports. Beautiful A++ Job on this.
Thnx HeavenGuard
Entry Number 2
Judges : J_Pizzle, Town, Cigue, Heavenguard
Technique (58.75/ 100)
(60 + 65 + 60 + 50 = 235 [/4] = 58.75)
+ Used if..then statements, for..to..do loops.
- Not many with..do statements, while..do loops.
+ Failsafes.
+ Autocolors.
- Used DTM's/Bitmaps excessively.
- Memory was not freed.
+ Marks were used.
Individuality (68.75/ 100)
(70 + 75 + 60 + 70 = 275 [/4] = 68.75)
+ Very good use of failsafes.
+ Debugging looks useful.
- Could have used more TPA usage rather then DTMs/Bitmaps/AC.
Follow All SRL Rules (80/ 100)
(85 + 85 + 60 + 90 = 320 [/4] = 80)
- An Endless Loop.
- Did not free memory.
+ Script disables if player is not logged in.
Creativity (70.25/ 100)
(80 + 85 + 70 + 46 = 281 [/4] = 70.25)
+ Universal Walking Method.
- Could have made it check for the talisman.
Efficiency (53.75/ 100)
(40 + 40 + 55 + 80 = 215 [/4] = 53.75)
- Memory could be freed with less use of Bitmaps/DTM's.
+ Failsafes were very good.
+ Has things to help debugging the script if necessary.
- Cases could have been easily used in places throughout the script.
Presentation (78.75/ 100)
(80 + 85 + 75 + 75 = 315 [/4] = 78.75)
+ Good Heading.
- Procedures/Functions were not commented.
+ Very detailed Progress Report.
+ Standards are good.
+ Organization.
Meeting All Requirements (92.5/ 100)
(100 + 100 + 80 + 90 = 370 [/4] = 92.5)
- Faulty AntiRandom.
+ Progress Report.
+ Antiban.
+ Multiplayer.
Added Extras (18.75/ 100)
(0 + 0 + 75 + 0 = 75 [/4] = 18.75)
- There are no added extras.
Total Score (521.5/ 800) = 65.19%
(58.75 + 68.75 + 80 + 70.25 + 53.75 + 78.75 + 92.5 + 18.75 = 521.5 [/8] = 65.19%)
+ : This is a positive mark that shows something you did well and should continue doing.
- : This is something that your script either seemed to lack or you for lack of a better word did poorly at showing.
No mark means that is neither good or bad it is just a suggestion that can be taken into consideration.
FEEDBACK
You use a lot of global variables I'm not gonna mark you down for that but just
fyi don't use that many of them cause they are very looked down upon by the rest of the community.
In you antiban you use a boolean but never use it anywhere in the script
so its kinda pointless also you only return true on two occasions now im not sure if that what you wanted to do
but to me it just dosn't make much sence.
This code
function AccurateBankonMMFinding: Boolean;
begin
if(not LoggedIn)then
Exit;
Result := True;
if(not(DTMRotated(Bankers5p1, x, y, MMX1, MMY1, MMX2, MMY2))) then
begin
if(not(DTMRotated(Bankers5, x, y, MMX1, MMY1, MMX2, MMY2))) then
begin
if(not(DTMRotated(Bankers4p1, x, y, MMX1, MMY1, MMX2, MMY2))) then
begin
if(not(DTMRotated(Bankers4, x, y, MMX1, MMY1, MMX2, MMY2))) then
begin
if(not(DTMRotated(Bankers3p1, x, y, MMX1, MMY1, MMX2, MMY2))) then
begin
if(not(DTMRotated(Bankers3, x, y, MMX1, MMY1, MMX2, MMY2))) then
begin
Bye('Could not find bankers on MM.', 'Lost');
Result := False;
end else
Exit;
end else
Exit;
end else
Exit;
end else
Exit;
end else
Exit;
end else
Exit;
end;
is kinda pointless as it is now what i would actually do is that I would have it return a integer and do something like this
function AccurateBankonMMFinding: integer;
begin
if(not LoggedIn)then
Exit;
if((DTMRotated(Bankers5p1, x, y, MMX1, MMY1, MMX2, MMY2))) then
begin
result := Bankers5p1;
exit;
end;
if((DTMRotated(Bankers5, x, y, MMX1, MMY1, MMX2, MMY2))) then
begin
result := Bankers5;
exit;
end;
end;
Now that is just my personal preference and would save you some lines of coding later on.
In your deposit funtion not a very big deal but its what the "pros" do
you have
begin
Mouse(x, y, 9, 9, False);
Wait(300 [COLOR="Lime"]+[/COLOR] Random(300));
ChooseOption('ll');
Wait(500 [COLOR="Lime"]+[/COLOR] Random(300));
if FindBitmapToleranceIn(AirRune, x, y, MIx1, MIy1, MIx2, MIy2, 20) then
you could do
begin
Mouse(x, y, 9, 9, False);
waitoption('ll', 500);
Wait(500 [COLOR="Lime"]+[/COLOR] Random(300));
if FindBitmapToleranceIn(AirRune, x, y, MIx1, MIy1, MIx2, MIy2, 20) then
+ I like your failsafes
MAIN LOOP
- In this code
until false;
FreeDTM(Bankers5p1);
FreeDTM(Bankers5);
FreeDTM(Bankers4p1);
FreeDTM(Bankers4);
FreeDTM(Bankers3p1);
FreeDTM(Bankers3);
FreeBitmap(RuneEssence);
FreeBitmap(AirRune);
FreeBitmap(AirTiara);
FreeBitmap(AirTalisman);
if the script does
BYE('blah', 'blah');
then it wont free the dtms/bitmaps what I would do is
procedure free;
begin
FreeDTM(Bankers5p1);
FreeDTM(Bankers5);
FreeDTM(Bankers4p1);
FreeDTM(Bankers4);
FreeDTM(Bankers3p1);
FreeDTM(Bankers3);
FreeBitmap(RuneEssence);
FreeBitmap(AirRune);
FreeBitmap(AirTiara);
FreeBitmap(AirTalisman);
end;
then what I would do is in you BYE procedure put a free in there some where just so it doesn't do any memery leaks
- Lots of global variables -- efficiency
- Lots of bitmaps -- efficiency
code]
procedure W(Info: string);
begin
if (not(Debug)) then
Exit;
Writeln('' + Info + '');
end;
- wasted function. could be:
procedure W(Info: string);
begin
if (Debug) then Writeln(Info);
end;
- '' + Loc +'' = Loc. Stop that.
end else
Exit;
end else
Exit;
end else
Exit;
end else
Exit;
end else
Exit;
end else
Exit;
- all unnecessary. there's nothing else in that function. and.. really? 6 DTMs? that's a little haphazard.
- Don't call OpenTheBank in OpenTheBank. Shorten that function.
Tries := 0; set automatically
A for-to loop probably would have been more efficient in DepositInventory
- Be more consistent in WithdrawEss. Either close the bank and exit the function or break the loop and close the bank.
- Mouse(623, 108, 15, 15, True); Random click? Could be better.
- Why Exit Craft? There's nothing else in it.
+ Nice Progress Report.
if (OpenTheBank) then
begin
if (DepositInventory) then
begin
if (WithdrawEss) then
begin
if (BankToRuins(1)) then
begin
if (Craft) then
begin
if (RuinsToBank(1)) then
- That's not necessary since you have if (not LoggedIn) in each function. Just log out on mistakes instead of passing it this far.
until false;
- DTMs/BMPs will never be freed.
+ Opening: Wow, nice opening. It explains nicely the instructions [which is really the important part] and the credits =].
Procedure Load; Amazing job getting all of those DTM's. However you could have just taken all of those variables that it uses from the global variables, and then just put them in the procedure variables. Another way would be to use casings. Procedure(Load : String); case Load of // begin =]. Ofcourse the way you did it I really find no problem with. I assume you knew what you were doing when you named the Banker DTM's .
+ Procedure W(Info : String); I love this. This is exactly what I like to see. Not only is it going to be useful for your debugging, it also shows me that your taking the time to make debugging easier for yourself and others. Great job.
Function Antiban : Boolean; Great job on actually putting Antiban into your script. You get props, but you have it set as a boolean. Now theres nothing wrong with this, but I don't see anything that comes from it. Also, you should make it FindNormalRandoms everytime this function is called rather then having a 7.5% chance whenever the function is called. Overall its decent ^^.
+ Procedure ProgressReport : Forward; So you know how to use forwarding, thats really good. Your taking a lot of steps into debugging, which is very good to see.
+ Procedure Bye(Explain, Loc: String); This is very, very good. Goes perfectly into Debugging. It also calculates your exp and levels gained =]. I would have put logout at the end of the function so it takes care of everything one shot. Great job.
Function AccurateBankonMMFinding: Boolean; Alright I like the name =] Niceness. There are a bit too many if..then's floating around though, so if I were you id probably store the dtm's into a TIntegerArray and then call them out using a For..To..Do loop. This way it would look much cleaner. Also, maybe use a failsafe like FindSymbol(), or try a go at TPA's. This function is nice though, and i love the concept.
+ Function DDTMs(PathNum: Integer): Integer; Its a beautiful thing when I see DDTM's being setup. While I would have used various Arrays to setup the subpoint variables and then a for..to..do loop to set up everything, I dont expect you to do that xD. Great job.
+ Function OpenTheBank : Boolean; Wow, nice function. Its definitely has failsafes to say the least ^^. Once again alot of If.Then's were used, so I would have probably used a while..do loop instead of all the checking for things and working backwards :P. Good good job though.
+ Function DepositInventory : Boolean; I love the fact your using Bitmaps, i have rarely seen it being used. Although you might want to FreeBitmap(); before you exit/finish your function. Overall its nice, I would have used DTMs since im more familiar with them and they seem extremely reliable, but hell, if you can use bitmaps like that your good bro.
+ Function WithdrawEss: Boolean; Love it, Everything checks out good, however I would use functions like WithdrawItem(); as a failsafe =]
+ Function FindRuins(var fx, fy: Integer): Boolean; Autocolor, plus you check the uptext =]. I would change the WriteLN's into W(); Good function.
+ Procedure SmartRun(Energy: Integer); Nice Job. Everything you need in a simple -self made- procedure =] excellent.
+ Function UniversalWalking(Num, MSR, MER, MRad, MRandX, MRandY: Integer): Boolean; Whoa that looks nice. I would take out all the findnormalrandoms, but overall it looks pretty nice. Great job using the variables for the procedure itself. I would have added in TPA's as well, but its fine the way it is.
+ Function BankToRuins(I: Integer): Boolean; Im really starting to like your variables that your putting into the functions/procedures . Everything looks alligned and together in the function, I like that you used i as point numbers, great job.
+ Function RuinsToBank(I: Integer): Boolean; Same thing except vice-versa I see. Nice Job. I would say you could combine these 2 walking functions into 1 big one, but it looks more organized with the script this way =].
+ Function FindAltar(var fx, fy: Integer): Boolean; AutoColor + Uptext Check. Clever, maybe a nice use of TPA's could have been demonstrated here.
+ Function FindPortal(var fx, fy: Integer): Boolean; An AutoColor that comes with a uptext check =] I like. Good job, once again I would replace the writeln() with W(); I would have used TPA's for this one but hey, it works =].
+ Function Craft: Boolean; Everything ties in here nicely. I dont like the static click you do with the mouse too much, but I guess its there for a good re-positioning. Great use of the Player arrays ^^.
+ Procedure ProgressReport; Couldn't ask for much more, It tells you the total amount of things the script did, as well as detailed individual player reports. Beautiful A++ Job on this.
Thnx HeavenGuard
Entry Number 2
Judges : J_Pizzle, Town, Cigue, Heavenguard
Technique (58.75/ 100)
(60 + 65 + 60 + 50 = 235 [/4] = 58.75)
+ Used if..then statements, for..to..do loops.
- Not many with..do statements, while..do loops.
+ Failsafes.
+ Autocolors.
- Used DTM's/Bitmaps excessively.
- Memory was not freed.
+ Marks were used.
Individuality (68.75/ 100)
(70 + 75 + 60 + 70 = 275 [/4] = 68.75)
+ Very good use of failsafes.
+ Debugging looks useful.
- Could have used more TPA usage rather then DTMs/Bitmaps/AC.
Follow All SRL Rules (80/ 100)
(85 + 85 + 60 + 90 = 320 [/4] = 80)
- An Endless Loop.
- Did not free memory.
+ Script disables if player is not logged in.
Creativity (70.25/ 100)
(80 + 85 + 70 + 46 = 281 [/4] = 70.25)
+ Universal Walking Method.
- Could have made it check for the talisman.
Efficiency (53.75/ 100)
(40 + 40 + 55 + 80 = 215 [/4] = 53.75)
- Memory could be freed with less use of Bitmaps/DTM's.
+ Failsafes were very good.
+ Has things to help debugging the script if necessary.
- Cases could have been easily used in places throughout the script.
Presentation (78.75/ 100)
(80 + 85 + 75 + 75 = 315 [/4] = 78.75)
+ Good Heading.
- Procedures/Functions were not commented.
+ Very detailed Progress Report.
+ Standards are good.
+ Organization.
Meeting All Requirements (92.5/ 100)
(100 + 100 + 80 + 90 = 370 [/4] = 92.5)
- Faulty AntiRandom.
+ Progress Report.
+ Antiban.
+ Multiplayer.
Added Extras (18.75/ 100)
(0 + 0 + 75 + 0 = 75 [/4] = 18.75)
- There are no added extras.
Total Score (521.5/ 800) = 65.19%
(58.75 + 68.75 + 80 + 70.25 + 53.75 + 78.75 + 92.5 + 18.75 = 521.5 [/8] = 65.19%)