Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Group: Naming Inconsistencies #1547

Open
kylemcdonald opened this Issue Sep 6, 2012 · 77 comments

Comments

Projects
None yet
Contributor

kylemcdonald commented Sep 6, 2012

Naming inconsistencies are: poorly named methods, functions, constants, classes, and arguments.

This is a group of issues identified during Code Review 2012. Add any relevant issues to this group, and when they are all closed we will close this issue. If an issue fits this pattern, but is more significant or not a simple fix, please add it as a separate issue.

Contributor

underdoeg commented Sep 6, 2012

ofFbo::Settings::depthStencilAsTexture should be useDepthStencilAsTexture to be in sync with useStencil and useDepth https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/gl/ofFbo.h#L71

Contributor

ofZach commented Sep 6, 2012

ofGraphics::Command - non US spelling for "centre"

Contributor

ofZach commented Sep 6, 2012

we are using to and into to mean the same thing in ofpixels:

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/graphics/ofPixels.h

see for example, mirrorTo and pasteInto.

of3dUtils.h

should ofDrawAxis be renamed to ofDrawAxes to follow ofDrawRotationAxes plural..

Contributor

NickHardeman commented Sep 6, 2012

QTKitMovieRenderer.h and QTKitMovieRenderer.m should be ofQTKitMovieRenderer.h and ofQTKitMovieRenderer.mm

Contributor

ofTheo commented Sep 6, 2012

enums should not use hex notation. and should start with = 0 and not enumerate the rest of the list: https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/utils/ofConstants.h#L371

Except for ofOrientation which needs to start at 1 for iOS compatability

ofCamera.h

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/3d/ofCamera.h#L58-61

worldToScreen
screenToWorld
worldToCamera
cameraToWorld

should be prefixed with get

Contributor

ofTheo commented Sep 6, 2012

ofLoopType at top of ofConstants.h should be somewhere later and not hex notation.
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/utils/ofConstants.h#L8

Member

jvcleave commented Sep 6, 2012

ofAppBaseWindow.h

Change setupOpenGL to setup?
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/app/ofAppBaseWindow.h#L15

Remove "Window" from function names (e.g. setWindowPosition -> setPosition, this would effect every appWindow however)
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/app/ofAppBaseWindow.h#L22

enableSetupScreen/disableSetupScreen() could benefit with comment/explanation of usage
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/app/ofAppBaseWindow.h#L49

ofQuickTimePlayer.h

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/video/ofQuickTimePlayer.h#L76

allocated should be changed to b or is to mark as bool

Contributor

ofZach commented Sep 6, 2012

ofPolyline features hard coded default curve resolutions, ie:

int curveResolution=16

this should be a #define such as default_curve_resolution

QTKitMovieRenderer.h & QTKitMovieRenderer.h

the files should be prefixed with of

Owner

arturoc commented Sep 6, 2012

not sure this belongs here but why ofSetupPerspective gets an ofOrientation parameter? shouldn't we just use ofSetOrientation and ofSetupPerspective gets the orientation using ofGetOrientation? passing that parameter to setupPerspective doesn't really set the orientation and it actually has to be set to the current orientation of the screen to actually work properly

Contributor

ofTheo commented Sep 6, 2012

#define name clashes

PI -> OF_PI
TWO_PI -> OF_TWO_PI etc

DEG_TO_RAD - maybe we add ofDegreesToRadians(float deg) ? and slowly deprecate DEG_TO_RAD
MIN -> ofMin()
MAX -> ofMax()

CLAMP -> ofClamp
ABS -> ofAbs

https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/utils/ofConstants.h#L311

ofURLFileLoader.cpp

ofURLFileLoader::get should be should be changed to ofURLFileLoader::load and ofURLFileLoader::loadAsync

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/utils/ofURLFileLoader.cpp#L45

Contributor

kylemcdonald commented Sep 6, 2012

makeScaleMatrix and makeTranslationMatrix need named arguments instead of just floats.

Contributor

ofZach commented Sep 6, 2012

ofRenderCollection draw(ofMesh.....) would be great to have one method if at all possible (there are two now)

https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/graphics/ofRendererCollection.h#L39-45

Contributor

kylemcdonald commented Sep 6, 2012

ofQuaternion::length2() should be called ofQuaternion::lengthSquared() and ofQuaternion::conj() should probably be conjugate().

Owner

arturoc commented Sep 6, 2012

