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.

Sign up for GitHub

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

Improved performance of chords creation for a specific key by creating functions instead of creating them on init #52

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Improved performance of chords creation for a specific key by creating functions instead of creating them on init #52

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b2085a
improved performance on chords creation for key by creating functions…
SebastianBoldt Feb 8, 2025
8bd6b7e
fixed unit-tests
SebastianBoldt Feb 8, 2025
b4f736a
converted functions to computed properties
SebastianBoldt Feb 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 30 additions & 25 deletions Sources/Tonic/Key.swift
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

I strongly recommend that these utility functions live outside the actual implementation of Key -- as these functions are not directly related to the actual data structure, they should live in a file called like Key+Extensions as they are utility/connivence functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I'm unsure about proceeding with this because the overall style of the project typically consolidates all related business logic into a single file. Particularly considering that the functions for initially populating these values were also included in the same file, I would prefer to keep them where they are.

I transformed them from actual functions into computed variables; perhaps this is an improvement.
If you'd still like me to transfer them to a file, I can certainly do that. I don't mind at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmmm you are right, we probably need to have some style opinion, as that is mine, I like spectating nice utility functions into ...+Extensions files. Your additions LGTM and don't actually impact the underlying data structure so we should be good.

Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,6 @@ public struct Key: Equatable {
/// A note set containing all the notes in the key
public let noteSet: NoteSet

/// All the traditional triads representable root, third, and fifth from each note in the key
public let primaryTriads: [Chord]

/// All chords that fit in the key
public let chords: [Chord]

/// Initialize the key
/// - Parameters:
/// - root: The primary note class of the key, also known as the tonic
Expand All @@ -36,7 +30,36 @@ public struct Key: Equatable {
}
}
noteSet = NoteSet(notes: r)
}

/// The type of accidental to use in this key
public var preferredAccidental: Accidental {
if root.accidental == .sharp {
return .sharp
}
if root.accidental == .flat {
return .flat
}

let naturalKeysWithFlats: [Key] = [.F, .d, .g, .c, .f]
if naturalKeysWithFlats.contains(self) {
return .flat
}
return .sharp
}

/// All chords that fit in the key
public var chords: [Chord] {
let table = ChordTable.shared
var chords: [Chord] = []
for (_, chord) in table.chords where chord.noteClassSet.isSubset(of: noteSet.noteClassSet) {
chords.append(Chord(chord.root, type: chord.type))
}
return chords
}

/// All the traditional triads representable root, third, and fifth from each note in the key
public var primaryTriads: [Chord] {
let table = ChordTable.shared

var chords: [Chord] = []
Expand All @@ -53,25 +76,7 @@ public struct Key: Equatable {

let primaryTriadsStartingWithC = primaryTriads.sorted(by: { $0.root.letter < $1.root.letter })
let rootPosition = primaryTriadsStartingWithC.firstIndex(where: { $0.root == root }) ?? 0
self.primaryTriads = Array(primaryTriadsStartingWithC.rotatingLeft(positions: rootPosition))

self.chords = chords
}

/// The type of accidental to use in this key
public var preferredAccidental: Accidental {
if root.accidental == .sharp {
return .sharp
}
if root.accidental == .flat {
return .flat
}

let naturalKeysWithFlats: [Key] = [.F, .d, .g, .c, .f]
if naturalKeysWithFlats.contains(self) {
return .flat
}
return .sharp
return Array(primaryTriadsStartingWithC.rotatingLeft(positions: rootPosition))
}
}

Expand Down