ronantakizawa/codereview-bench huggingface dataset:
Repository: CryptoBlades/cryptoblades
File: frontend/src/components/smart/CharacterList.vue (Vue)

Code before:
  methods: {
    ...mapActions(['fetchCharacters']),

    getCharacterArt,

    saveFilters() {
      sessionStorage.setItem('character-levelfilter', this.levelFilter);
      sessionStorage.setItem('character-elementfilter', this.elementFilter);

      if(this.isMarket) {
        sessionStorage.setItem('character-price-order', this.priceSort);
      }
      this.$emit('character-filters-changed');
    },

    clearFilters() {
      sessionStorage.clear();

      this.elementFilter = '';
      this.levelFilter = '';
      this.priceSort = '';

      this.$emit('character-filters-changed');
    },

    priceSortText(type) {
      if (type === '1') {
        return 'Price: Low -> High';
      } else if (type === '-1') {
        return 'Price: High -> Low';
      }
      return 'Any';
    },
  },

  components: {
    CharacterArt,
  },

  mounted() {
    this.levelFilter = sessionStorage.getItem('character-levelfilter') || '';
    this.elementFilter = sessionStorage.getItem('character-elementfilter') || '';
    if(this.isMarket) {
      this.priceSort = sessionStorage.getItem('character-price-order') || '';
    }
  }
};
</script>

<style scoped>
.character-list {

Review comment (suggestion):
this could be done a bit better:

```
const sorts = [
 { name: 'Any', dir: 0 },
 { name: 'Price: Low -> High', dir: 1 },
 { name: 'Price: High -> Low', dir: -1 },
]
```
then use that. then we can add more sorts later without so much boilerplate.

Code after:

  watch: {
    async characterIdsToDisplay(characterIds) {
      await this.fetchCharacters(characterIds);
    }
  },

  methods: {
    ...mapActions(['fetchCharacters']),

    getCharacterArt,

    saveFilters() {
      localStorage.setItem('character-levelfilter', this.levelFilter);
      localStorage.setItem('character-elementfilter', this.elementFilter);

      if(this.isMarket) {
        sessionStorage.setItem('character-price-order', this.priceSort);
      }
      this.$emit('character-filters-changed');
    },

    clearFilters() {
      sessionStorage.clear();

      this.elementFilter = '';
      this.levelFilter = '';
      this.priceSort = '';

      this.$emit('character-filters-changed');
    },
  },

  components: {
    CharacterArt,
  },

  mounted() {
    this.levelFilter = sessionStorage.getItem('character-levelfilter') || '';
    this.elementFilter = sessionStorage.getItem('character-elementfilter') || '';
    if(this.isMarket) {
      this.priceSort = sessionStorage.getItem('character-price-order') || '';
    }
  }
};
</script>

