5
\$\begingroup\$

I just finished (re)reading Clean Code, and one of the ideas I learned was the SRP. However, I am not quite sure on what defines a "Single Responsibility." In the piece of code below, I have a Note class which I think may be doing too many things; it stores pitch and length, and converts these into a Names enum and a Lengths enum. Is this more than one responsibility, and if so, how should I define the classes instead? Other tips are welcome as well).

public class Note {

    public enum Lengths {
        DOUBLE_WHOLE_NOTE(0.5f),
        ...
        SIXTYFOURTH_NOTE(64);

        private float length;

        Lengths(float length) {

            this.length = length;
        }

        public float getLength() {

            return length;
        }

        private static final Map<Float, Lengths> lookup = new HashMap<Float, Lengths>();
        static {
            for (Lengths length : Lengths.values())
                lookup.put(length.getLength(), length);
        }

        public static Lengths get(float length) {

            return lookup.get(length);
        }

    }

    public enum Names {
        C(0),
        ...
        REST(-1);

        private final int number;

        Names(int number) {

            this.number = number;
        }

        public int getValue() {

            return number;
        }

        private static final Map<Integer, Names> lookup = new HashMap<Integer, Names>();
        static {
            for (Names name : Names.values())
                lookup.put(name.getValue(), name);
        }

        public static Names get(Integer value) {

            return lookup.get(value);
        }

    }

    private int pitch;
    private float length;

    public int getPitch() {

        return pitch;
    }

    public void setPitch(int pitch) {

        this.pitch = pitch;
    }

    public float getLength() {

        return length;
    }

    public void setLength(float length) {

        this.length = length;
    }

    private Note(int pitch, float length) {

        this.length = length;
        this.pitch = pitch;
    }

    public int getOctave() {

        return (pitch / 12) - 5;
    }

    public Note.Names getNameOfNote() {

        return Names.get(this.getPitch() % 12);
    }

    public Note.Lengths getLengthOfNote() {

        return Lengths.get(this.getLength());
    }

}
\$\endgroup\$
  • 1
    \$\begingroup\$ Here's a nice article that made practical SRP much clearer for me: sites.google.com/site/unclebobconsultingllc/… Be sure to check the article comments too, where there are plenty of further ideas. \$\endgroup\$ – Juha Untinen May 22 '13 at 10:55
  • \$\begingroup\$ Unlce Bob defines responsibility as 'a reason to change', so the SRP comes down to : a class should have only one reason to change. I think your classes are fairly safe in that department, but you may want to take a look at removing some primitive obsession \$\endgroup\$ – bowmore May 22 '13 at 15:34
4
\$\begingroup\$

Although you put the three classes into one file, they are already three classes. So you already split the responsibility. Translating a float to a length is one "responsibility" and translating the pitch to a name is another. Seen from this angle everything is fine.

SRP is a heuristic for good OO code. You won't go straight to hell if your class does two things. But if your class does 20 things you should consider a refactoring.

If you have the feeling that your code is not readable or your file is too long, just extract the inner enum to standalone enum. In most cases it is no difference if you have an inner class/enum or a real one, so you should prefer the more readable solution.

In addition to that, I would store the length as the enum at the note and translate it once in the setter (reused in the constructor) instead doing it every time in the getter. I'm also in doubt if I would offer an interface with a integer length getter and the enum length getter. Usually you want to force the caller to use the enum object, I guess.

You should try to use this and the getters in a consistent way.

Have you considered all the nasty pitfalls in using the float type? If you don't want to do any math I would prefer String, otherwise you might want to think about using BigDecimal.

\$\endgroup\$
  • \$\begingroup\$ Thanks mnhg, you've put my mind at ease and I'll get to work implementing your suggestions, but before that, I still have one question. What do you mean by float pitfalls? I know that are are not as precise as doubles, but I doubt that length will be used past .5 or .25. Is it still a problem? \$\endgroup\$ – Russell May 22 '13 at 10:15
  • \$\begingroup\$ Float and doubles have the same issues. Check e.g. floating-point-gui.de . It really depends on what you are planing to do with this floats. Even adding then up might lead to a minor error and a comparison might fail. \$\endgroup\$ – mheinzerling May 22 '13 at 10:57
  • \$\begingroup\$ Alright, time to change to a BigDecimal, thanks for the catch. \$\endgroup\$ – Russell May 22 '13 at 11:35
3
\$\begingroup\$

A word of caution for anyone who stumbles on this question: Just do not use Float or Double as Map keys.

Do not use plurals as type names.

See:

Cars audi = new Cars();

vs

Car bmw = new Car();
\$\endgroup\$
1
\$\begingroup\$

I would use more standardized names.

The length is known as the duration. It can be a float. This way you also allow for non-powers of 2 (like triplets, dotted notes, fractional time signatures, etc.)

 public float getLength() {

     return length;
 }
public float getDuration() {

       return duration;
 }

The name of the note is generally either in roman numeral or scientific pitch notation. Your name is actually the latter. You could also add a helper getPitchClass. Note that the modular arithmic congruent operation is (value % size + size) % size. This allows for negative pitches. I noticed C5 is pitch 0. So B4 would be -1.

public Note.Names getNameOfNote() {

      return Names.get(this.getPitch() % 12);
}
public int getPitchClass() {

      return (this.getPitch() % 12 + 12) % 12;
}

public Note.Names getScientificPitchClassName() {

      return Names.get(this.getPitchClass());
}

The octave should get floored to allow also negative octaves. Since octave 5 is the zero point, octave 4 would be -1.

public int getOctave() {
    return (pitch / 12) - 5;
}
public int getOctave() {

     return Math.Floor(pitch / 12) - 5;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.