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

Code cleanup for System.IO.Packaging #2699

Closed
EricWhiteDev opened this Issue Aug 9, 2015 · 5 comments

Comments

Projects
None yet
9 participants
@EricWhiteDev
Contributor

EricWhiteDev commented Aug 9, 2015

  • ContentType.cs - line 234 - use String.Equals instead of String.Compare
  • ContentType.cs - 83 - if (contentType.Length == 0)
  • PackagePart.cs - 849 - return !s.CanRead && !s.CanSeek && !s.CanWrite;
  • Package.cs - 976 - What's the purpose of the try block with an empty finally?
  • PackageUriHelper.cs - 1297 - at least one other place where the same thing was declared. Should they be consolidated?
  • PackageUriHelper.cs - 1025 - Rather than using comments, these could use named arguments.
  • PackageUriHelper.cs - 991 - There are various places thus far in the implementation that use '/' hardcoded... should they instead be using ForwardSlashChar?
  • PackageUriHelper.cs - 926 - Why not just: return the_condition;
  • PackUriHelper.cs - 695 - the chars array could be stackalloc'd instead of heap allocated.
  • PackUriHelper.cs - 451 - Any reason not to combine these Path.Combine calls?
  • InternalRelationshipCollection.cs - 592 - Debug.Fail?
  • InternalRelationshipCollection.cs - 325 - Formatting off
  • IgnoreFlushAndCloseStream.cs - 18 - This comment appears out-of-date: there is no IgnoreFlushAndClose property, and Flush/Dispose never delegate to the wrapped stream's corresponding members.
  • IgnoreFlushAndCloseStream.cs - 48 - get { return !_disposed && _stream.CanRead; }
  • FileFormatException.cs - 139 - readonly?
  • ContentType.cs - 666 - This loop could be replaced by a call to Array.IndexOf.
  • ContentType.cs - 620 - return IsAsciiLetter(character) || (character >= '0' && character <= '9');
  • ContentType.cs - 561 - It seems a bit odd to be explicit about the kind of comparison we want for the characters and to be doing a Compare rather than Equal when what we're checking for is just an empty string.
  • ContentType.cs - 527 - if (string.IsNullOrEmpty(token))
  • ContentType.cs - 531 - The body of this loop could be simplified to just: if (!IsAsciiLetterOrDigit(token[i]) && !IsAllowedCharacter(token[i])) throw new ArgumentException(SR.InvalidToken);
  • ContentType.cs - 398 - There's a lot of string manipulation / allocation / etc. happening in these various helper functions. Doesn't need to be fixed now, as it's not clear that it's a problem. But it'd be interesting to do some perf testing / profiling and see what pops as hot spots that we'll potentially want to fix.
  • ContentType.cs - 390 - "ans" => "and"
  • ContentType.cs - 225 - Do we know that MoveNext is always going to return true here such that Current will be valid?
  • ContentType.cs - 89 - It doesn't appear that we know at this point that contentType isn't empty. Couldn't this result in indexing out of bounds into contentType?
  • ContentType.cs - 262 - "paramterKey" => "parameterKey"
  • PackUriHelper.cs - 791 - Could this just do a ordinal-ignorecase comparison rather than allocating new strings with ToUpperInvariant?
  • ContentType.cs - 323 - Is this type used anywhere? It appears to be dead code.
  • ContentType.cs - 294 - Is this type used anywhere? It appears to be dead code.
  • PackUriHelper.cs - 737 - I don't know if allocations matter here, but you could trivially avoid this allocation by using IndexOf instead of Contains.
  • ContentType.cs - 604 - Couldn't it use Array.IndexOf?
@joshfree

This comment has been minimized.

Show comment
Hide comment
@joshfree
Member

joshfree commented Oct 14, 2015

@EricWhiteDev

This comment has been minimized.

Show comment
Hide comment
@EricWhiteDev

EricWhiteDev Oct 15, 2015

Contributor

@joshfree I have been pulled off of this project. I am headed out for a vacation for the next month. When I return, if possible I can work on these.

Contributor

EricWhiteDev commented Oct 15, 2015

@joshfree I have been pulled off of this project. I am headed out for a vacation for the next month. When I return, if possible I can work on these.

@joshfree joshfree modified the milestones: Future, 1.0.0-rtm Oct 15, 2015

@joshfree joshfree assigned ianhays and unassigned FiveTimesTheFun Oct 15, 2015

@lakkijyotsna

This comment has been minimized.

Show comment
Hide comment
@lakkijyotsna

lakkijyotsna Nov 4, 2015

Hi,
I have a document of type docx, which has a Property ContentType defined as 'Document'. While reading this using System.IO.Packaging ContentType is read as null.
Is this the problem occured because of definition of contentType property in docx file?

lakkijyotsna commented Nov 4, 2015

Hi,
I have a document of type docx, which has a Property ContentType defined as 'Document'. While reading this using System.IO.Packaging ContentType is read as null.
Is this the problem occured because of definition of contentType property in docx file?

Maxwe11 added a commit to Maxwe11/corefx that referenced this issue Nov 8, 2015

alexsorokoletov added a commit to alexsorokoletov/corefx that referenced this issue Nov 8, 2015

Code cleanup for System.IO.Packaging
Third batch of changes in System.IO.Packaging

Related to #2699
@alexsorokoletov

This comment has been minimized.

Show comment
Hide comment
@alexsorokoletov

alexsorokoletov Nov 8, 2015

Contributor

Can someone please clarify following items:

PackUriHelper.cs - 695 - the chars array could be stackalloc'd instead of heap allocated.
InternalRelationshipCollection.cs - 592 - Debug.Fail?
ContentType.cs - 398 - There's a lot of string manipulation / allocation / etc. happening in these various helper functions. Doesn't need to be fixed now, as it's not clear that it's a problem. But it'd be interesting to do some perf testing / profiling and see what pops as hot spots that we'll potentially want to fix.
ContentType.cs - 225 - Do we know that MoveNext is always going to return true here such that Current will be valid?
ContentType.cs - 89 - It doesn't appear that we know at this point that contentType isn't empty. Couldn't this result in indexing out of bounds into contentType?

Also, what is the best way to work with changes related to the same issue from multiple people?
Please advise.

Contributor

alexsorokoletov commented Nov 8, 2015

Can someone please clarify following items:

PackUriHelper.cs - 695 - the chars array could be stackalloc'd instead of heap allocated.
InternalRelationshipCollection.cs - 592 - Debug.Fail?
ContentType.cs - 398 - There's a lot of string manipulation / allocation / etc. happening in these various helper functions. Doesn't need to be fixed now, as it's not clear that it's a problem. But it'd be interesting to do some perf testing / profiling and see what pops as hot spots that we'll potentially want to fix.
ContentType.cs - 225 - Do we know that MoveNext is always going to return true here such that Current will be valid?
ContentType.cs - 89 - It doesn't appear that we know at this point that contentType isn't empty. Couldn't this result in indexing out of bounds into contentType?

Also, what is the best way to work with changes related to the same issue from multiple people?
Please advise.

@Maxwe11

This comment has been minimized.

Show comment
Hide comment
@Maxwe11

Maxwe11 Nov 8, 2015

Contributor

PackUriHelper.cs - 695 - the chars array could be stackalloc'd instead of heap allocated.
InternalRelationshipCollection.cs - 592 - Debug.Fail?

done in #4408

Contributor

Maxwe11 commented Nov 8, 2015

PackUriHelper.cs - 695 - the chars array could be stackalloc'd instead of heap allocated.
InternalRelationshipCollection.cs - 592 - Debug.Fail?

done in #4408

hughbe added a commit to hughbe/corefx that referenced this issue Feb 9, 2016

hughbe added a commit to hughbe/corefx that referenced this issue Mar 4, 2016

Cleanup and reformat System.IO.Packaging
Removes strange comments
Removes empty finally block
Fixes #2699

hughbe added a commit to hughbe/corefx that referenced this issue Mar 20, 2016

Cleanup and reformat System.IO.Packaging
Removes strange comments
Removes empty finally block
Fixes #2699

@stephentoub stephentoub assigned stephentoub and unassigned ianhays Mar 20, 2016

@stephentoub stephentoub closed this in #5994 Mar 21, 2016

@karelz karelz modified the milestones: Future, 1.0.0-rtm Dec 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment