1

I have a volume variable that I want to be protected from modification unless the person calls a certain function. Is there a way to make it to where it can only be modified by that function other than creating a private class within the class. I suppose that creating a private class is a good idea, but I would be interested if someone else has a different approach. AudioPlayer should never be allowed to change the volume without calling SetVolume. That's the code I have here, but I wondered if people had a different way.

public class AudioPlayer
{
    private class VolumeManager
    {
        private AudioPlayer mAudioPlayer;
        public VolumeManager(AudioPlayer audioPlayer)
        {
            mAudioPlayer = audioPlayer;
        }

        private float volume;

        public void SetVolume(float _volume)
        {
            volume = _volume;

            //Do other necessary things that must happen when volume is changed
            //This is the point of the question
            mAudioPlayer.ModifyChannelVolume(Volume);
        }
        public float GetVolume()
        {
            return volume;
        }
    }

    private VolumeManager mVolumeManager;
    public AudioPlayer()
    {
        mVolumeManager = new VolumeManager(this);
    }

    public void ModifyVolume(float volume)
    {
        mVolumeManager.SetVolume(volume);
    }
}
Gandalf458
  • 2,139
  • 1
  • 21
  • 36
  • 3
    Why not just make Volume a private float? – heartyporridge Mar 05 '15 at 19:08
  • 2
    I think a private class is the only way of doing this. But in this particular case, why not just put the custom code in the setter instead of a separate method? – Cameron Mar 05 '15 at 19:08
  • @Cameron I don't really like the idea of a setter doing other things, but your suggestion is good and could be used by others. as for the comment from analytalica, That would still allow a programmer on my team who didn't understand the setup to modify the volume without calling the other hooks. – Gandalf458 Mar 05 '15 at 19:13
  • If you can settle with not modifying it outside the class, then you can do something like `public float Volume {get; private set;}` (this is called an auto property). Then only the class it's in can set the volume, and you can expose the ability to change the volume through public methods that contain your logic. – mason Mar 05 '15 at 19:13
  • 1
    Maybe I am missing something: how does the 'private nested class' change the 'private setter' access? If it was a 'public non-nested class' with a 'private setter' it would still be inaccessible. Using the private modifier on the property's 'set' *already* accomplishes the stated goal.. – user2864740 Mar 05 '15 at 19:14
  • @user2864740 the point is to force AudioPlayer to modify the volume only through the SetVolume function – Gandalf458 Mar 05 '15 at 19:15
  • 1
    @Gandalf458 It has to use SetVolume because the *setter is already private*.. outside types aren't blessed-to-private-access. – user2864740 Mar 05 '15 at 19:15
  • 2
    @user2864740 I think the OP is wondering if there is a better approach than hiding the volume field inside a private nested class even though the current approach is already making sure the volume can only be set through the `SetVolume` method on the `mVolumeManager` – cgijbels Mar 05 '15 at 19:17
  • 1
    Stop changing code randomly. Ask a question and post the *actual code*. Only modify for correction, not to change the behavior. If there is new code that changes the behavior, post that as a [partial bit of] *new* code and explain how it relates, etc. – user2864740 Mar 05 '15 at 19:19
  • 1
    @user2864740 He's also worried about future maintainers of this class, not just callers into the class from outside. Future maintainers or inheriters can modify the field without using hte SetVolume() function. – Joel Coehoorn Mar 05 '15 at 19:20
  • @user2864740 the edit was for clarity. Functionality is %100 the same as before. – Gandalf458 Mar 05 '15 at 19:20
  • @Gandalf458 Except for the introduction of the type-breaking change .. – user2864740 Mar 05 '15 at 19:21
  • @user2864740 type-breaking change? Your answers are not benefitting the discussion and your downvote is unnecessary since the whole point is to discuss the question's purpose, not to tell me that I'm wrong. – Gandalf458 Mar 05 '15 at 19:24
  • @Gandalf458 I already explained why the entire notion of 'private class' is silly in context - it doesn't change if it's a private property or a private setter. Adding or removing an exposed member is a type/ABI-breaking change. This question still lacks a .. question. If you're wondering if it should be a property or a field or a Get/Set method vs a propety get/set: do what you like as it doesn't matter - still can't access a 'private' member. – user2864740 Mar 05 '15 at 19:27
  • @user2864740 it could be a public class instead of private. The point of it being private was because only AudioPlayer needs to know about it. I appreciate your feedback. I suppose I was looking for an access modifier that would prevent the variable from being changed. In C++ I would have just made the variable a static variable inside of the function, but alas C# does not allow that. Hence why I had C# in the question, though other people seemed to think that was not important. – Gandalf458 Mar 05 '15 at 19:30
  • @Gandalf458 One cannot restrict access to a *specific* method within a class; the visibility of members is class-wide. The direct access to members has to be guarded with honored coding. – user2864740 Mar 05 '15 at 19:31
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/72365/discussion-between-gandalf458-and-user2864740). – Gandalf458 Mar 05 '15 at 19:33

