Skip to main content
Code Review

Return to Answer

added 85 characters in body
Source Link
Martin R
  • 24.2k
  • 2
  • 37
  • 95
let ustchord = Chord(notes: [0, 2, 4, 6, 7, 9, 10])
print(ustlet upperStructures = chord.upperStructureTriads()
for us in upperStructures {
 print(us)
}

// [22 major,
// 6 diminished]diminished
let ust = Chord(notes: [0, 2, 4, 6, 7, 9, 10])
print(ust.upperStructureTriads())
// [2 major, 6 diminished]
let chord = Chord(notes: [0, 2, 4, 6, 7, 9, 10])
let upperStructures = chord.upperStructureTriads()
for us in upperStructures {
 print(us)
}

// 2 major
// 6 diminished
Source Link
Martin R
  • 24.2k
  • 2
  • 37
  • 95

Disclaimer: I am not an expert in music theory, and english is not my first language. Therefore I may use the wrong terms in the following review.

Review of your current code

I/O should be separated from the computation. In your case, the findUpperStructureTriads() function should return something instead of printing it. That makes the function better usable and testable, and increases the clarity of the program.

Also global variables (triadRoots and triadQualities) should be avoided: Calling findUpperStructureTriads() twice will show unexpected results.

The nested loops are better and simpler written as for-loops over (half-open) ranges:

for firstIndex in 0 ..< degs.count {
 for secondIndex in firstIndex+1 ..< degs.count {
 for thirdIndex in secondIndex+1 ..< degs.count {
 // ...
 }
 }
}

and similarly

for inversionCount in 0 ..< 2 { ... }

and

for i in 0..<triadRoots.count { ... }

Note that your version

for i in 0...(triadRoots.count - 1) { ... }

would crash with a "Fatal error: Can't form Range with upperBound < lowerBound" if triadRoots.count is zero.

Use let for variables which are never mutated, e.g.

let threeNoteGroup = [degs[firstIndex], degs[secondIndex], degs[thirdIndex]]

The default case in checkForTriad() is a case which "should not occur" – unless you made a programming error. To detect such an error early, you can use

default:
 fatalError("Should never come here")

But actually function can be replaced by a more efficient dictionary lookup:

let qualities: [[Int]: String] = [
 maj : "major",
 min : "minor",
 dim : "diminished",
]
// ...
if let quality = qualities[newGroup] {
 print(threeNoteGroup, threeNoteGroup[inversionCount])
 triadRoots.append(threeNoteGroup[inversionCount])
 triadQualities.append(quality)
 break
}

Reducing an integer note to an octave can be done with modulo arithmetic:

extension Int {
 var degreeInOctave: Int {
 let mod12 = self % 12
 return mod12 >= 0 ? mod12 : mod12 + 12
 }
}

An alternative approach

Your program tests each subset of three notes of the given degrees, and its three inversions if it is one of the known triads. It may be more efficient to traverse the given list only once and consider each note as the possible root note of each triad. Then you have only to test if the other notes of the triad are in the list or not.

More structure

Using types gives the objects we operate on a name, allows to group the functionality with the objects, provide initialization methods, etc.

For example, instead of a plain array of notes (or is it degrees?) we can define a Chord structure:

struct Chord {
 let notes: [Int] // Increasing array of degrees in the range 0...11
 
 init(notes: [Int]) {
 // Reduce module 12 and sort:
 self.notes = notes.map { 0ドル.degreeInOctave }.sorted()
 }
 
 func translated(by offset: Int) -> Chord {
 return Chord(notes: notes.map { 0ドル + offset })
 }
}

The init method ensures that the numbers are sorted in increasing order and in the proper range. The translated(by:) method computes a new chord by shifting all degrees. More methods can be added later if needed, e.g.

struct Chord {
 mutating func invert() { ... }
}

for chord inversion.

Triads could be defined as an enumeration:

enum Triad: String, CaseIterable {
 case major
 case minor
 case augmented
 case diminished
 case sus4
 case sus2
 
 var chord: Chord {
 switch self {
 case .major: return Chord(notes: [ 0, 4, 7 ])
 case .minor: return Chord(notes: [ 0, 3, 7 ])
 case .augmented: return Chord(notes: [ 0, 4, 8 ])
 case .diminished: return Chord(notes: [ 0, 3, 6 ])
 case .sus4: return Chord(notes: [ 0, 5, 7 ])
 case .sus2: return Chord(notes: [ 0, 2, 7 ])
 }
 }
}

Instead of global variables (maj, min, ...) we can now refer to a triad as values of the enumeration, e.g.

let triad = Triad.major
print(triad) // major
print(triad.chord) // Chord(notes: [0, 4, 7])

and with the conformance to CaseIterable we get the list of all triads for free:

for triad in Triad.allCases { ... }

Finally we need a type for the results, which are triads at a certain position, for example:

struct UpperStructure: CustomStringConvertible {
 let triad: Triad
 let root: Int
 
 var description: String {
 return "\(root) \(triad.rawValue)"
 }
}

The description method provides the textual representation of the values, and can be adjusted to your needs.

With these preparations, we can define a function to find all upper structure triads in a given chord. This can be a method of the Chord type instead of a global function, so that we now have

struct Chord {
 let notes: [Int]
 
 init(notes: [Int]) {
 self.notes = notes.map { 0ドル.degreeInOctave }.sorted()
 }
 
 func translated(by offset: Int) -> Chord {
 return Chord(notes: notes.map { 0ドル + offset })
 }
 
 func upperStructureTriads() -> [UpperStructure] {
 let notesSet = Set(notes)
 var result: [UpperStructure] = []
 
 for rootNote in notes {
 for triad in Triad.allCases {
 let chordNotes = triad.chord.translated(by: rootNote).notes
 if chordNotes.contains(6) && chordNotes.contains(9)
 && notesSet.isSuperset(of: chordNotes) {
 result.append(UpperStructure(triad: triad, root: rootNote))
 }
 }
 }
 
 return result
 }
}

All possible combinations of root notes and triads are tested, and a Set is used to make the containment test efficient.

Usage example:

let ust = Chord(notes: [0, 2, 4, 6, 7, 9, 10])
print(ust.upperStructureTriads())
// [2 major, 6 diminished]
default

AltStyle によって変換されたページ (->オリジナル) /