<style scoped>
.character-list {
  list-style-type: none;
  margin: 0;
Repository: CryptoBlades/cryptoblades
File: frontend/src/views/Blacksmith.vue (Vue)

Code before:
    },

    isWeaponRare() {
      return this.getWeaponToBurn().stars >= 3;
    },

    isWeaponReforged() {
      return this.getWeaponToBurn().bonusPower > 0;
    },

    getWeaponToBurn() {
      return this.ownWeapons.find(x => x.id === this.burnWeaponId);
    },

    getCurrentListofWeapons(){
      this.ownedWeaponIds.forEach(x => {
        this.currentListofWeapons.push(x);
      });
    },

    viewNewWeapons(){
      this.newForged = [];
      this.ownedWeaponIds.forEach(x => {
        this.newForged.push(x);
      });
      if(this.x1Forge === true){
        this.newForged.splice(0,this.ownedWeaponIds.length-2);
      }
      else if(this.x10Forge === true){
        this.newForged.splice(0,this.ownedWeaponIds.length-11);
      }

      // eslint-disable-next-line no-constant-condition
      if (this.newForged.length > 0 && !this.onError){
        this.spin = true;
        this.$refs['new-weapons'].show();

        setTimeout(() => {
          this.spin = false;
        }, 10000);
      }


    },

    async onReforgeWeapon() {
      try {
        await this.reforgeWeapon({
          burnWeaponId: this.burnWeaponId,
          reforgeWeaponId: this.reforgeWeaponId,
        });

Review comment (suggestion):
never check against true, just check if(this.x1Forge). perhaps instead, do `this.forgeAmount = 1 or 10` and check that. but really, this should be a number and you should do the math based on that number.

Code after:
      this.newForged = [];
      this.ownedWeaponIds.forEach(x => {
        this.newForged.push(x);
      });

      this.newForged.splice(0, this.ownedWeaponIds.length - offset + 1);


      // eslint-disable-next-line no-constant-condition
      if (this.newForged.length > 0 && !this.onError){
        this.spin = true;
        this.$refs['new-weapons'].show();

        setTimeout(() => {
          this.spin = false;
        }, 10000);
      }


    },

    async onReforgeWeapon() {
      try {
        await this.reforgeWeapon({
          burnWeaponId: this.burnWeaponId,
          reforgeWeaponId: this.reforgeWeaponId,
        });

        this.burnWeaponId = null;
      } catch (e) {
        console.error(e);
        this.$dialog.notify.error('Could not forge sword: insuffucient funds or transaction denied.');
      }
    }
  },

  components: {
    WeaponGrid,
    BigButton,
    WeaponIcon,
    BModal,
    NftList,
  },
};
</script>

<style scoped>

.new-weapon-header-text{
   color: #9e8a57;
  font-size: 34px;
Repository: CryptoBlades/cryptoblades
File: frontend/src/components/smart/PvPArenaSummary.vue (Vue)

Code before:
        rank: null,
        element: null,
      }
    },
    activeWeaponWithInformation: {
      default: {
        weaponId: null,
        information: {}
      }
    },
    activeShieldWithInformation: {
      default: {
        shieldId: null,
        information: {}
      }
    },
    duelHistory: {
      default: []
    }
  },

  data() {
    return {
      tab: 0,
      dayjs,
      loading: true,
      duelQueue: [],
      isCharacterInDuelQueue: false,
    };
  },

  computed: {
    ...mapState(['currentCharacterId', 'contracts', 'defaultAccount']),
  },

  methods: {
    ...mapActions([
      'withdrawFromArena',
      'getDuelQueue'
    ]),

    setTab(tabNumber) {
      this.tab = tabNumber;
    },

    handleErrorMessage(value, errorMessage, returnedMessage) {
      if (value.includes(`reverted with reason string '${errorMessage}'`)) {
        return this.$dialog.notify.error(returnedMessage);
      }
      return 'There has been an error. Try again.';
    },

Review comment (bug):
By default loadings often are initialized to `false`, then on the action start, switched to `true`, after the action (often in `finally` block for actions that may fail) switched back to `false` again

Code after:
        rank: null,
        element: null,
      }
    },
    activeWeaponWithInformation: {
      default: {
        weaponId: null,
        information: {}
      }
    },
    activeShieldWithInformation: {
      default: {
        shieldId: null,
        information: {}
      }
    },
    duelHistory: {
      default: []
    }
  },

  data() {
    return {
      tab: 0,
      dayjs,
      loading: false,
      duelQueue: [],
      isCharacterInDuelQueue: false,
    };
  },

  computed: {
    ...mapState(['currentCharacterId', 'contracts', 'defaultAccount']),
  },

  methods: {
    ...mapActions([
      'withdrawFromArena',
      'getDuelQueue'
    ]),

    setTab(tabNumber) {
      this.tab = tabNumber;
    },

    handleErrorMessage(value, errorMessage, returnedMessage) {
      if (value.includes(`reverted with reason string '${errorMessage}'`)) {
        return this.$dialog.notify.error(returnedMessage);
      }
      return this.$dialog.notify.error(i18n.t('pvp.genericError'));
    },
Repository: CryptoBlades/cryptoblades
File: frontend/src/components/smart/PvPArenaSummary.vue (Vue)

Code before:

    async handleEnterArenaClick() {
      this.$emit('enterMatchMaking');
    },

    async leaveArena() {
      this.loading = true;

      if (this.isCharacterInDuelQueue) {
        alert(i18n.t('pvp.currentlyInDuel'));
        this.loading = false;
        return;
      }

      try {
        await this.withdrawFromArena(this.currentCharacterId);

        this.$emit('leaveArena');
      } catch (err) {
        console.log('leave arena error: ', err.message);

        this.handleErrorMessage(err.message, 'Char not in arena', 'The character is not in the arena');
        this.handleErrorMessage(err.message, 'Defender duel in process', 'Duel already in process');
      }

      this.loading = false;
    },
  },

  async created() {
    this.loading = true;

    this.duelQueue = await this.getDuelQueue();

    if (this.duelQueue.includes(`${this.currentCharacterId}`)) {
      this.isCharacterInDuelQueue = true;
    }

    this.loading = false;
  }
};
</script>