@justdayan ofURLFileLoader::get is called get because it uses the http GET method, at some point we would want to add POST which result would be similar to get and the load naming can be confusing, there could be a load method though that calls get and in the future if we add post it can have a method parameter but i think it's fine as it is

Contributor

ofTheo commented Sep 6, 2012

Owner

arturoc commented Sep 6, 2012

about custom constructors: i think we should add custom constructors to every class that has a load, setup or allocate method, it's not the most usual in OF but is a pretty used pattern in c++, if you create a class inside a function only to be used in that function it's shorter and useful. it also allows for one of the most characteristic patterns in c++: http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization

Owner

arturoc commented Sep 6, 2012

why do we have ofBuffer::getText and string() operator ?

yes we should probably deprecate that one, i added the cast operator later and didn't remove getText for backwards compatibility

Contributor

ofTheo commented Sep 6, 2012

ofFile::getSize returns uinit64_t - is this okay across all platforms?
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/utils/ofFileUtils.h#L142

ofQTKitGrabber.mm

listVideoDevices
listAudioDevices
listVideoCodecs
listAudioCodecs

should be:

getAudioDevices()
getVideoDevices()
getAudioCodecs()
getVideoCodecs()

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/video/ofQTKitGrabber.mm#L52-55

Contributor

ofZach commented Sep 6, 2012

ofRenderCollection:: bClearBg

https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/graphics/ofRendererCollection.h#L259

method starts with "b" should be isClearBg -- even then it's not super clear, maybe something like "isBackgroundAutoCleared()" or something is easier to understand.

Contributor

underdoeg commented Sep 6, 2012

ofLight::getIsEnabled -> isEnabled
ofLight::getIsDirectional -> isDirectional
ofLight::getIsSpotlight ->isSpotlight
ofLight::getIsPointLight -> isPointLight

Also isSpotLight has upper case L but isPointlight does not.

https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/gl/ofLight.h#L47

Contributor

NickHardeman commented Sep 6, 2012

@arturoc "ofURLFileLoader::get is called get because it uses the http GET method, at some point we would want to add POST which result would be similar to get and the load naming can be confusing, there could be a load method though that calls get and in the future if we add post it can have a method parameter but i think it's fine as it is"

I think a load function that has a method as an argument would be ideal.
Something like load(string url, string method) and loadAsync(string url, string method, string name)
The reason that it is confusing is that it is not consistent with the naming of the global function, which is ofLoadURL();
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/utils/ofURLFileLoader.h#L59

Owner

arturoc commented Sep 6, 2012

all the classes that have a load*() method should have a load() instead, we can deprecate or not the old ones

Contributor

ofZach commented Sep 6, 2012

ofPixels::allocate param needs to be better named;

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/graphics/ofPixels.h#L35

this should be ofPixelsFormat format, not ofPixelFormat type

Contributor

damian0815 commented Sep 6, 2012

ofBaseSoundPlayer: loadSound(), unloadSound() <-> setup/close

Contributor

ofTheo commented Sep 6, 2012

https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/math/ofMath.h

ofRandomf() ofRandomuf()

@kylemcdonald suggests ofRandom(float max = 1);

ofRandomuf() would then become ofRandom()

Not sure about ofRandomf() though. -> ofSignedRandom() ?

Also see #224. (Taking the liberty of editing-in-line here. Christoph)

Contributor

ofTheo commented Sep 6, 2012

https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/math/ofMath.h

ofRadToDeg -> ofRadiansToDegrees ?
ofDegToRad -> ofDegreesToRadians

or maybe its fine?

Contributor

damian0815 commented Sep 6, 2012

ofBaseSoundPlayer <-> ofBaseVideoPlayer consistency.

setVolume, setPan should be consistent with video. setLoop should be setLoopState. getIsPlaying() should be isPlaying / isPaused after ofBaseVideoPlayer. getDuration() needs to be added. isFinished() should be added.

Contributor

damian0815 commented Sep 6, 2012

ofBaseSoundStream <-> ofBaseVideoGrabber

listDevices should return a vector, setDeviceID should work like device selection on the ofBaseVideoGrabber.

Contributor

danomatika commented Sep 6, 2012

Global texture functions use "Tex" in naming while functions inside ofTexture use "Texture" ...

Owner

arturoc commented Sep 6, 2012

ofBaseVideoPlayer::close should be unloadSound.

i think close it's fine, ofSoundPlayer::unloadSound should be renamed to close

Member

julapy commented Sep 6, 2012

ofRectangle::canonicalize
ofRectangle::getCanonicalized
ofRectangle::isCanonicalized
are ambiguous.

should the process of having positive width and height be done internally/automatically?

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/types/ofRectangle.h#L66-68

Contributor

danomatika commented Sep 6, 2012

There are a number of bool getters in ofTexture.h that could be renamed:

  • ofGetUsingArbTex -> ofUsingArbTex
  • ofGetUsingNormalizedTexCoords -> ofUsingNormalizedTexCoords
  • ofGetUsingCustomTextureWrap -> ofUsingCustomTexWrap
  • ofGetUsingCustomMinMagFilters -> ofUsingCustomMinMagFilters

Also missing:

  • ofUsingTextureEdgeHack
Member

julapy commented Sep 6, 2012

ofRectangle::isEmpty
is ambiguous.

should it be,
ofRectangle::isZero ?

Member

julapy commented Sep 6, 2012

ofRectangle::getMin and ofRectangle::getMax are ambiguous.

something easier to read and understand could be,
ofRectangle::getTopLeftCorner
ofRectangle::getBottomLeftCorner
ofRectangle::getTopRightCorner
ofRectangle::getTopLeftCorner

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/types/ofRectangle.h#L76-82

Contributor

danomatika commented Sep 6, 2012

There are some weird function variable names in ofVbo: "vert0x", "color0r", "normal0x", "texCoord0x", ...

Also, the following getters could be renamed:

  • getIsAllocated -> isAllocated
  • getUsingVerts -> usingVerts or usingVertices
  • getUsingColors -> usingColors
  • getUsingNormals -> usingNormals
  • getUsingTexCoords -> usingTexCoords
  • getUsingIndices -> usingIndices
Member

julapy commented Sep 6, 2012

should ofBaseRenderer also include,
virtual void drawCube(float x, float y, float z, float size)=0; ?

Contributor

danomatika commented Sep 6, 2012

The ofCairoRender types are enums within the class namespace. Since OF currently does not use namespaces yet, they should probably be moved out into the global namespace and renamed to:

enum Type {
    PDF,
    SVG,
    IMAGE,
    FROM_FILE_EXTENSION
}

becomes

enum ofCairoType {
    OF_CAIRO_FILE_EXT, // guess from the file extension
    OF_CAIRO_PDF,
    OF_CAIRO_SVG,
    OF_CAIRO_IMAGE
}

Note: probably good to move the default (FROM_FILE_EXT) to be the first enum

Contributor

danomatika commented Sep 6, 2012

The CIRC_RESOLUTION define in ofGraphics.h should probably be in ofGraphics.cpp or at least renamed to something like OF_DEFAULT_CIRCLE_RESOLUTION.

Member

bakercp commented Sep 6, 2012

regarding ofBuffer::getText etc -- this should eventually accept an ofEncoding.

Member

bakercp commented Sep 6, 2012

@julapy similar functions are in the current ofRectangle #1513 PR

Contributor

danomatika commented Sep 6, 2012

Could ofBeginSaveScreenAsPDF & ofEndSaveScreenAsPDF in ofGraphics be renamed?

  • ofBeginSaveScreenAsPDF -> ofBeginPDF
  • ofEndSaveScreenAsPDF -> ofEndPDF
Member

bakercp commented Sep 6, 2012

@julapy regarding canonicalize -- that was added as a way around the fact that we support -w and -h. I totally agree, the wording is bad, but it was the closest thing I could find in other contexts (i.e. cinder, etc). Enforcing +w/+h internally is a bit tricky and would break backward compatibility.

Contributor

danomatika commented Sep 6, 2012

Could the ofFillFlag enum be renamed to ofFillType?

It could also just be a bool, as in ofGetFill() returns a bool, but I can see having it as an enum is good for future additions, such as a possible Gradient fill type.

In any case, I'd rename the enums. Maybe something like:

enum ofFillFlag{
    OF_OUTLINE= 0,
    OF_FILLED = 1,
};

to

enum ofFillType{
    OF_NO_FILL= 0,
    OF_FILLED = 1,
};

Also, there could be a bool function that checks if the current type is filled or not:
ofIsFilled or ofUsingFill ...

Member

julapy commented Sep 6, 2012

a bunch on functions in ofPolyline that perform smoothing, resampling etc should be able to perform the operation on themselves or return a new ofPolyline object.

ofPolyline::getSmoothed should have a matching function,
ofPolyline::smooth(int smoothingSize, float smoothingShape = 0);

