Closed darcydev closed 11 months ago
Thanks!
Unit tests - definitely appreciated! I would prefer using assert
rather than expect
for consistency with the other tests.
leagueUrlFactory - idk, your version is like twice as long, I think the original is clearer. If it's about performance rather than clarity, you'll have to convince me of that :)
getRelativeType - when I wrote that I was thinking "this looks pretty shitty" so I agree with you there. Possibly a case/switch statement would be better? I know performance doesn't really matter for this function, but it feels wrong to me to create this object just to not really use it as an object.
And since you are new here, please make sure you're okay with the license (it's not a normal open source license) and if so fill out the CLA and email it to me.
After that, I'd definitely accept a PR with the unit tests, and possibly also a getRelativeType refactor.
Unit tests - updated to assert
here: https://github.com/zengm-games/zengm/pull/453/commits/6ff5720dbef991ed5dff52250dc9d5466588d5f7
leagueUrlFactory - fair call. Have reverted the changes to the helper in the PR.
getRelativeType - updated to a switch/case in this commit: https://github.com/zengm-games/zengm/pull/453/commits/53c2ba3cff24f9f8d7f73a4dd53cfbe3ed05195c. Note that have left the default type
to return the same as uncle
but might be something to consider for future PR too.
License - have read and filled out and emailed you the CLA ✅
Thanks!
Description
getRelativeType
function to streamline the code. The original implementation included multiple conditional checks, which could result in unnecessary computations. The PR implementsswitch
/case
for improved readability too.Changes Made
getRelativeType
leagueUrlFactory