D21893: magic-hat, add multiple datasets

[Aman Kumar Gupta]

amankumargupta added a comment.


  lots of trailing whitespaces.

INLINE COMMENTS

> MagicHat.qml:68
> +            property var levels: activity.datasetLoader.item.data
> +            property var range: activity.datasetLoader.item.range
>              property alias bonus: bonus

"calculation_range" would be better?

> magic-hat.js:66
>      }
> -
> +    coefficientsNeeded = items.range / 30 <= 1 ? false : true
>      for(var j=0; j<3; j++) {

don't use hardcoded values. Keep it in a variable and use that variable it. Will be more flexible and easy for anyone to find modify it he wants.
Also, please add a comment mentioning why items.range / 30 <= 1 is done. Improves readability of the code.

> magic-hat.js:67
> +    coefficientsNeeded = items.range / 30 <= 1 ? false : true
>      for(var j=0; j<3; j++) {
>          items.repeatersList[0].itemAt(j).initStars()

missing spaces between operators and operands.

> magic-hat.js:86
>      }
> +    var subtractor = mode === "minus" ? 0 : 1
> +    numberOfStars[0] = items.levels[currentLevel].maxStars[0] > 0 ? getRandomInt(items.levels[currentLevel].minStars[0], (items.levels[currentLevel].maxStars[0] / coffieients[0]) - subtractor) : 0

Put the expressions in parenthesis. Same comment for the ones below.
Improves code readability.

> magic-hat.js:130
> +    for(var i=0; i<3; i++) {
> +        items.repeatersList[0].itemAt(i).coefficientVisible = visibility
> +        items.repeatersList[1].itemAt(i).coefficientVisible = visibility

Why not use a nested loop from 0 to 2?

> magic-hat.js:142
> +        for(var i=0; i<3; i++) {
> +            items.repeatersList[0].itemAt(i).starsColor = colorValue
> +            items.repeatersList[1].itemAt(i).starsColor = colorValue

nested loop would be better

> magic-hat.js:157
>  function verifyAnswer() {
> -    if(numberOfUserStars[0] === nbStarsToCount[0] &&
> -       numberOfUserStars[1] === nbStarsToCount[1] &&
> -       numberOfUserStars[2] === nbStarsToCount[2]) {
> -        items.bonus.good("flower")
> +    if(items.range / 30 <= 1) {
> +        if(numberOfUserStars[0] === nbStarsToCount[0] &&

The same thing for this formula like I said previously for the one above.

> magic-hat.js:166
>      } else {
> -        items.bonus.bad("flower")
> +        var userStars = numberOfUserStars[0] * answerCofficients[0] + numberOfUserStars[1] * answerCofficients[1] +
> +                                                    numberOfUserStars[2] * answerCofficients[2];

Can you rename these two variables to more understandable names (this one and the one below)?

REPOSITORY
  R2 GCompris

REVISION DETAIL
  https://phabricator.kde.org/D21893

To: AkshayCHD, #gcompris_improvements, timotheegiet
Cc: amankumargupta, timotheegiet, #gcompris_improvements, kde-edu, #gcompris, sanjayshetty, parimalprasoon, harrymecwan, ganeshredcobra, nityanandkumar, echarruau, andreask, rahulyadav, narvaez, scagarwal, apol, jjazeix, bcoudoin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20190622/5d5a67b4/attachment-0001.html>