ofPolyline::getResampledBySpacing should have a matching function,
ofPolyline::resampleBySpacing(float spacing)

ofPolyline::getResampledByCount should have a matching function,
ofPolyline::resampleByCount(int count)

ofPolyline::simplify should have a matching function,
ofPolyline::getSimplified(float tolerance=0.3)

Member

julapy commented Sep 6, 2012

code inside ofPolyline::getBoundingBox
can now be replaced by ofRectangle::growToInclude
so we are using the same code for the same operations in the framework.

Contributor

danomatika commented Sep 6, 2012

ofbClearBg() in ofGraphics.h should be renamed to something like ofIsBackgroundAuto to match ofSetBackgroundAuto.

Also, the comment line above is not needed anymore ...

Contributor

danomatika commented Sep 6, 2012

ofSetDrawBitmapMode in ofGraphics should match what it's actually doing ... setting the bitmap string mode. It could be renamed to ofSetBitmapStringMode.

This alos connects to the ofDrawBitmapMode in ofConstants. Is that only for the bitmap string drawing? if so, if could be renamed to ofBitmapStringMode.

Contributor

danomatika commented Sep 6, 2012

ofImage::setUseTexture and ofImage::isUsingTexture seem awkwardly named. The comment above even describes them as being used to "enable or disable" the image's built in texture.

They could be renamed to ofImage::enableTexture(bool) and ofImage::isTextureEnabled.

Contributor

danomatika commented Sep 6, 2012

The overall OF api can't decide between "Tex" versus "Texture" in function naming, see ofTexture.h and ofImage.h ...

Contributor

NickHardeman commented Sep 6, 2012

ofTrueTypeFont getStringAsPoints() and getCharacterAsPoints() both return an ofPath, should they be renamed something like getStringAsPaths() and getCharacterAsPath()?

getStringAsPoints
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/graphics/ofTrueTypeFont.cpp#L608

getCharacterAsPoints
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/graphics/ofTrueTypeFont.cpp#L554

Contributor

NickHardeman commented Sep 6, 2012

in ofMatrix3x3, transpose(const ofMatrix3x3& A) returns a matrix, should it be named getTransposed()?
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/math/ofMatrix3x3.cpp#L73

same for inverse(const ofMatrix3x3& A)
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/math/ofMatrix3x3.cpp#L128

Contributor

danomatika commented Sep 6, 2012

ofPath's fill, stroke, and color settings seem a little misnamed. They don't exactly match the ofGraphics calls.

Here's a suggestion:

  • setFilled() -> fill() / noFill()
  • setColor() & setHexColor() -> remove, see #1571
  • setFillColor() & setFillColorHex() -> setFill() & setFillHex()
  • setStrokeColor() & setStrokeColorHex() -> setStroke() & setStrokeHex()
  • hasOutline() -> is this needed?

Also, there is a setPolyWindingMode() but a getWindingMode() ... either use "poly" in the name or not.

Can flagShapeChanged() be renamed to something a little more clear like shapeWasChanged(), updateShape, updatePath, etc ?

Member

julapy commented Sep 6, 2012

in ofCairoRenderer::viewport
the viewport rectangle is made using ofGetWindowWidth and ofGetWindowHeight
shouldn't it be using ofGetViewportWidth and ofGetViewportHeight

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/graphics/ofCairoRenderer.cpp#L754-755

Contributor

ofZach commented Sep 6, 2012

ofDoesHWOrientation() seems like an odd function name. Wondering about calling this something more clear, such as ofGetDoesHWControlOrientation() or ofGetHWHasOrientationSupport()

Contributor

elliotwoods commented Sep 6, 2012

Interesting ambiguity between definite usage principle for naming 'get' and 'set' functions

Noting ofCamera, I used 'cameraToWorld' without 'get' as its not returning a property of the camera, but is performing an operation on the input. Do we think get should still be used in that case (I.e. when returning something which can't be considered a parameter of the class.

Contributor

NickHardeman commented Sep 7, 2012

@elliotwoods that makes sense. I'm thinking of the vector class with something like getRotated( float angle, const ofVec3f& axis ), where the function is also operating on the input. Although rotated() is also defined, which just returns getRotated()

Contributor

underdoeg commented Sep 13, 2012

I can start renaming those. I just need some hints on how to handle the changes. Should I mark them as deprecated? I heard with have some macro to do that?

This was referenced Jun 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment