[Kde-games-devel] Review Request 111662: Hard AI for KSquares

[Albert Astals Cid]

> On Aug. 14, 2013, 9:42 p.m., Albert Astals Cid wrote:
> > Hi Tom, thanks a lot for the contribution!
> > 
> > Sadly it seems that Matt is not answering so it'll have to be someone without much knowledge on ksquares doing the review.
> > 
> > I'm going to be a bit annoying i know, but do you think you could try to isolate the new code so that it's only executed when we are on hard mode? This way it'd be easier for someone with no knowledge at all on ksquares to accept the change, based in the fact that if the patch is broken it'll only break the new hard mode, and not the existing ones.
> > 
> > I tried looking at if the code already does that but it seems that it's a bit "convoluted", but if you want to explain to me that the other difficulties don't get changed by the new code i'm also open to that.
> 
> Tom Vincent Peters wrote:
>     Hello Albert, thank you very much for taking the time to review this.
>     
>     I assume that it will be more annoying for you to look into any form of this code than for me to change it in a way that makes reviewing it easier. So I'm wondering: Should I rather leave the original code untouched, or should I avoid code duplication?
>     
>     Leaving the original code intact would mean that I either introduce at least one new method in the aiController class or (less preferred) create a whole new class that looks almost like the aiController that is in Diff revision 2 (only without premature return statements for easy/medium levels). 
>     
>     Avoiding code duplication would mean some more work but would surely be possible by saving the results of the calculations of existing AI code in class variables and then reusing these for further calculations. This actually is something I thought about before when I considered introducing new classes that  inherit each other. At that time I decided against it to avoid unnecessary structual changes and continued on the path of the code.
>     
>     I'm not 100% sure that my changes don't affect the other difficulties. Thus it would be unreasonable to argue for keeping the code the way it is. I think it is saner to separate old and new code.
>     
>     To help you with reviewing: In my old review request there is a small change that puts the internal line index in the tool-tip of each line. These help a lot with interpreting the debug messages in the AI code.

code duplication is bad too. I was hoping you could basically add a few "if (difficulty == hard)" around so that it's clearer only for hard that is executed so the easy/medium code is [almost] untouched (even if some new parameters are added to some methods or something)

Is this possible at all?


- Albert


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111662/#review37801
-----------------------------------------------------------


On July 25, 2013, 11:28 p.m., Tom Vincent Peters wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111662/
> -----------------------------------------------------------
> 
> (Updated July 25, 2013, 11:28 p.m.)
> 
> 
> Review request for KDE Games and Matt Williams.
> 
> 
> Description
> -------
> 
> Hard AI for KSquares, 
> originally submitted about a year ago: https://svn.reviewboard.kde.org/r/6951/
> The new AI level is able to do hard hearted handouts and doublecrosses.
> This time there are only minimal UI changes that are required for the new AI level (and some credits).
> It seems that the medium ai uses hard hearted handouts more often than before (doesn't seem to influence the strength of the medium AI, see "Testing Done")
> 
> 
> Diffs
> -----
> 
>   src/prefs_ai.ui 9774aed 
>   src/ksquareswindow.cpp 092c795 
>   src/main.cpp 58184b6 
>   src/aicontroller.h beaafa8 
>   src/aicontroller.cpp 0bff5e6 
> 
> Diff: http://git.reviewboard.kde.org/r/111662/diff/
> 
> 
> Testing
> -------
> 
> 100 autoplay games medium against hard
> a lot of games against hard ai
> some games against medium ai
> Test whether medium ai code changed as a side effect of the new code: https://git.reviewboard.kde.org/r/111705/
> 
> 
> Thanks,
> 
> Tom Vincent Peters
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-games-devel/attachments/20130815/49eced5f/attachment-0001.html>