6

for example, suppose I need to play different sounds according to "grade":

file list:

fairSound.mp3
goodSound.mp3
excellentSound.mp3

code:

showResult(grade){
  if(grade==0 || grade==1){
    SoundUtility.play(fairSound);
  }else if(grade==2 || grade==3){
    SoundUtility.play(goodSound);
  }else if(grade==4){
    SoundUtility.play(excellentSound);
  }
}

but I don't want any branching at this case, so my question is, is it a good or an anti pattern to duplicate sounds:

file list:

sound0.mp3 (copy from fairSound.mp3)
sound1.mp3 (copy from fairSound.mp3)
sound2.mp3 (copy from goodSound.mp3)
sound4.mp3 (copy from goodSound.mp3)
sound4.mp3 (excellentSound.mp3)

so that it doesn't need to write any branching case in the program in this case:

showResult(grade){
  SoundUtility.play(sound[grade]);
}

? Is it violating DRY principle?

Note: I also don't want to something like:

var arr=[fairSound.mp3,fairSound.mp3,goodSound.mp3,goodSound.mp3,excellentSound.mp3];

into my code because when I want to change grade 0 to play fairSound.mp3, I still need to modify the code

  • 4
    you dont need to actually copy the files for this code to work. just point the array elements at the correct loaded into memory sound file – Ewan Nov 2 '18 at 6:17
  • 4
    @gnat: you completely missed the point of the question. – Doc Brown Nov 2 '18 at 7:05
  • 3
    You seem focused on adhering to fairly specific patterns. Instead, focus on the best overall solution regardless of whether it matches any pattern or not. It seems that your if based code or the array would be a totally fine solution. – usr Nov 2 '18 at 8:25
  • 1
    Using a config file as suggested in Doc Brown's answer gives you the flexibility you need. As for DRY, it's not really about repetition, it's about implicit coupling between different pieces of code that always have to change together. – Filip Milovanović Nov 2 '18 at 8:47
  • 1
    Forget DRY how about the fact that "fairsound" is a more meaningful name than "sound0"? Really this is a mapping problem - which if-else/switch and yes a map can express. In this case I'd probably be going with a switch statement. Using an array like that is effectively like a map - the indices implicitly being keys. If this is about not changing code then have it in configuration! A very simple grade.X.sound=fair/good/excellent would express this and property files like this should have a nice easy natural fit into a map. – cyborg Nov 2 '18 at 11:54
23

If changing the sound files for each single grade without modifying the code is the requirement here, I would externalize the configuration (mapping).

Create entries in your configuration mechanism (config files or database) which contain the sound file names for each grade.

Instead of duplicating the sound files I would rather list the same file name multiple times.

Example for an .ini file:

[resultSounds]
0=fairSound.mp3
1=fairSound.mp3
2=goodSound.mp3
3=goodSound.mp3
4=excellentSound.mp3

Read your configuration file into some data structure (like an array or a hash map) and use that in your method to find the file to play.

  • 1
    This should be the answer. This problem is easily solved by some sort of mapping from grades to sounds, whether it's in database tables, config files or hard coded in your source code. The OP's approach is an anti pattern in that the solution is so simple. – Greg Burghardt Nov 2 '18 at 11:38
11

It is neither good or bad, and definitely neither a "pattern" nor an "anti-pattern". This is all about what your (non-functional) requirements are.

You wrote you want to be able "to change grade 0 to play fairSound.mp3" (or another sound), without changing the code. This looks to me as if it is more or less "by chance" that some grades have the same sound. So you could treat them as if those soundfiles were all different, in which case it is clear you should duplicate the mp3 files. This makes sure whenever you change the sound of one grade, the other grades are not affected.

However, if you want to be able to exchange the mp3 file of one sound later, and make sure all usages of this particular file will be updated later as well (for all the grades where it was used), you should avoid duplication. You can achieve this most easily by the kind of array you mentioned in your question. If you want to avoid having to change the code afterwards, put the array into an external configuration file outside of the code.

TLDR; think about the kind of "changeability" you want to achieve: either optimize for changeability "per grade", or for changeability per "sound category".

  • Up-ed. While configuration may be the implementation solution of choice, this answer illuminates the fundamental problem. Either way the literal OP question conflated it with software version control. Is there is a clear definition of why and when this application's repo branches? An software version control plan is essential for any application you or a customer cares about. – radarbob Nov 9 '18 at 19:10
