Code, phpDoc and refactoring

[LastDragon]

It seems to me, or the code in the new version put to worse? Below are some "peculiarities" that seems to me to complicate further development.

1) A huge number of copy-paste - why not create separate methods?

2) A huge number of E_NOTICE - to find their own mistakes in them can not ... The fact that among them there are reports about the missing localization strings - you do not know, because "But we don't treat E_NOTICE as bugs." (http://community.invisionpower.com/tracker/issue-21221-fix-e-notice/)

3) A strange use of interfaces, for example, http://community.invisionpower.com/tracker/issue-21694-refactoring-usercpforms/

4) You've recently opened docs in phpDoc, it is good, BUT:
4.1) Why should declare the object type as "object"?

We need to define the type of object, and then watching his methods ... Better to use the class name, because In this case, available autocompletion (in Eclipse), which greatly speeds up the coding.

4.2) Why add an obvious attribute @access?

What bad points (1-3) can be found in "Code Complete" by Steve McConnell, And how to correct the situation in great detail is written in "Refactoring: Improving the Design of Existing Code" by Martin Fowler.

I hope that your code will be better.

[Mark]

1) Could you give an example? Obviously we create methods where appropriate, I'm sure there's a reason for anywhere we haven't.

2) E_NOTICE is not an error - this is a quote from php.net:

Run-time notices. Indicate that the script encountered something that could indicate an error, but could also happen in the normal course of running a script.

3) interface_usercp defines the necessary methods and properties needed for classes that implement it (which is the code for a tab in the user cp for an application). That's how interfaces work. It deliberately doesn't define optional methods or properties. PHP Interfaces.

4) I actually, personally agree with you on this one, and in my code don't define @access or use type "object" - but doing so is not incorrect. And it's a comment, it doesn't affect the speed of anything ;)

[LastDragon]

1) Could you give an example? Obviously we create methods where appropriate, I'm sure there's a reason for anywhere we haven't.

One example (from last): IPSLib::getAppDir() && IPSLib::getAppFolder() methods return essentially the same thing, but in different ways ... Why not use IPSLib::getAppFolder() in IPSLib::getAppDir()?

2) E_NOTICE is not an error - this is a quote from php.net:

This potential error!

For example, now you do not know that missing some localization strings:
Notice: Undefined index: clock_short2 in /admin/sources/classes/class_localization.php on line 191

Only one, because I'm using xdebug and find the error rather difficult (large image)...

Perhaps this is due to improper localization, but the point is that if it E_NOTICE such errors would be detected immediately. Now you dont know about them, but that does not mean there are no errors.

3) interface_usercp defines the necessary methods and properties needed for classes that implement it (which is the code for a tab in the user cp for an application). That's how interfaces work. It deliberately doesn't define optional methods or properties. PHP Interfaces.

Please do not give a link to php.net. The first version of my post contained a much more interesting questions, but it was edited ...

My opinion on why this interface is useless, I wrote in you Tracker.

Do you think that the need to examine your code to write a new tab, it's good?
If well-use interface that would not have to do.

4) I actually, personally agree with you on this one, and in my code don't define @access or use type "object" - but doing so is not incorrect. And it's a comment, it doesn't affect the speed of anything ;)

You are wrong. Now:

I must remember all the existing methods?

I think so much better:


I'll ask in this topic: "Using usercpFormsExt.php: extending tab OR splitting tab?"

I looked at the /admin/applications/core/modules_public/usercp/manualResolver.php and did not understand what you wanted to implement: the extending tab (other application) OR splitting a tab into two classes (implemented)?

[Mark]

getAppFolder uses the applications cache, getAppDir does not.
This is because in some areas (i.e. the installer) the applications cache isn't set up. getAppDir cannot rely on getAppFolder, but it's handy to have getAppFolder which will execute faster (since it doesn't have to run the check).

This potential error!

For example, now you do not know that missing some localization strings:

Notice: Undefined index: clock_short2 in /admin/sources/classes/class_localization.php on line 191

Only one, because I'm using xdebug and find the error rather difficult (large image)...

Perhaps this is due to improper localization, but the point is that if it E_NOTICE such errors would be detected immediately. Now you dont know about them, but that does not mean there are no errors.

If you're going to argue with the PHP manual, I don't really know what to say to that.

I get your point, E_NOTICE is generated for a reason, and when developing we have it turned on. But adding isset calls for the sake of avoiding E_NOTICE (which should be turned off in a live environment) is silly.
You're right, they can be useful to detect errors, but you're wrong in saying that they are errors in themselves. They're notices of potential problems.

Please do not give a link to php.net. The first version of my post contained a much more interesting questions, but it was edited ...

My opinion on why this interface is useless, I wrote in you Tracker.

Do you think that the need to examine your code to write a new tab, it's good?

If well-use interface that would not have to do.

I'm not saying the code cannot be improved at all and I'm sure whoever deals with your report will make a decision on it.
But I disagree that it's bad programming practice, it makes use of the interface how they are intended. Just because it's different to how you might use it, doesn't make it incorrect.

You are wrong. Now:

I must remember all the existing methods?

I think so much better:

Well that's something php Eclipse is doing, not our code.

None of our developers use that program.

[Guest]

getAppFolder uses the applications cache, getAppDir does not.

This is because in some areas (i.e. the installer) the applications cache isn't set up. getAppDir cannot rely on getAppFolder, but it's handy to have getAppFolder which will execute faster (since it doesn't have to run the check).

In general, I disagree with the topic, but this particular example is really poor form. There should be one method that tries to get the value from the cache, and on failure, falls back to the alternative method. To have two methods is bad, especially when they're named very similarly, but in a way that does not make their differences obvious.

The fallback is the logical solution, but worst case you could at least have a parameter for it, i.e. getAppDir($skip_app_cache = false) or similar, for situations in which you know for sure the cache is unavailable.

[Brandon D]

You guys have E_NOTICE on when developing? I don't see how you do it. I have to turn it off whenever I work with IPB, as most pages have tons of notices generated by IPB and seeing any relevant ones from my own code would be fun to say the least.

I think the purpose of using @access is for the phpDocs where you might not have access to the code itself at the time.

[LastDragon]

getAppFolder uses the applications cache, getAppDir does not.

Hmm... we have а different code?

static public function getAppDir( $app, $module='' )

	{

		$location = '';


		if ( ! $app )

		{

			return FALSE;

		}


		/* Ok, chicken and egg scenario. Applications has not been set up - most likely because

		   we're using this function before the caches have been loaded and unpacked.

		   So we guess based on folder names.... */

		if ( ! is_array( ipsRegistry::$applications ) OR ! count( ipsRegistry::$applications ) OR ! isset( ipsRegistry::$applications[ $app ] ) )

		{

			$location = self::extractAppLocationKey( $app );

		}

		else

		{

			$location = ipsRegistry::$applications[ $app ]['app_location'];

		}


		$pathBit       = IPS_ROOT_PATH . 'applications';

		$modulesFolder = ( IPS_AREA != 'admin' ) ? 'modules_public' : 'modules_admin';


		switch ( $location )

		{

			default:

			case 'root':

				$pathBit .= '/' . $app;

			break;

			case 'ips':

				$pathBit .= '_addon/ips/' . $app;

			break;

			case 'other':

				$pathBit .= '_addon/other/' . $app;

			break;

		}


		if ( $module )

		{

			return $pathBit . "/" . $modulesFolder . "/" . $module;

		}

		else

		{

			return $pathBit;

		}

	}


	static public function getAppFolder( $app )

	{

		if ( ! $app OR ! isset(ipsRegistry::$applications[ $app ]) )

		{

			return FALSE;

		}


		switch ( ipsRegistry::$applications[ $app ]['app_location'] )

		{

			default:

			case 'root':

				$pathBit = 'applications';

			break;

			case 'ips':

				$pathBit = 'applications_addon/ips';

			break;

			case 'other':

				$pathBit = 'applications_addon/other';

			break;

		}


		return $pathBit;

	}

Both methods use the cache, the difference only if the application is not in the cache. + Instead of

if ( ! is_array( ipsRegistry::$applications ) OR ! count( ipsRegistry::$applications ) OR ! isset( ipsRegistry::$applications[ $app ] ) )

You can use

if ( ! isset( ipsRegistry::$applications[ $app ] ) )

But I disagree that it's bad programming practice ...

I am sure your opinion about many things will change after reading the above books.

Well that's something php Eclipse is doing, not our code. None of our developers use that program.

Guys, this is not about who is using Eclipse or not. The correct data types substantially increase development speed and reduce errors. You do not have to memorize more than 9000 methods and 1000 objects - let the machine work for you.

The faster the speed of development, the more features can be implemented.

If you're going to argue with the PHP manual, I don't really know what to say to that.

php.net also said that:

Note: Enabling E_NOTICE during development has some benefits. For debugging purposes: NOTICE messages will warn you about possible bugs in your code. For example, use of unassigned values is warned. It is extremely useful to find typos and to save time for debugging. NOTICE messages will warn you about bad style. For example, $arr[item] is better to be written as $arr['item'] since PHP tries to treat "item" as constant. If it is not a constant, PHP assumes it is a string index for the array.

You guys have E_NOTICE on when developing? I don't see how you do it.

I'm using error_reporting() in my methods. Everything is fine, if not call showError()...

[Brandon D]

I'm using error_reporting() in my methods. Everything is fine, if not call showError()...

I'm aware -- I was responding to Mark. I simply meant that I don't use E_ALL with IPB even though I'd like to because any relevant notices are generally lost in ones generated by IPB.

[bfarber]

We turn on E_ALL during last stages of development, clean up a few things here and there, and turn it off for release. We do not full-time develop with E_NOTICE on (I don't at least) generally speaking. As Mark already pointed out, they're notices not errors.

While we can take your feedback into account, I'd urge you to keep in mind that IPB has been around nearly a decade. IPB 3.0 is not our first release, we're not some startup that doesn't know how to develop with PHP. Nevertheless, if you have constructive feedback on how you feel the application can be improved from a developer's perspective, by all means share it. For instance, your getAppDir and getAppFolder suggestion is something we can look into in the future.


I like your suggestion about objects in the phpdoc comments (none of our editors auto-suggest methods from linked classes, at least mine doesn't, but I can see how that would be useful if your editor did). However, phpdoc comments are not a bug, they don't slow down the codebase, and are something that it's hard to justify spending enormous amounts of time on (there's a LOT of code in IPB, and it's not just a simple matter of changing object to the class name expected when you're talking 100's of files with 1000's of methods). Perhaps we can evolve that over time, as we come across comment blocks.

@access is used to indicate visibility, especially when the docs are exported. I hardly see why there would be a complaint about this. If you don't need the info, don't look at it. Yes, it can be determined from looking at the keyword in front of the method when you have the code opened up. It still doesn't hurt to put the phpdoc keyword in the comments in any way shape or form, so I could never justify spending days to remove this when it doesn't hurt anything, personally.

[LastDragon]

We turn on E_ALL during last stages of development, clean up a few things here and there, and turn it off for release. We do not full-time develop with E_NOTICE on (I don't at least) generally speaking. As Mark already pointed out, they're notices not errors.

This is not surprising. With such a huge number of E_NOTICE .... Find something among them is impossible.

But everything will be different if we assume that during normal operation in the code is no E_NOTICE. Once you make a mistake in the variable name (a simple typo, it happens ...) -> E_NOTICE -> you fix it immediately. Now: typo -> release -> bug -> fix.

Which variant is better?

While we can take your feedback into account, I'd urge you to keep in mind that IPB has been around nearly a decade. IPB 3.0 is not our first release, we're not some startup that doesn't know how to develop with PHP. Nevertheless, if you have constructive feedback on how you feel the application can be improved from a developer's perspective, by all means share it. For instance, your getAppDir and getAppFolder suggestion is something we can look into in the future.

Please, no offense.

According to my observations you have 2 types of code:
1) Normal
2) Horrible (which I criticize)

Horrible code becomes larger with each release => number of errors will become more and more. Correct if I'm wrong - even now most of the errors are very stupid. (They are already perceived as a feature of IPS code. It's sad. Do you agree?)

Alternative methods getAppDir and getAppFolder (not tested):

    /**

     * Generates the app [ -> module ] path. The module is optional, if module is not

     * specified then just the app dir is returned. If this is called from the ACP and module

     * is present, then it'll return modules_admin, otherwise modules_public

     *

     * @access    public

     * @param    string        application

     * @param    string        module (optional)

     * @return    mixed        Directory to app or module (or false if error)

     */

    static public function getAppDir( $app, $module='' )

    {

        if ( ! $app )

        {

            return FALSE;

        }


        $location = self::extractAppLocationKey( $app );

        $pathBit  = IPS_ROOT_PATH;


        switch ( $location )

        {

            default:

            case 'root':

                $pathBit .= 'applications/' . $app;

            break;

            case 'ips':

                $pathBit .= 'applications_addon/ips/' . $app;

            break;

            case 'other':

                $pathBit .= 'applications_addon/other/' . $app;

            break;

        }


        if ( $module )

        {

            $modulesFolder = ( IPS_AREA != 'admin' ) ? 'modules_public' : 'modules_admin';


            $pathBit .= "/" . $modulesFolder . "/" . $module;

        }


        return $pathBit;

    }


    /**

     * Extracts app_location from app key

     *

     * @access    public

     * @param    string        File path

     * @return    string        root, ips, other

     */

    static public function extractAppLocationKey( $app )

    {

        $location = 'other';


        if ( isset( ipsRegistry::$applications[ $app ]['app_location'] ) )

        {

            $location = ipsRegistry::$applications[ $app ]['app_location'];

        }

        else if ( is_dir( IPS_ROOT_PATH . 'applications/' . $app ) )

        {

            $location = 'root';

        }

        else if ( is_dir( IPS_ROOT_PATH . 'applications_addon/ips/' . $app ) )

        {

            $location = 'ips';

        }


        return $location;

    }


    /**

     * Generates the app folder name, either "applications" or "applications_addon"

     *

     * @deprecated use self::getAppDir($app) instead

     *

     * @access    public

     * @param    string        application

     * @return    mixed        Directory to app or module (or false if error)

     */

    static public function getAppFolder( $app )

    {

        trigger_error('Deprecated. Use IPSLib::getAppDir($app) instead', E_USER_NOTICE);


        return self::getAppDir($app);

    }

However, phpdoc comments are not a bug, they don't slow down the codebase, and are something that it's hard to justify spending enormous amounts of time on (there's a LOT of code in IPB, and it's not just a simple matter of changing object to the class name expected when you're talking 100's of files with 1000's of methods). Perhaps we can evolve that over time, as we come across comment blocks.

I do not call to leave everything and begin to edit all comments. To get started, simply fix ipsCommand (it is used most often) and other core classes (not all). New methods, of course, should be written correctly.

@access is used to indicate visibility, especially when the docs are exported. I hardly see why there would be a complaint about this. If you don't need the info, don't look at it. Yes, it can be determined from looking at the keyword in front of the method when you have the code opened up. It still doesn't hurt to put the phpdoc keyword in the comments in any way shape or form, so I could never justify spending days to remove this when it doesn't hurt anything, personally.

@access appeared in PHP 4 when no access modifiers. Now the only situation where it may need - to remove a public method of documentation. In other cases, it is useless.

Add it simple. But now you have to track changes in the declarations of methods and modify it. It takes a little time. But you can forget about it. The result - the documentation does not reflect reality. With the accumulation of such errors, it will be useless and harmful. In the end, we return to start...




Nobody answered my question about usercp:

I looked at the /admin/applications/core/modules_public/usercp/manualResolver.php and did not understand what you wanted to implement: the extending tab (other application) OR splitting a tab into two classes (now implemented)?