2 Answers2

3

The problem, as I see it, is that even with a private field it's still somewhat intuitive and natural to want to assign directly to the field. We want to make very sure this doesn't happen. In that case, I recommend building this as a property, rather than a field, and just being disciplined about only assigning to the property:

public class AudioPlayer
{

    public float Volume 
    {
       get { return _volume_NeverSetThisDirectly;}
       set 
       {
           _volume = value;
           //Do other necessary things that must happen when volume is changed
           ModifyChannelVolume(_volume_NeverSetThisDirectly);
       }
    }
    [Browsable(false)]
    [DebuggerBrowsable(DebuggerBrowsableState.Never)]
    [EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)]
    private float _volume_NeverSetThisDirectly; //Never assign to this directly!
}

This won't enforce it to the degree that you're asking for, but it does flip the intuitive and natural way someone will work in this class to use the value in the right way, rather than the wrong way. It's also a heck of a lot less code and complexity to maintain. The addition of the attributes largely won't effect things for someone working in this class already, but since we're changing to use social pressures rather than technical prohibitions, the more warning signs we have up, the better.

This also gives you an opening in the future when you find that one weird case where you do want to change the volume field without all of that other stuff happening, that you can do so without needing to do strange things to a private class instance.

Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794
  • I am on a team of many people working quickly. Expecting every programmer to read all the comments when under a time constraint is futile. :( – Gandalf458 Mar 05 '15 at 19:26
  • @Gandalf458 It's not about the comment. It's about, "Who is really going to use `_volume` when `Volume` comes up more naturally"... especially if you hide the field from their intellisense prompts. If you want, you can even change the name of `_volume` to something like `_volumeNeverSetDirectly`. – Joel Coehoorn Mar 05 '15 at 19:28
  • In fact, I think I'll add that to the answer. – Joel Coehoorn Mar 05 '15 at 19:32
  • naming it neverSetDirectly is actually a neat idea, especially if the variable was hidden from Intellisense. I don't know how to hide it though. – Gandalf458 Mar 05 '15 at 19:32
  • 1
    @Gandalf458 You can only protect your code from other team members up to a point. If you are working with people who will make changes without consideration of the impact, then you might want to invest less time in 'hiding' code like this and buy some sort of stick instead... – Paddy Mar 05 '15 at 19:35
  • I added the attribute declarations. Note that in doing this, I checked the documentation, which explicitly states that "`EditorBrowsableAttribute does not suppress members from a class in the same assembly`". However, since part of the point of this is using social pressure, the more warning signs we have up the better. As long it compiles, I may as well leave it there. – Joel Coehoorn Mar 05 '15 at 19:39
0

Maybe, you can declare the Volume variable as private, and only modified the variable within the function, and use a property to expose the Volume field.

private float Volume;  
public float pVolume
    {
        get
        {
            return Volume;
        }
    }
Renato Reyes
  • 189
  • 1
  • 1
  • 10