2

Is using the { character twice in your program a violation of DRY? I hope you will say, naturally not.

When evaluating if something is a violation of DRY, you must not only consider the similarity between the pieces of code (or the files in your case), but also the reason why it looks so similar.

If it looks similar because it is required to be the same (the same functionality is coded twice, or grades 0 and 1 must always play the same sound), then you have spotted a violation of DRY.
If it looks similar today, but it is completely fine and expected that a new requirement can come in that will cause it to be different, then the similarity is incidental and not a violation of DRY. For example, if it is reasonable that there might be a request to play different sounds for grade levels 0 and 1, then the fact that the files sound0.mp3 and sound1.mp3 are currently the same is not an issue.

0

What you're doing is mapping the sound file with an identifier in your program (literally an index in your array). This isn't an antipattern if each individual file is unrelated with another.

However now you're introducing grade quality into the equation, so you end up with groups of sounds related to each other. In order to fix this, you're having to map your mapping. This hardly makes sense, as the mapping is arbitrary and entirely up to you on how to structure, so it makes sense to rearrange in such a way as to take this into consideration.

Logical sound grouping

One possible way might be creating subgroups of sounds. Rather than have an array of sounds [A, B, C, D], with A, B, C being different qualities of the same sound, you could group it as [[A, B, C], [D]]. The grade being passed now directly corresponds with the index in your subarray. Notice that D stands alone. If a high grade of sound D is requested, you can grab item at index min(gradeArr.length - 1, gradeParam), or in other words, grab the appropriate grade quality or the maximum available otherwise.

File naming approach

A second approach might be give a logical grouping to files belonging to the same sound grade. So for sound "birdChirping", to represent multiple sound grade qualities, the files birdChirping_01.mp3, birdChirping_02.mp3, and birdChirping_03.mp3 might be present in the same folder. In your program, you have only "birdChirping" as its name. Give the name and the sound grade, you can retrieve and play the appropriate file. In your program, rather than map every single sound, you map only the sound type (or in this case "birdChirping").

This is more dynamic in that you don't have to switch up your program for every new file, however you might run into problems when the caller asks for a sound grade which doesn't exist. You may need to list all files named birdChirping_*.mp3 to know what sound qualities are available done in this way. So while more dynamic, it is also less robust, so you should take that into consideration.

Conclusion

Make the data work for you. Don't be afraid to rearrange in more helpful configurations if you need to. You're right to think that the program shouldn't have ugly if else sections for grade. This is how you fix it. You make the program work off the data it's given without any further checks or mappings.

Hope that helps you!

0

The other answers are excellent and I particularly agree with @Neil regarding the loss of robustness if your code now relies on files that might not be there. I just wanted to add that assuming that you have the configuration testing setup to deal with these issues (e.g. crash verbosely on start-up if any required files are missing), I would argue that managing the files by filenames is not a bad idea at all.

This is a particularly good approach in a case when different files are needed in different environments. So each environment would have a different configuration of these files, and to save space and simplify the configuration, you don't actually need to copy them, but rather can instead rely on symlinks. For example, you could have the following file structure, inspired by that used in apache for site configurations:

├── sounds-available
│   ├── excellentSound.mp3
│   ├── fairSound.mp3
│   └── goodSound.mp3
└── sounds-enabled
    ├── excellentSound.mp3 -> ../sounds-available/excellentSound.mp3
    ├── fairSound.mp3 -> ../sounds-available/fairSound.mp3
    └── goodSound.mp3 -> ../sounds-available/fairSound.mp3

To sum up, this pattern moves the configuration flexibility out of the program into the environment itself, which can then be automated and source-controlled by a configuration management system such as Chef/Puppet/Ansible. There are pros and cons to this approach, but I would argue it's a very valid choice for some use-cases.

-2

You want to replace some perfectly fine code (well, it should be a switch handling the default value) with something much more complicated in order to follow some principle. In the current code, everything is in one place, so any changes will be localised.

And yes, if your specs change, you change the code.

Is it a good pattern or an anti-pattern? It’s not a pattern at all. Nobody else thinks it is a good idea.

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.