<style scoped lang="scss">
.wrapper {
  display: flex;
  flex-direction: column;
  min-height: 100vh;
}
span, p, li, button, a {
  font-family: 'Roboto';

Review comment (bug):
I would place it in `finally` block, just for the sake of keeping it tidy, also, not sure if this gets called on try fail

Code after:

    async handleEnterArenaClick() {
      this.$emit('enterMatchMaking');
    },

    async leaveArena() {
      this.loading = true;

      if (this.isCharacterInDuelQueue) {
        alert(i18n.t('pvp.currentlyInDuel'));
        this.loading = false;
        return;
      }

      try {
        await this.withdrawFromArena(this.currentCharacterId);

        this.$emit('leaveArena');
      } catch (err) {
        console.log('leave arena error: ', err.message);

        this.handleErrorMessage(err.message, 'Char not in arena', i18n.t('pvp.charNotInArena'));
        this.handleErrorMessage(err.message, 'Defender duel in process', i18n.t('pvp.duelInProcess'));
      } finally {
        this.loading = false;
      }
    },
  },

  async created() {
    this.loading = true;

    try {
      this.duelQueue = await this.getDuelQueue();

      if (this.duelQueue.includes(`${this.currentCharacterId}`)) {
        this.isCharacterInDuelQueue = true;
      }
    } catch (err) {
      console.log('get duel queue error: ', err.message);
      this.handleErrorMessage();
    }

    this.loading = false;
  }
};
</script>

<style scoped lang="scss">
.wrapper {
  display: flex;
Repository: CryptoBlades/cryptoblades
File: migrations/164_cb_upgrade.js (JavaScript)

Code before:
const { upgradeProxy } = require("@openzeppelin/truffle-upgrades");
const CryptoBlades = artifacts.require("CryptoBlades");

module.exports = async function (deployer, network) {
    if (network === "development"
    || network === "development-fork"
    || network === 'bsctestnet'
    || network === 'bsctestnet-fork'
    || network === 'hecotestnet'
    || network === 'okextestnet'
    || network === 'polygontestnet'
    || network === 'avaxtestnet'
    || network === 'avaxtestnet-fork' || network === 'auroratestnet') {
        await upgradeProxy(CryptoBlades.address, CryptoBlades, { deployer });
    }
};

Review comment (suggestion):
Migration 163 won't work, because game contract needs upgrading before `setTokensPricesAddress` can be called.
We should have this upgrade in migration 163.

Code after:
const { upgradeProxy } = require("@openzeppelin/truffle-upgrades");
const CryptoBlades = artifacts.require("CryptoBlades");
const TokensManager = artifacts.require("TokensManager");

module.exports = async function (deployer, network) {
    if (network === "development"
    || network === "development-fork"
    || network === 'bsctestnet'
    || network === 'bsctestnet-fork'
    || network === 'hecotestnet'
    || network === 'okextestnet'
    || network === 'polygontestnet'
    || network === 'avaxtestnet'
    || network === 'avaxtestnet-fork' || network === 'auroratestnet') {
        const game = await upgradeProxy(CryptoBlades.address, CryptoBlades, { deployer });

        const tokensManager = await TokensManager.deployed();

        await game.grantRole(await game.GAME_ADMIN(), tokensManager.address);
    }
};
Repository: CryptoBlades/cryptoblades
File: contracts/TokensManager.sol (Solidity)

Code before:
        uint256 expectedTokens = game.fight(char, wep, target, fightMultiplier);

        uint256 offset = ABDKMath64x64.mulu(getSkillToNativeRatio(), expectedTokens.mul(combatTokenChargePercent));

        require(msg.value == offset, 'Offset error');
    }

    function retrieve(address addressToTransferTo, uint256 amount) external restricted {
        payable(addressToTransferTo).transfer(amount);
    }

    function getSkillToNativeRatio() public view returns (int128) {
        return ABDKMath64x64.divu(skillTokenPrice, tokenPrice);
    }

    function setTokenPrice(uint256 price) external restricted {
        tokenPrice = price;
    }

    function setSkillTokenPrice(uint256 price) external restricted {
        skillTokenPrice = price;
    }

    function setCombatTokenChargePercent(uint256 percent) external restricted {
        combatTokenChargePercent = percent;
    }
}

