patternjavaMinor
Note class for managing information on various musical notes
Viewed 0 times
managingmusicalnotenotesforclassvariousinformation
Problem
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
```
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 lookup = new HashMap();
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 lookup = new HashMap();
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
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 lookup = new HashMap();
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 lookup = new HashMap();
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
Solution
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
You should try to use
Have you considered all the nasty pitfalls in using the float type? If you don't want to do any math I would prefer
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.Context
StackExchange Code Review Q#26460, answer score: 4
Revisions (0)
No revisions yet.