Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Several little improvements and minor functionality extending #50

Merged
merged 9 commits into from
Oct 28, 2015

Conversation

Mrnikbobjeff
Copy link

added the necessary code to check whether Spotify.exe and SpotifyWebHelper.exe actually exist and code which checks the location of SpotifyWebHelper.exe as mentioned in #48
replaced all occurances of checking whether a string is empty via str == ""; with String.IsNullOrEmpty(str);

sealed StringAttribute because it is only contains Text as an attribute
sealed SpotifyWebAPI class because it implements IDisposable
changed Exception catching where the Exception is not used to ignore the Exception object
removed Console.WriteLine("Exception: " + e.Message) in HttpProcessor.Process as it is a library function which might not be used in a console application
added capital letters for the first word in 3 Exception constructors
changed ruleset back to Microsoft Managed Recommended Rules

removed nircmd.dll and replaced internal mute function calls with the IAudioSession interface which works on Windows 7 or newer, #26 is solved by this
Added advanced volume control for spotify's volume mixer

beautified the code via codemaid to improve readability

mrnikbobjeff added 7 commits October 16, 2015 13:54
… == "" with String.IsNullOrEmpty(str)

sealed StringAttribute because it is only contains text as attribute
sealed SpotifyWebAPI class as it implements IDisposable
Changed Exception catching where the Exception is not used to ignore the Exception object
Removed Console.WriteLine("Exception: " + e.Message) int HttpProcessor.Process as it is a library function which might not be used in a console application
Organised using statements alphabetically
Changed Mute and UnMute function to work with both x86 and x64 processes on Windows 7 or newer
Added functions to manipulate the Volume Control of Spotify
@Mrnikbobjeff Mrnikbobjeff changed the title Added support for different locations of SpotifyWebHelper.exe Several little improvements and minor functionality extending Oct 17, 2015
@JohnnyCrazy
Copy link
Owner

Wow, thanks for this PR, nice changes! 🌟
Will review them somewhere in the next days, but already looks good! :)

Imporved spelling in new commits
if (File.Exists("nircmd.dll"))
DoNirCmd("muteappvolume spotify.exe 1");
//Windows < Windows Vista Check
Contract.Requires(Environment.OSVersion.Version.Major >= 6);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, after reading a bit about Contracts, I'm unsure if it's worth using here while we still have some normal "if-throw-else"-statements. So why did you chose this over "if-throw-else"-statements? Any particular reason?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason for using Contracts was the option to immediately throw an Exception if an argument is invalid. I became accustomed to it, in hindsight I should have stayed with if-throw-else. The main advantage for continuous Contract usage is the possibility to combine it with IntelliSense
figure 2

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh, I actually like the idea, but I wonder if it's good to put it in a library. ("if-throw-else" seems to be a more general solution, let me think and research a bit about it before changing 👍 )

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wikipedia article describes DbC really well, here is the link: https://en.wikipedia.org/wiki/Design_by_contract

Changed use of nullable return types to throw exceptions instead of returning null
@Mrnikbobjeff
Copy link
Author

So, any idea when this will be merged? I am currently working on a separate implementation of JSON.net, which could possibly remove the dependency, thus making the library a standalone module. I would like to create a new branch for that, but I will only start implementing it here once the existing pull request is merged.

@JohnnyCrazy
Copy link
Owner

Will do shortly.

Regarding json.Net:
I actually would like to keep it. It's well tested, widely used and you just need it in a lot of cases. An own implementation could lead to problems.

JohnnyCrazy added a commit that referenced this pull request Oct 28, 2015
Several little improvements and minor functionality extending
@JohnnyCrazy JohnnyCrazy merged commit cd74868 into JohnnyCrazy:master Oct 28, 2015
@@ -207,6 +203,7 @@ public void Dispose()
{
IsActive = false;
_listener.Stop();
GC.SuppressFinalize(this);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for this call? @Mrnikbobjeff

@Mrnikbobjeff
Copy link
Author

When implementing the dispose pattern you might also add a finalizer to your class that calls Dispose(). This is to make sure that Dispose() always gets called, even if a client forgets to call it.

To prevent the dispose method from running twice (in case the object already has been disposed) you add GC.SuppressFinalize(this);. The documentation provides a sample:

class MyResource : IDisposable
{

// This destructor will run only if the Dispose method 
// does not get called.
~MyResource()      
{
    // Do not re-create Dispose clean-up code here.
    // Calling Dispose(false) is optimal in terms of
    // readability and maintainability.
    Dispose(false);
}

// Implement IDisposable.
// Do not make this method virtual.
// A derived class should not be able to override this method.
public void Dispose()
{
    Dispose(true);
    // This object will be cleaned up by the Dispose method.
    // Therefore, you should call GC.SupressFinalize to
    // take this object off the finalization queue 
    // and prevent finalization code for this object
    // from executing a second time.
    GC.SuppressFinalize(this);
}

private void Dispose(bool disposing)
{
    // Check to see if Dispose has already been called.
    if(!this.disposed)
    {
        // If disposing equals true, dispose all managed 
        // and unmanaged resources.
        if(disposing)
        {
            // Dispose managed resources.
            component.Dispose();
        }

        // Call the appropriate methods to clean up 
        // unmanaged resources here.
        resource.Cleanup()          
    }
    disposed = true;         
}

}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants