![[de]](/mwf/flags/de.png "Germany")
I noticed that different people use f.e. the access specifiers (public, private, protected) completely differently to the extent that you can see who wrote the code by looking at it.Sooo, what are our guidelines?
My personal guidelines are about this:
* if it's supposed to be called from the outside, make it public
* no specifier otherwise
Also comments, - do we want capitalization and proper punctuation or not?
I'd still write protected and private even though it is ignored to some sense. public is the default so having no specifier otherwise does not make sense to me. Also for me it looks strange if some functions do not come with a access specifier.
I always prefer proper English in any situation, especially the use of smileys, ... and !!! in code is just horrible.
I always prefer proper English in any situation, especially the use of smileys, ... and !!! in code is just horrible.
>public is the default
How is it more default than
private? They all have no effect :D>I always prefer proper English in any situation, especially the use of smileys, ... and !!! in code is just horrible.
Ok, nothing at all against that. But we should all do that and possibly add it to our style guidelines?
At least keeping the access identifiers allows for replacing them easily.
I think we can rid of protected somehow. Private makes still sense to me.
I think we can rid of protected somehow. Private makes still sense to me.
private if it's not supposed to be called from outside and nothing otherwise.protected doesn't really make sense because engine callbacks are part of the public interface. There's no reason for a script to not call them.
The only exception might be timer/effect callbacks. But since the engine can also call private, they can as well be private.
I'd prefer Zapper's way. Often it doesn't feel quite right to declare something private, I can't really overlook all of the possible use cases. But I can easily say that I intended a function to be called from outside.
private declaration is not a matter of "could this function be useful for someone?", but rather if it's part of the interface. If we don't have defined interfaces, it's impossible to improve scripts without possibly breaking external code that relied on certain function or variable names.We have a horrible amount of regression bugs in scripts. For example, this commit fixes a bug caused by an #appendto accessing an internal variable that was renamed. If internal variables and functions were marked as private, this would not have happened.
>We have a horrible amount of regression bugs in scripts. For example, this commit fixes a bug caused by an #appendto accessing an internal variable that was renamed. If internal variables and functions were marked as private, this would not have happened.
Please be realistic - it would either have happened exactly the same OR the original author would first have removed the
private specifier... :)At least that is how we have handled such things in C4Script for the last 10 years or so...
PS: also,
public would mark functions as part of the interface just as well as a missing private? Or do I misunderstand you here?
Traditionally, leaving out the specifier means the declaration is public, yes. Right now this doesn't matter of course, because the access specifier is simply ignored by the engine. It's like a comment that other scripters may or may not recognize.> Please be realistic - it would either have happened exactly the same OR the original author would first have removed the private specifier... :)
I think the author would have introduced getters/setters (GetAiming()/SetAiming()) and the person who refactored the script would have seen these functions and adjusted them accordingly.
> If internal variables and functions were marked as private, this would not have happened.
Wouldn't it? If it was still possible (technically), these kind of FYI markups are most likely to be ignored. If we'd again introduce some mechanism for 'private' (like 'do not access via #appendto'), you are still stuck with the same problem: How do I, as the person writing the javelin, determine whether fAiming is an internal variable or part of an interface?
If private was just a markup (that's what it's now, right?) , you'd still have to rely on to things that were ignored in your example: The person accessing the fAiming variable should have thought about writing get/set functions into the javelin; making fAiming private would be a reminder to do that (but could also be ignored what is likely to happen). Otherwise, i.e. fAiming was not private, the person changing it to aiming should have made a complete search for all occurences of fAiming and announce publicly a script-breaking change. There is no reminder to do that and even if, could have been ignored again.
Yeah people can ignore the access marker, but that's still better than dropping all qualifiersBecause without any indication, a scripter can't know which functions are allowed to be used and which are not. So even people who want to do it properly are screwed.
Yeah, but this is not really an argument why private is better than public (as Clonkonaut and I suggested) :I
What Zapper said:> Yeah, but this is not really an argument why private is better than public (as Clonkonaut and I suggested) :I
Also, what I mentioned to begin with. I am always very sure when writing a function that I want it to be called from somewhere else. I am never really sure when to forbid calling it from somewhere else. Out of caution you might end up with very few private functions/variables. Or you specifically say that everything that's not definitely intended to be called from elsewhere (which I would mark with public) should be private. Then using public saves some time.
Doesn't really protect you from regression though. It's still very possible for that to happen, only then you can blame someone.
Oh I thought the argument was about whether private/public should exist at all, not about which should be the default.For the default I don't care, except that existing scripts may assume that the default is public, because that is how the engine used to behave.
Yes and we should maybe emit warnings for a while when a private function is being accessed from outside the object, and not have errors there for some time.
> except that existing scripts may assume that the default is public, because that is how the engine used to behave.
We should say then, from now on the default is private. I tried to explain my struggle to really say when something's private. People writing #appendto should therefore always expect stuff to change when it's not specifically set to public.
![[tr]](/mwf/flags/tr.png "Turkey")
From other script languages I am used to public being the default and certainly newcomers to our (C4Script) code might expect the same. For me, it doesn't matter anyway since I never leave out the access declaration. I am a friend of clearly defined interfaces. (Directly accessing variables from a different object is problematic there.
We're still missing properties (=local variables) from the list. My idea would be the following:private properties have lowercase names and public interface properties are uppercase.
Currently, most properties follow this convention. Most local variables are lowercase, except for stuff that was moved from the DefCore like "Name", "Description" or "Collectible". They are all part of an interface that shouldn't change.
External objects that store stuff in other objects should also use lowercase. E.g. in adventures you may do stuff like GetCursor().has_visited_smith = true; which is technically public but not part of an interface.
Using properties instead of functions (like SetName()/GetName()) is rare, but may sometimes be done for performance reasons or if overloading should be simplified (e.g.: CreateObject(Firestone).ExplodeSize = 100) or simply when they're used as symbolic constants.
What about properties of proplists that are part of a public interface? They are (mostly) lowercase currently.F.e.
Mushroom->Place(..., {size = 100, allow_tunnels = true}); instead of Mushroom->Place(..., {Size = 100, AllowTunnels = true});At least for me, this looks more intuitive in lower case, especially since I mentally connect CamelCase stuff in scripts with functions.
(This is not a veto against your proposal, which I think is sensible!)
Based on the discussion I would suggest to extent our C4Script Style Guidelines with the following:----------------
Methods that are part of a public interface and intended to be called from other objects or scripts, should be prefixed with the
public keyword. As a part of the public interface of an object public methods should be documented with comments accordingly.Don't use an access specifier for other methods.
private and protected are deprecated.----------------
That would also help us when we potentially want to generate an automatic documentation some day - it would just take all public methods and assume other people are interested in them.
That is fine for me, but I'd still specify the private keyword. It just looks odd having one type of functions without keyword. Please also write something about global access specifiers, so that people are not confused.
But then we'd again have two states (interface/non interface) and three markup options, which is very redundant (not to say confusing for newcomers)!I think the aesthetical aspects of leaving out an access specifier are highly subjective :) Maybe it looks cleaner to you when grouping all the
public functions at the top of the script so they are not mixed with the others? That'd be a good idea anyway, I guess.I will add something about
global functions!
global is already documented, since it's the only specifier that has actual functional meaning for the engine.
(I am with Maikel here:)The meaning of leaving out an access modifier (which would be private as of your decision) will be "hidden" somewhere in the wiki, in a part of the wiki that is for contributors to OC, not general users of C4Script.
The code however should be understandable for anyone reading it. So if a method should only be called from local context (=is a private method) then this is an important information the reader will want to know without searching for the "meaning" of this in our code guidelines.
Sorry to bring this up again even though you tried to bring this discussion to a result but from this perspective it begs the question where it makes sense at all to NOT supply an access modifier for interface documentation's sake. The only use case I can come up with are quick'n'dirty scripts where the author simply doesn't care about where the methods are called from or some callback functions which aren't really part of the documented interface but still callable from the outside.
In both cases leaving out the modifier would be like 'public', the same as in most other (script) languages. (See my previous point)
Yes, always writing the identifier sounds like a good compromise. I'll try to do that from now on.> In both cases leaving out the modifier would be like 'public', the same as in most other (script) languages. (See my previous point)
Since the engine ignores all specifiers anyway, there's no need to discuss what leaving it out "means". But if the engine is ever changed to recognize the "private" keyword again, then functions without the keyword will have to be public anyway for compatibility reasons.
Now what about "protected"?
Why does the engine ignore the keywords?
The only effect the keywords had was to make the calling of the functions more complicated - you had to use PrivateCall and ProtectedCall instead of -> or Call. They didn't have any upsides, just some error messages the engine could whack you with. That was clearly crazy, so I simplified it to just Call and ->. Usage of the keywords do not produce error messages or warnings because that would drown out more useful messages. Though perhaps the ExtraWarnings option should do that.Now, some actually useful "private" functionality would be the ability to have a private func Foo in object Bar, and a global func Foo, where CreateObject(Bar)->Foo() calls the global func, while functions in Bar's script doing Foo() call the private one. That would actually help with resiliency against changes.
The idea of data hiding isn't just to annoy people or to prevent problems with overloads. It's also to allow creation of an interface and prevent scripters from relying on object internals. Because otherwise you get bugs in all kinds of scripts whenever any internals of an object are changed.Whether this is strictly enforced, just a warning or even done through a convention (stuff like "all private function begin with an underscore"), doesn't matter much. But at least there should be some way to say "don't access this directly" so good scripters can write sane scripts.
> But at least there should be some way to say "don't access this directly" so good scripters can write sane scripts.
Uh, no. That has actually never worked.
When I am writing a scenario or an expansion pack and for some functionality MUST access the internal of one of the original objects, I will always do that. Don't forget that only the core team has the ability to "just quickly write a setter/getter" instead of accessing a local variable directly.
Everyone else either has to use the internals or give up a feature...
And saying "here is a patch guys, I want to release my pack next week and it really needs this getter/setter" is not exactly viable either... Most people will always take features over upward compatibility, I guess.
Yeah. You are describing the reason why there was a PrivateCall in the first place in Clonk (Rage). That doesn't invalidate Sven2's point though (if that even was your intention).
Of course I know how important code modularity is and how much money it can save you in a bigger company :)My point is that it doesn't really help US in this situation if we try to enforce it. Other companies can guarantee stuff like "If you only use our API interface, your stuff won't break for at least two major releases!" - we can't. A lot of our changes also affect public interfaces.
My other point was that our public interfaces only cover what WE needed as an interface. Not what third-party content developers needed. Sometimes our requirements will cover their's, sure. But if you really, really need a bow that reloads faster on Wednesdays, you will most likely have to "hack" the old bow in some way and change some internals.
And hey! When you do that with an
appendto, you won't have to care about PrivateCall anyway, since the script becomes part of the original object and is thus allowed to access variables etc. anyway.So I think the best we can do is give a lead on how we intended the methods to be used (aesthetical
private/public). And if the lead is "I didn't really plan this to be accessed from outside" then the one hacking the object has to figure out themselves if they really need to change/call that method anyway...
As more people seem to be in favour of always using private&public now..-------------------------
Non-global methods that are part of a public interface and intended to be called from other objects or scripts, should be prefixed with the
public keyword. As a part of the public interface of an object public methods should be documented with comments accordingly.Non-interface methods should be prefixed with the
private keyword. Do not leave out the access specifier. The keyword protected is deprecated.-------------------------
?
Powered by mwForum 2.29.7 © 1999-2015 Markus Wichitill