Review comment (performance):
A little sidebar here, not necessary to change anything now but, for the future:
The bot that updates the native token and skill token prices might want to send updates for both at once, in which case it would make sense for a function that takes parameters for both. This is only really relevant for cutting costs on expensive networks, and only if the rate of updates is frequent enough to make a difference.

Code after:
    }

    function fight(uint256 char, uint256 wep, uint32 target, uint8 fightMultiplier) external payable {
        (uint256 tokens, uint256 expectedTokens) = game.fight(char, wep, target, fightMultiplier);

        uint256 offset = ABDKMath64x64.mulu(getSkillToNativeRatio(), expectedTokens.mul(combatTokenChargePercent));

        require(msg.value == offset, 'Offset error');

        if (tokens == 0) {
            payable(msg.sender).transfer(msg.value);
        }
    }

    function retrieve(address addressToTransferTo, uint256 amount) external restricted {
        payable(addressToTransferTo).transfer(amount);
    }

    function getSkillToNativeRatio() public view returns (int128) {
        return ABDKMath64x64.divu(skillTokenPrice, tokenPrice);
    }

    function setTokenPrice(uint256 price) external restricted {
        tokenPrice = price;
    }

    function setSkillTokenPrice(uint256 price) external restricted {
        skillTokenPrice = price;
    }

    function setCombatTokenChargePercent(uint256 percent) external restricted {
        combatTokenChargePercent = percent;
    }
}
Repository: CryptoBlades/cryptoblades
File: frontend/src/components/smart/PvPArenaPreparation.vue (Vue)

Code before:
  font-family: 'Roboto';
  font-size: .75rem;
  color: #b4b0a7;
  background-color: #151515;
  border-color: transparent;
  border-radius: 0.2rem;
  width: 6rem;
}
.selectFilter:first-of-type {
  width: 4rem
}
.noWeaponsOrShields {
  display: flex;
  margin: 0 auto;
  font-family: 'Roboto';
  color: #b4b0a7;
  font-size: 0.75rem;
  margin-top: 1rem;
}
.mainWrapper {
  display: flex;
  width: 100%;
  margin: 0 auto;
  justify-content: space-between;
}
@media  all and (max-width: 1057px) {
  .mainWrapper{
    flex-direction: column;
  }
  .arenaSignup .bottomList ul li{
    white-space: inherit !important;
  }
}
.title {
  margin-bottom: 0.75rem;
  color: #cec198;
  font-size: 1.25rem;
  font-family: 'Trajan';
  line-height: 1.75rem;
  padding: 0;
  text-transform: uppercase;
}
.arenaSignup {
  p {
    margin-bottom: 0;
    color: #b4b0a7;
    font-size: 1rem;
    line-height: 1.5rem;
  }
  .top {
    display: flex;

Review comment (suggestion):
Can you try adding flex-wrap:wrap to .mainWrapper and setting a min-width / width to the character image? (currently the image just scales down when the screen gets smaller and the wrapper doesn't wrap)
This way we can avoid the media query, which will be invalid if any other component changes

Code after:
  font-family: 'Roboto';
  font-size: .75rem;
  color: #b4b0a7;
  background-color: #151515;
  border-color: transparent;
  border-radius: 0.2rem;
  width: 6rem;
}
.selectFilter:first-of-type {
  width: 4rem
}
.noWeaponsOrShields {
  display: flex;
  margin: 0 auto;
  font-family: 'Roboto';
  color: #b4b0a7;
  font-size: 0.75rem;
  margin-top: 1rem;
}
.mainWrapper {
  display: flex;
  width: 100%;
  margin: 0 auto;
  justify-content: space-between;
  flex-wrap: wrap;
}
.title {
  margin-bottom: 0.75rem;
  color: #cec198;
  font-size: 1.25rem;
  font-family: 'Trajan';
  line-height: 1.75rem;
  padding: 0;
  text-transform: uppercase;
}
.arenaSignup {
  p {
    margin-bottom: 0;
    color: #b4b0a7;
    font-size: 1rem;
    line-height: 1.5rem;
  }
  .top {
    display: flex;
    margin-top: 1.5rem;
    margin-bottom: 1rem;
    vertical-align: middle;
    align-items: center;
    .circle {
      display: flex;
      width: 1.75rem;
Repository: CryptoBlades/cryptoblades
File: frontend/src/components/smart/AdminTabs/SkillAdmin.vue (Vue)

Code before:
        || !isValidWeb3Address(this.transferModel.sourceWallet || ''))
        || this.skillAllowance < this.transferModel.amount
        ||  this.transferModel.amount <= 0;
    }
  },
  methods: {
    ...mapActions([
      'approveSkillTransfer',
      'transferSkill',
      'getSkillAllowance',
      'fetchSkillBalance'
    ]),
    async onTransfer() {
      this.loading = true;
      try {
        await this.transferSkill({sourceAddress: this.transferModel.sourceWallet,
          receiverAddress:  this.transferModel.receiverAddress, amount:  this.transferModel.amount });

        await this.fetchSkillBalance();

        this.resetFormFields();
      } catch(e) {
        console.error(e);
      }

      this.loading = false;
    },
    resetFormFields() {
      this.transferModel = {
        sourceWallet: null,
        receiverAddress: null,
        amount: 0
      };
    }
  },
});
</script>

<style scoped>
</style>


Review comment (refactor):
Please move this to `finally` block, because this line won't be executed if an error occurs on line `79` or `82`, always disable loading state in `finally` block

Code after:
      return (!isValidWeb3Address(this.transferModel.receiverAddress || '')
        || !isValidWeb3Address(this.transferModel.sourceWallet || ''))
        || this.skillAllowance < this.transferModel.amount
        || this.transferModel.amount <= 0;
    }
  },
  methods: {
    ...mapActions([
      'approveSkillTransfer',
      'transferSkill',
      'getSkillAllowance',
      'fetchSkillBalance'
    ]),
    async onTransfer() {
      this.loading = true;
      try {
        await this.transferSkill({sourceAddress: this.transferModel.sourceWallet,
          receiverAddress:  this.transferModel.receiverAddress, amount:  this.transferModel.amount });

        await this.fetchSkillBalance();

        this.resetFormFields();
      } catch(e) {
        console.error(e);
      } finally {
        this.loading = false;
      }
    },
    resetFormFields() {
      this.transferModel = {
        sourceWallet: null,
        receiverAddress: null,
        amount: 0
      };
    }
  },
});
</script>

<style scoped>
</style>
Repository: DiceDB/dice
File: tests/command_count_test.go (Go)

Code before:
package tests

import (
	"fmt"
	"testing"

	"gotest.tools/v3/assert"
)

func TestCommandCOUNT(t *testing.T) {
	commandsCount := 40

	subscriber := getLocalConnection()

	responseValue := fireCommand(subscriber, "COUNT")
	responseCount  := responseValue.(int64)
	if responseValue == nil {
		t.Fail()
	}

	assert.Assert(t, responseCount == int64(commandsCount),
		fmt.Sprintf("Unexpected number of CLI commands found. %d expected, %d found", commandsCount, responseCount))
}

func call(howmany int, t *testing.B) {
	subscriber := getLocalConnection()
	for i := 0; i < howmany; i++ {
		rp := fireCommandAndGetRESPParser(subscriber, "COUNT")
		if rp == nil {
			t.Fail()
		}
	}
}

func BenchmarkCountCommand200(t *testing.B) {
	call(200, t)
}

func BenchmarkCountCommand2000(t *testing.B) {
	call(2000, t)
}

Review comment (bug):
instead of typecasting this to `int64` you can set commandCount as `int64` while declaring. Also, is this a good idea to test this? So, every time you add a new command, this test is bound to fail. Can you confirm how is Redis codebase testing this?

Code after:
package tests

import (
	"fmt"
	"gotest.tools/v3/assert"
	"net"
	"testing"
)

func TestCommandCount(t *testing.T) {
	connection := getLocalConnection()
	commandCount := getCommandCount(connection)
	if commandCount <= 0 {
		t.Fail()
	}
	assert.Assert(t, commandCount > 0,
		fmt.Sprintf("Unexpected number of CLI commands found. expected greater than 0, %d found", commandCount))
}

func getCommandCount(connection net.Conn) int64 {
	responseValue := fireCommand(connection, "COMMAND COUNT")
	if responseValue == nil {
		return -1
	}
	return responseValue.(int64)
}

func BenchmarkCountCommand(b *testing.B) {
	connection := getLocalConnection()
	for n := 0; n < b.N; n++ {
		commandCount := getCommandCount(connection)
		if commandCount <= 0 {
			b.Fail()
		}
	}
}
Repository: DiceDB/dice
File: tests/command_count_test.go (Go)

Code before:
package tests

import (
	"fmt"
	"testing"

	"gotest.tools/v3/assert"
)

func TestCommandCOUNT(t *testing.T) {
	commandsCount := 40

	subscriber := getLocalConnection()

	responseValue := fireCommand(subscriber, "COUNT")
	responseCount  := responseValue.(int64)
	if responseValue == nil {
		t.Fail()
	}

	assert.Assert(t, responseCount == int64(commandsCount),
		fmt.Sprintf("Unexpected number of CLI commands found. %d expected, %d found", commandsCount, responseCount))
}

func call(howmany int, t *testing.B) {
	subscriber := getLocalConnection()
	for i := 0; i < howmany; i++ {
		rp := fireCommandAndGetRESPParser(subscriber, "COUNT")
		if rp == nil {
			t.Fail()
		}
	}
}

func BenchmarkCountCommand200(t *testing.B) {
	call(200, t)
}

func BenchmarkCountCommand2000(t *testing.B) {
	call(2000, t)
}

Review comment (suggestion):
Giving such generic function name is a really bad idea as this function is available throughout the package. Please reer to other benchmark functions and go's best practices and structure this better.

Also, update the output of number of allocs and time taken during the benchmar.

Code after:
package tests

import (
	"fmt"
	"gotest.tools/v3/assert"
	"net"
	"testing"
)

func TestCommandCount(t *testing.T) {
	connection := getLocalConnection()
	commandCount := getCommandCount(connection)
	if commandCount <= 0 {
		t.Fail()
	}
	assert.Assert(t, commandCount > 0,
		fmt.Sprintf("Unexpected number of CLI commands found. expected greater than 0, %d found", commandCount))
}

func getCommandCount(connection net.Conn) int64 {
	responseValue := fireCommand(connection, "COMMAND COUNT")
	if responseValue == nil {
		return -1
	}
	return responseValue.(int64)
}

func BenchmarkCountCommand(b *testing.B) {
	connection := getLocalConnection()
	for n := 0; n < b.N; n++ {
		commandCount := getCommandCount(connection)
		if commandCount <= 0 {
			b.Fail()
		}
	}
}