Page MenuHomePhabricator

Refactor if's

Authored by 12mohaned on Jun 19 2020, 7:03 AM.



Refactor nested if's in to make the code more readable and cleaner

Test Plan

Make run

Diff Detail

rDQ DiamondQuest
Lint Not Applicable

Event Timeline

Restricted Application completed remote builds in Restricted Buildable.Jun 19 2020, 7:03 AM

Thanks for this! I left one suggestion inline on how you can further improve on this. I'd like your thoughts. If you want to implement this, just keep working on your local diamond branch, and arc diff subsequent changes. They'll update this Differential.


I think these were originally four separate items because they'll be handled separately. However, I think you're onto something! KeyboardController won't actually handle any of these directly, but rather will forward them onto to controller that does the actual handling.

I wonder if we can clean this up a bit further however by using in and a tuple? If this is defined somewhere logical, maybe in a separate module...


Then this particular conditional can be changed to...

if event.key in arrows:

What do you think?

I won't be able to get to this tonight, but I'll review it tomorrow for sure. 😊

  • refactor and add tuple arrows
  • Keys
Restricted Application completed remote builds in Restricted Buildable.Jun 20 2020, 7:44 AM
jcmcdonald added a reviewer: jcmcdonald.

Couple of other small things. This is looking great!


Don't forget to add your authorship to this file! :)

(Also suggested a slight edit to the description, but change at will.


My only suggestion here would be to move arrows to be a class attribute of Key, like this.


Class methods conventionally start with cls (for "class") rather than self.


With the suggested changes above, arrows would change to cls.arrows.


Add your name here. Names are sorted alphabetical by last name.

  • Add Authorship
  • add arrows as attribute
Restricted Application completed remote builds in Restricted Buildable.Jun 20 2020, 2:47 PM
This revision is now accepted and ready to land.Jun 20 2020, 5:09 PM
emlarson added a subscriber: hdavis.

Thank you for all your hard work on key presses, @12mohaned (and @hdavis/@jcmcdonald in! Everything looks good on my end. I'm sure once we get the avatar on the screen, this is going to work really well.

Okay, @12mohaned, now that this has been approved, you can land this. Here's the steps:

git checkout master
git pull
git checkout diamond
git rebase  # follow any instructions on screen to rebase if necessary
arc land
This revision was automatically updated to reflect the committed changes.

Nevermind, @hdavis needed this, so I've gone ahead and landed it myself. :) Thanks for the hard work @12mohaned!