Adjust the sound volume while playing #4
Conversation
|
/codehelper hello |
![devd-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| private static SoundController instance; | ||
| private MediaPlayer inGameSound; | ||
| private BorderPane root; | ||
| private static double volume = 1.0; |
There was a problem hiding this comment.
Improvement: Consider initializing the inGameSound field in the constructor rather than having it as null initially. This would prevent potential null pointer exceptions if methods like stopInGameSound() are called before playInGameSound().
| { | ||
| inGameSound.stop(); | ||
| } | ||
| Media media = new Media(getClass().getResource(musicResource).toExternalForm()); |
There was a problem hiding this comment.
Suggestion: Consider adding a check for null before accessing the resource to prevent potential NullPointerException if the resource is not found:
if (getClass().getResource(musicResource) == null) {
System.err.println("Resource not found: " + musicResource);
return;
}
| sfxVolumeLabel.setText(String.format("%.0f%%", sfxVolume * 100)); | ||
| System.out.println("VFX Volume label updated to: " + sfxVolumeLabel.getText()); | ||
| if(sfxVolume == 0.00) | ||
| { |
There was a problem hiding this comment.
Best Practice: Consider using a logger instead of System.out.println() for better control over logging output.
| private BorderPane root; | ||
| private static double volume = 1.0; | ||
| private static double prevVolume = 1.0; | ||
| private static double sfxVolume = 1.0; |
There was a problem hiding this comment.
Improvement: The prevVolume and prevSfxVolume fields are static but could be instance variables since they're specific to each instance of SoundController. Since this is a singleton, it might not cause issues, but it's better practice to make them instance variables.
| sfxPlayer.play(); | ||
| } | ||
| public double adjustInGameSFX(Slider sfxVolumeSlider, Label sfxVolumeLabel, Image sfxMute, Image sfxAudioOn, ImageView sfxImageViewPlaying) { | ||
| sfxVolumeSlider.setValue(sfxVolume); |
There was a problem hiding this comment.
Suggestion: Consider adding a null check for the sfxImageViewPlaying parameter before accessing it to prevent potential NullPointerException.
| sfxPlayer.play(); | ||
| } | ||
| public double adjustInGameSFX(Slider sfxVolumeSlider, Label sfxVolumeLabel, Image sfxMute, Image sfxAudioOn, ImageView sfxImageViewPlaying) { | ||
| sfxVolumeSlider.setValue(sfxVolume); |
There was a problem hiding this comment.
Design Improvement: The adjustInGameSFX and adjustVolume methods are quite long and do multiple things. Consider breaking them down into smaller, more focused methods for better maintainability.
| public void changed(ObservableValue<? extends Number> observable, Number oldValue, Number newValue) { | ||
| Platform.runLater(() -> { | ||
|
|
||
| volume = newValue.doubleValue(); |
There was a problem hiding this comment.
Improvement: Consider adding a null check for the inGameSound before setting its volume to prevent potential NullPointerException.
| import javafx.scene.media.MediaPlayer; | ||
|
|
||
| public class SoundController { | ||
| private static SoundController instance; |
There was a problem hiding this comment.
Suggestion: Consider adding JavaDoc to the SoundController class and its public methods to better document their purpose and usage.
| @@ -20,7 +22,7 @@ public class DinosaurController { | |||
| private Entity score; | |||
| private Entity life; | |||
| private int lives = 3; | |||
|
|
|||
| private MediaPlayer inGameSound; | |||
There was a problem hiding this comment.
Improvement: The inGameSound field is now declared in both DinosaurController and SoundController. Since SoundController manages the sound, consider removing the duplicate field from DinosaurController.
| public Image getViewImage() { | ||
| return viewImage; | ||
| } | ||
| public void playSoundEffect(String soundResource) { |
There was a problem hiding this comment.
Suggestion: Consider adding proper error handling when playing sound effects. If the sound file is missing, the current implementation might throw an exception.
| public PauseMenu() { | ||
| super(MenuType.GAME_MENU); | ||
| DinosaurApp.initLanguages(); | ||
|
|
||
| DinosaurApp.initLanguages(); | ||
|
|
||
| PauseButton btnBack = new PauseButton(getLocalizationService().getLocalizedString("Pause.1"),() -> fireResume()); | ||
|
|
There was a problem hiding this comment.
Code Style: The comment //need2ComeBack seems like a TODO item. Consider using a proper TODO comment format like // TODO: [description] or removing it if no longer needed.
| //These are for tracking how many times the pauseMenu has been opened. Helps in getting the mute/unmute image from SoundController | ||
| //For first time, this helps loading the unmuted icon and if count increases more than 1, it changes the icon depending on the controller | ||
| //At this point of time, I really don't have other proper method, It works so why not, But if you know any other solution please let me know and contribute | ||
| private int count = 0; |
There was a problem hiding this comment.
Design Concern: The comment about tracking how many times the pause menu has been opened suggests a workaround solution. Consider refactoring this approach to a more robust design that doesn't rely on counting menu opens.
|
|
||
| ControlButton btnControls = new ControlButton(getLocalizationService().getLocalizedString("Pause.3")); | ||
|
|
||
|
|
There was a problem hiding this comment.
Resource Management: Consider using try-with-resources for handling input streams to ensure they are properly closed after use.
|
|
||
| //These are for tracking how many times the pauseMenu has been opened. Helps in getting the mute/unmute image from SoundController | ||
| //For first time, this helps loading the unmuted icon and if count increases more than 1, it changes the icon depending on the controller | ||
| //At this point of time, I really don't have other proper method, It works so why not, But if you know any other solution please let me know and contribute |
There was a problem hiding this comment.
Improvement: The count and sfxCount variables are used to track menu opens, but this approach seems fragile. Consider using a more direct approach to check the current state from SoundController instead.
| public static final String SHOOT_SOUND = "shoot.wav"; | ||
| * SOUNDS | ||
| */ | ||
| public static final String ENEMYSHOOT_SOUND = "/assets/sounds/enemyShoot.wav"; |
There was a problem hiding this comment.
Consistency: The sound path constants in GameConstants have been updated to include the /assets prefix, which is good for consistency, but ensure all code that uses these constants has been updated accordingly.
👉 Checklist
✍ Description of the Pull Request
🔗 Issue link