Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove starter cards, add some info in Log, Add Color team for Dragon #120

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

m0ssa99
Copy link
Contributor

@m0ssa99 m0ssa99 commented May 19, 2024

m0ssa99 added 3 commits May 19, 2024 22:49
modified:   .gitignore
modified:   Api/SplinterlandsAPI.cs
modified:   Bot/BotInstance.cs
modified:   Config/Settings.cs
modified:   Model/DetailedCard.cs
modified:   Model/LogSummary.cs
modified:   Model/UserCard.cs
modified:   .gitignore
modified:   Api/SplinterlandsAPI.cs
modified:   Bot/BotInstance.cs
modified:   Config/Settings.cs
modified:   Model/DetailedCard.cs
modified:   Model/LogSummary.cs
modified:   Model/UserCard.cs
@Plloi
Copy link
Contributor

Plloi commented May 21, 2024

this pr has mixed tabs and spaces for indentation.
this also has multiple changes that (imo) would be better split into multiple commits/pull requests to be assessed separately

@Plloi
Copy link
Contributor

Plloi commented May 21, 2024

also commenting out no longer valid/needed functions/code clutters the code, (yes i know it's already like that, but why make it worse?)

@@ -134,20 +134,21 @@ public static async Task<(int newRating, int ratingChange, decimal spsReward, in
{
gameResult = 2;
}

var rhsr = JToken.Parse((string)matchHistory["battles"][0]["dec_info"]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no indentation, could also use a better name, rewardInfo or matchRewards maybe


return (newRating, ratingChange, spsReward, gameResult);
return (newRating, ratingChange, spsReward, gameResult,glint);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would suggest a space between the comma and glint for readability and to match style

@@ -172,6 +173,41 @@ public static async Task<JToken> GetPlayerBalancesAsync(string username)
}
return null;
}
public static async Task<decimal> GetTotalUnclaimedBalanceAsync(string username, string accessToken)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function looks good, but indentation is inconsistent

@@ -208,7 +244,7 @@ public static async Task<UserCard[]> GetPlayerCardsAsync(string username, string
data = await Helper.DownloadPageAsync($"{Settings.SPLINTERLANDS_API_URL_FALLBACK}/cards/collection/{ username }?token={ accessToken }&username={ username }");
}

DateTime oneDayAgo = DateTime.Now.AddDays(-1);
DateTime oneDayAgo = DateTime.Now.AddDays(-2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onedayago now 2 days?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be renamed to twoDaysAgo then probably?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe cooldown, in case the change it again

@@ -282,6 +284,8 @@ private async Task<(string secret, string tx, JToken team)> SubmitTeamAsync(stri
var cardQuery = CardsCached.Where(x => x.card_detail_id == (string)team["summoner_id"]);
string summoner = cardQuery.Any() ? cardQuery.First().card_long_id : null;
string monsters = "";
var culoare = (string)Settings.CardsDetails[((int)team["summoner_id"]) - 1].GetstringCardColor();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. mixing languages in a codebase can be confusing
  2. is there an issue this is meant to solve i'm not aware of?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Changed
  2. Is for color of team,

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't understand this change.

1.Why is it needed?
2.Is it ment to be called "color"? It says "culoare"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, when submitting a team you now need to provide an “allyColor” parameters that matches the element you want to be selected for that summoner, i.e. “Blue”, “Red”, “Green”, “Black”, or “White”., Dragon and Gray is determined by the card colors in the deck.
I changed variable name

@@ -829,7 +840,7 @@ public async Task<DateTime> DoBattleAsync()
{
CurrentlyActive = false;
}
Settings.LogSummaryList.Add((LogSummary.Index, LogSummary.Account, LogSummary.BattleResult, LogSummary.Rating, LogSummary.ECR, LogSummary.QuestStatus));
Settings.LogSummaryList.Add((LogSummary.Index, LogSummary.Account, LogSummary.BattleResult, LogSummary.Rating, LogSummary.ECR, LogSummary.QuestStatus,LogSummary.SPSStake));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would suggest a space between the comma and LogSummary.SPSStake for readability and to match style

@@ -1077,11 +1088,11 @@ private async Task<JToken> GetTeamAsync(JToken matchDetails, bool ignorePrivateA

private async Task ShowBattleResultLegacyAsync(string tx)
{
(int newRating, int ratingChange, decimal decReward, int result) battleResult = new();
(int newRating, int ratingChange, decimal decReward, int result,int glint) battleResult = new();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space after comma here to

for (int i = 0; i < 14; i++)
{
await Task.Delay(6000);
battleResult = await SplinterlandsAPI.GetBattleResultAsync(Username, tx);
battleResult = await SplinterlandsAPI.GetBattleResultAsync(Username, tx,AccessToken);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and here

@Plloi
Copy link
Contributor

Plloi commented May 21, 2024

Logic all looks good.
Not sure i would fully nix the starter card logic, making the starter array empty should stop it from picking starter cards.

most of the issues i see are style/readability related

@der1sebi
Copy link

Logic all looks good. Not sure i would fully nix the starter card logic, making the starter array empty should stop it from picking starter cards.

most of the issues i see are style/readability related

As starter cards are not allowed in wild ranked anymore we can delete it I guess

@PCJones
Copy link
Owner

PCJones commented May 22, 2024

Someone needs to fix the merge conflicts first. Also, are all changes compatible with the API?

I sadly don't have the time to fully dive into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants