Ocarina-TUI - play ocarina in the terminal

code review
1

Me and my fiancee have been watching About Oliver's playthrough of OoT, which got me inspired to make a TUI application for playing the ocarina.

https://github.com/sermuns/ocarina-tui/

It has been a very fun project, though I'm a bit scared of Nintendo Ninjas. Wish me luck!

I would love to get feedback on the code. I don't like premature abstraction but also sometimes lean too heavily into hardcoding things.

2 Likes
2

having a None variant in an enum that doesn't expect one, like Color, or here Song, leads to lots of weird, messy and buggy code. you should instead an Option<Song>.
this would for example remove all the panicky code in song.rs.

you could then make

impl TryFrom<[NoteButton; NUM_NOTES]> for Song

or a custom fn that returns an option intead, which would make clear to the caller that not all sequences are valid.

i am not familiar enough with rustysynth or tinyaudio to talk on the rest though

1 Like
3

same thing for NoteButton btw.

if self.current_note.is_none() && new_note.is_some() {
    ocarina_synth.note_on(MIDI_CHANNEL, new_note.midi_key(), MIDI_VELOCITY);
} else if self.current_note.is_some() && new_note.is_none() {
    ocarina_synth.note_off(MIDI_CHANNEL, self.current_note.midi_key());
} else if self.current_note.is_some() && new_note.is_some() && self.current_note != new_note
{
    ocarina_synth.note_off(MIDI_CHANNEL, self.current_note.midi_key());
    ocarina_synth.note_on(MIDI_CHANNEL, new_note.midi_key(), MIDI_VELOCITY);
}

could be a really nice and clear match that can't sneakily panic if written incorretly if you used Option<NodeButton> instead

4

Ahh I considered it, but somehow settled on implementing my own None variant (twice). Looking at it now, it's obvious I should've used the more native Option<Song> and Option<NoteButton>.

Great observation, I will implement it.

5

Yeah, that code is particularly ugly.

Let's say self.current_note and new_note are Option<_>. How could we use match to do this? Something like

match (self.current_note, new_note) {
    (None, Some(_)) => ...,
    (Some(_), None) => ...,
    _ => {},
}

?

6 
match (self.current_note, new_note) {
    (None, Some(new)) => ocarina_synth.note_on(MIDI_CHANNEL, new.midi_key(), MIDI_VELOCITY),
    (Some(current), None) => ocarina_synth.note_off(MIDI_CHANNEL, current.midi_key()),
    (Some(current), Some(new)) if current!= new => {
          ocarina_synth.note_off(MIDI_CHANNEL, current.midi_key());
          ocarina_synth.note_on(MIDI_CHANNEL, new.midi_key(), MIDI_VELOCITY);
     },
    _ => {}
}
7

also, it feels to me as if you use From too much, instead of specific functions that are named after what they do, like From<NoteButton> for &str.
when to implement From is not super cut and dry, although the From<NoteButton> for &str is one of the example cited in the do for when not to implement From

8

I would prefer something like this:

if self.current_note != new_note {
    if let Some(current) = self.current_note {
        ocarina_synth.note_off(MIDI_CHANNEL, current.midi_key());
    }
    if let Some(new) = new_note {
        ocarina_synth.note_on(MIDI_CHANNEL, new.midi_key(), MIDI_VELOCITY);
    }
}
1 Like
9

Excellent documentation! (which I hadn't read..)

Perhaps implementing Display for them is better? Or adding fn name(&self) -> &str to NoteButton and Song and using that for string representation?

10

name sounds great. Display could be really good too.
given your current specific use case, you probably want a standalone function

fn note_symbol(maybe_note : Option<NoteButton>) -> &'static str

you could also just implement Debug on NoteButton nd debug print it, as the only place where you print note rn s a debug window.
so

title_block.clone().title(format!(
    " {} {:?} {:?}",
    <&str>::from(self.current_note),
    self.note_idx,
    self.playing_song,
)),

would become

title_block.clone().title(format!(
    " {:?} {:?} {:?}",
    self.current_note,
    self.note_idx,
    self.playing_song,
)),
11

Thanks to @Morgane55440 and @Tom47 for wise feedback. I have implemented your suggestions.

1 Like