Webdav/review.txt

Review Alexandru 17-10-2007
---------------------------

[x] The docs folder contains many text files - are they to become pages in the
online documentation? I think the place to put this temporary text files is
design or even the root of Webdav. (the root is the place to put TODO anyway)

- The option class ezcWebdavMemoryBackendOptions contain constants defined
which are used in ezcWebdavMemoryBackend (and maybe other classes). Is this
the correct way to do things? I think the constants should be defined
in ezcWebdavMemoryBackend.

# TS: I don't think this is important enough to change it. In fact, the
# constants are used in the options so why declare them in the class itself?

[x] There is no tutorial.

- ezcWebdavMemoryBackend, ezcWebdavSimpleBackend and maybe other classes have
the extends and implements on many lines - I thought it was already discussed
on IRC to have everything on one line.

# TS: Did not notice that. Please clearify and add it to the guidelines so we
# can make it consistent.

- Sometimes constructors are documented with '@return void'. I thought we
don't write the '@return void' anymore (for constructors much less).

# TS: Yes, but we did not consider this as an important fix to remove it.

- Some methods don't have code blocks.

[x] Some classes have the description 'Description missing'.

- Instead of array_key_exists( $propertyName, $this->properties ) it can be
isset( $this->properties[$propertyName] ) (in __isset() methods) for speed.

# TS: No, it cannot. We must use array_key_exists() since we want to work
# around the problem that isset() returns false for properties that do exist
# but are null.

- Almost all the components have this order in the docblocks for methods:

   - short description
   - detailed description, examples
   - @throws
   - @param
   - @return

  Why in Webdav the order is different?

# TS: This does not really matter. However, I'll try to pay attention to it
# and fix it if i see it during development.

[x] In the structs folder the class variables are defined after the constructor.

[x] In the requests folder the @copyright and @license is used 2 times.


Review Alexandru 31-07-2008
---------------------------

- Some typos in the code and comments:

[x] /src/properties/resourcetype.php
    TYPE_RESSOURCE -> TYPE_RESOURCE

# TS: Kept the old constant for BC reasons.

[x] /src/transport.php
    retreiveBody -> retrieveBody

# TS: Kept old method for BC reasons, calling the new method only.

/src/path_factories/basic.php
  collectionPathes -> collectionPaths

/src/path_factories/automatic.php
  collectionPathes -> collectionPaths
  [x] + collectionPathes is not defined as a class variable

# TS: I think we should keep these 2 as they are, since emulating the BC using
# interceptors would lead to reference issues.

/src/structs/collection.php
  childs -> children

# TS: Same here.

several files:
[x]  pathes -> paths !!!
[x]  ressource -> resource
[x]  instanciate -> instantiate
[x]  powerfull -> powerful
[x]  childs -> children
[x]  @throws appear multiple times with the same exception (eg. __get)

- Some guidelines issues:

/src/options/backend_file_options.php
  useMimeExts -> useMimeExtensions

# TS: Changing this would break BC. I don't think we should do it in this
# minor case.

[x] /src/server.php
    properties are not documented (even if read-only)
    constructor is protected, but docblock says private

[x] /src/backends/simple.php
    var_dump leftover

[x] /src/structs/display_information*.php
    class variables appear after the methods definitions