search

iancoleman / perpetual_auction_currency_game

A game to simulate the Perpetual Auction Currency reward algorithm.
1 stars 1 forks source link

Totals table + style #1

Closed oetyng closed 4 years ago

oetyng commented 4 years ago

Added additional leader board for totals. Minor style fixes.

iancoleman commented 4 years ago

A few minor comments, I know this is early in development but with plans into the future it pays to keep things tight now.

iancoleman commented 4 years ago

This feature is a great idea and addition.

I think it might make sense to keep one table and add a column for total rewards.

It would benefit from having a clickable column headings for sorting by either reward this round or total rewards.

But for now two tables is fine.

oetyng commented 4 years ago

Oh, thought we were working on throw away code here so I didn't bother with that, was just playing around. Never mind the pr.

iancoleman commented 4 years ago

Oh yeah sorry for being a bit over zealous, you're right this is basically a throw away thing but I have a habit of being pretty strict about code in public / shared, especially when it has multiple contributors. Probably shouldn't have applied it so strictly to this project.

Anyway I like the feature a lot, and will include it in a feature issue. Funnily enough I hadn't intended this to be a multiple-round game, just a one-shot "here's a set try to win". But not cancelling future presses of Enter after first press sort of by accident made it multi-round. So I'm going to focus on making it properly multi-round rather than accidentally, and that will of course involve cumulative totals.

happybeing commented 4 years ago

I agree it felt over zealous and hope @oetyng won't be discouraged - I would have been.

I did though also find those points educational for my own improvement, so am glad from that point of view that you made them and explained why. I'm still very green with my JS code style and GitHub practices, so watching how experienced people do stuff is very helpful, especially when explained like that.

One Q is why do you 'hate' :wink: the default .gitignore?

iancoleman commented 4 years ago

The .gitignore automatically added by this commit makes assumptions about the work environment that are only meaningful to some users, so the entries are better placed in those users global git config. My $HOME/.gitignore_global has 27 entries; worth using if you can.

To add to your point about 'educational for my own improvement', I've been in similar situations where people have been tough on me on github. Hard and frustrating though it was, It helped me both a) take the effort to learn things I wouldn't have otherwise and b) appreciate the overall quality of a highly maintained codebase. I learned that a really tight codebase takes (for me anyhow) a lot of mental load away, and that tightness includes git history.

I think each point individually is really minor and I failed to realise that all of them in aggregate is quite intimidating. Apologies @oetyng and definitely please do stay open to future commits if you feel inclined, I promise to be less anal!

oetyng commented 4 years ago

No need to apologize. Do your thing :) I don't even code javascript so I'm not going to commit here. I got the impression first that you were just playing around with the code. I have coded up so many safenetwork simulation drafts that I probably just assumed this was a quick draft. Me thinking wrong.

When I just play around and do not intend to keep the code for long, I do minimum ceremony. (Which you might have noticed on the simulations repo I shared with you.) I'm just so used with writing code and actually throwing it away that I've found it to many times not be worth the extra faff for those things.

I've been code reviewing commits from a whole team of developers for years, so it's not that I'm not anal myself with code quality, I just didn't realize that you had bigger intentions for this code :)