Skip to content
This repository has been archived by the owner on Jan 15, 2021. It is now read-only.

Use IDictionary.Contains instead of exceptions to get glimpse request id #133

Closed
wants to merge 2 commits into from
Closed

Conversation

jeffora
Copy link
Contributor

@jeffora jeffora commented Nov 10, 2011

Don't use exceptions for control flow when it's not an 'exceptional' situation, and a perfectly good alternative exists. It's bad (tm). Or more to the point, it's friggen annoying when debugging (also slower etc).

@avanderhoorn
Copy link
Member

@jeffora Sorry for the delay. We are going to be doing some work on this stuff tomorrow night and will bring this in then. Thanks for the patch!

@nikmd23
Copy link
Member

nikmd23 commented Nov 19, 2011

how about this:

var guid = context.Items[GlimpseConstants.GlimpseRequestId] as Guid?;

        if (guid.HasValue)
            return guid.Value;

        Guid result;
        context.Items[GlimpseConstants.GlimpseRequestId] = result = Guid.NewGuid();
        return result;

@jeffora
Copy link
Contributor Author

jeffora commented Nov 19, 2011

Yeh that would work. Forgot about the need to store the new GUID when answering yesterday. Sorry took so long to comment, I didn't have access to a computer yesterday and wasn't sure on whether the context dictionary would throw a KeyNotFoundException. I'll update the commit with your change

… Guid and store new Guid if an existing one wasn't found.
@avanderhoorn
Copy link
Member

@nikmd23 are we good to pull this one in?

@jeffora
Copy link
Contributor Author

jeffora commented Feb 9, 2012

Closing this as I don't think it's relevant anymore leading into v1.0

@jeffora jeffora closed this Feb 9, 2012
@rancomarcus
Copy link

@jeffora Could you please clarify your last comment? Is a new release imminent? The generated NullReferenceExceptions are really messing up the debugging experience.

@jeffora
Copy link
Contributor Author

jeffora commented Apr 16, 2012

There's definitely one in the pipeline, although I'm not sure how close it is to being done. It's a fairly hefty rewrite, so this small change doesn't seem relevant moving forward. @avanderhoorn or @nikmd23 might be able to give more information

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

Successfully merging this pull request may close these issues.

None yet

4 participants