# Re: Code Review: zlib

**URL:** <https://www.ruby-forum.com/t/re-code-review-zlib/136400>\
**Category:** IronRuby\
**Created:** [May 1, 2008, 6:31pm UTC](https://www.ruby-forum.com/t/re-code-review-zlib/136400 "2008-05-01T18:31:06Z")\
**Posts on this page:** 1\
**Page:** 1

<div class="post-metadata">

**Author:** ![John\_LSSIRONRUBY](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/4491bb/32.png) [@John\_LSSIRONRUBY](https://www.ruby-forum.com/u/John_LSSIRONRUBY)\
**Post date:** [May 1, 2008, 6:31pm UTC](https://www.ruby-forum.com/t/re-code-review-zlib/136400/1 "2008-05-01T18:31:06Z")

</div>

Hi Michael,

Thanks so much for sending in this patch! I did a high-level review of  
the code (I didn’t attempt to analyze the algorithms that you’re using)  
for compatibility with the rest of the codebase, style, invariants etc.

I’m attaching a diff to this mail so that folks can follow along with  
the discussion. Note that the diff isn’t quite as useful as it might be  
in this case because the code formatting was different on the way in.

In the future, we can do a more detailed review of the algorithms but  
for now let’s just treat it as a black box.

Overall I think it’s awesome that it works as advertised. There’s just  
some work that needs to be done to make sure that the methods follow  
Ruby semantics.

I intend to check this code in as-is for the time being since we don’t  
have any tests that exercise it in our current test matrix. This should  
make it easier for folks to work on it instead of shipping around patch  
files on mailing lists. ETA should be tomorrow on this check-in.

# General comments:

1. Attach our standard MsPL copyright header to the top of files
2. Follow standard .NET naming conventions (PascalCaseMethodsAndClasses,  
\_instanceMembers). This is really easy to fix after the fact via VS  
refactoring tools, but it’s a matter of who does the work 🙂
3. Please spend some time thinking about your invariants, and in  
particular your non-nullable reference types (that’s what the /_!_/  
annotations mean). This will help you think about cases where you need  
to add explicit null checks vs. cases where you don’t.
4. Consider using #region to group chunks of code together
5. If there’s more work or missing work, a few TODO comments would help  
guide folks reading the code.
6. PLEASE format your code the way the rest of the source code is  
formatted, since this will make diffs on code reviews much more readable  
(there’s _way_ too much noise in this diff).

# Specific comments:

1. Using “initialize” to initialize your .NET invariants is a bad idea -  
someone could override initialize and invalidate your invariants. Move  
those things to a constructor instead.

Instead of:

```
        [RubyMethod("initialize")]
        public static ZStream/*!*/ Initialize(ZStream/*!*/ self) {
            self._outPos = -1;
            self._inPos = -1;
            self._bitBucket = 0;
            self._bitCount = 0;
            self._inputBuffer = new List<byte>();
            self._outputBuffer = new List<byte>();
            return self;
        }

```

Do:

```
        public ZStream() {
            _outPos = -1;
            _inPos = -1;
            _bitBucket = 0;
            _bitCount = 0;
            _inputBuffer = new List<byte>();
            _outputBuffer = new List<byte>();
        }

```

1. The HuffmanTree class:

a) does not appear to be a Ruby class  
b) should use properties with backing private fields

1. The GzipReader#open instance method:

This appears to be a private instance method in Ruby. You had it defined  
as a public instance method that creates a new instance of GZipReader. I  
doubt that this is correct. I suspect that it is used to initialize the  
current instance of GZipReader to the IO object that is passed in.

I believe that the code that you have in the GZipReader’s constructor  
should be moved to this method.

1. The GzipReader.open singleton methods:

You only have a single overload that handles the MutableString case. In  
most cases, Ruby libraries will do an implicit conversion via to\_str on  
any arbitrary object that is passed in as a parameter. You’ll need to  
use the [NotNull] attribute to force the call to the overload that  
accepts object for the case where the caller passes in nil.

Instead of:

public static GZipReader Open(CodeContext/_!_/ context, RubyClass/_!_/  
self, MutableString/_!_/ filename) { … }

Do

public static GZipReader Open(CodeContext/_!_/ context, RubyClass/_!_/  
self, [NotNull]MutableString/_!_/ filename) { … }

public static GZipReader Open(CodeContext/_!_/ context, RubyClass/_!_/  
self, object filename) {  
return Open(context, self, Protocols.CastToString(context, filename));  
}

1. The block method overload of GzipReader.open()

The signature wasn’t correct - the correct signature is:

public static void Open(CodeContext/_!_/ context, RubyClass/_!_/ self,  
BlockParam block, [NotNull]MutableString/_!_/ filename)

There were a few other problems in the original implementation

a) allocate just allocates memory - it doesn’t call initialize to  
initialize a new instance of a specific class; you’ll need to invoke new  
on that class.  
b) open is a private instance method, so you won’t need to invoke it via  
a site

# Tests:

I still have to sync to the latest rubinius specs which has a spec suite  
for zlib.

Thanks,  
-John