headers
Michael M Slusarz | 1 Oct 2003 19:14
Picon

Re: [PATCH] Adding quota info to IMP summary block

Quoting Etienne Goyer <etienne.goyer <at> linuxquebec.com>:

| Here is included a newer patch that is a bit cleaner and fix formatting
| issue with the first one.  If this is uncommitable, please tell so I can
| fix it (or drop the idea entirely).

Cleaned up and committed.  Thanks.

michael

______________________________________________
Michael Slusarz [slusarz <at> bigworm.colorado.edu]
The University of Colorado at Boulder

--

-- 
Horde developers mailing list
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe <at> lists.horde.org
Permalink | Reply |
headers
Etienne Goyer | 1 Oct 2003 19:23

Re: [PATCH] Adding quota info to IMP summary block

On Wed, Oct 01, 2003 at 11:14:14AM -0600, Michael M Slusarz wrote:
> Cleaned up and committed.  Thanks.

Thank you very much.  Just so I can improve the quality of my patch in
the future, which part needed cleaning ?

-- 
Etienne Goyer                    Linux Québec Technologies Inc.
http://www.LinuxQuebec.com       etienne.goyer <at> linuxquebec.com

--

-- 
Horde developers mailing list
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe <at> lists.horde.org
Permalink | Reply |
headers
Michael M Slusarz | 1 Oct 2003 20:37
Picon

Re: [PATCH] Adding quota info to IMP summary block

Quoting Etienne Goyer <etienne.goyer <at> linuxquebec.com>:

| On Wed, Oct 01, 2003 at 11:14:14AM -0600, Michael M Slusarz wrote:
| > Cleaned up and committed.  Thanks.
|
| Thank you very much.  Just so I can improve the quality of my patch in
| the future, which part needed cleaning ?

Duplicate/unnecessary code:
* You were checking for the existence of $_SESSION['imp'] although this
should already be taken care of earlier (if $_SESSION['imp'] isn't set, it
would crash long before you got to this block)
* You were calling an if() statement twice where the logic could be done in
a single if() statement
* You could simply append to the $html variable in one case instead of
storing in a temporary variable.  There was also no need to predeclare the
$message variable to ''.

Mainly, stupid nit-picky cruft that, after many, many, many, many hours of
staring at PHP/Horde code, you develop a nack for cleaning up :)  I'm
probably the only one that cares about this.

michael

______________________________________________
Michael Slusarz [slusarz <at> bigworm.colorado.edu]
The University of Colorado at Boulder

--

-- 
Horde developers mailing list
(Continue reading)

Permalink | Reply |
headers
Chuck Hagenbuch | 1 Oct 2003 20:44
Favicon
Gravatar

Re: [PATCH] Adding quota info to IMP summary block

Quoting Michael M Slusarz <slusarz <at> bigworm.colorado.edu>:

> Mainly, stupid nit-picky cruft that, after many, many, many, many hours of
> staring at PHP/Horde code, you develop a nack for cleaning up :)  I'm
> probably the only one that cares about this.

Hardly. I'm incredibly anal about stuff like this. Especially when I'm bored. :)

In all seriousness, it's a really good thing to always clean up stuff like that
when we can; it's had a huge impact in keeping our codebase clean and readable.
It's one of the things I'm most proud of about Horde.

-chuck

--
Charles Hagenbuch, <chuck <at> horde.org>
Born right the first time.

--

-- 
Horde developers mailing list
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe <at> lists.horde.org
Permalink | Reply |

Gmane