Advise on Class structure

Started by
5 comments, last by Jason Jones 9 years, 8 months ago
Hi, I have a couple of classes here, are they okay, how could I improve them,
thanks in advance . ..

class mesh_object
{
public:
    LPD3DXMESH              m_Mesh;
    LPD3DXMESH              sub_set;
    LPD3DXMESH              inplace;
    LPD3DXMESH              sub_Surface ;
    LPD3DXMESH              tm_mesh;
    LPD3DXMESH              Quad_Clone;
    LPD3DXMESH              Bounding_Box;
    LPD3DXMESH              m_worldAxisCursor;
    LPD3DXMESH              m_localdAxisCursor;
    LPD3DXMESH              pCursor;
    LPD3DXMESH              LINE_SEG_MID_POINTS ;
    LPD3DXMESH              e_tmp;
    D3DVECTOR*              v_quad_ptr;
    DWORD dw_select;
    DWORD dw_select_II;
    WORD* v_sel;
    DWORD v_num; 
    D3DVECTOR v_Center;
    materials*             mat_ptr;
 
    mesh_object();
    void createSphere(  LPDIRECT3DDEVICE9&  );
    void Build_Cursor(  LPDIRECT3DDEVICE9&  );
    LPD3DXMESH createAxisCursor(  LPDIRECT3DDEVICE9&   );
    void render( HWND,
                 LPDIRECT3DDEVICE9&, 
                 materials*,
                 matrix, 
                 LPD3DXMESH, 
                 LPD3DXMESH,
                 LPD3DXMESH,
                 LPD3DXMESH,
                 LPD3DXMESH,
                 LPD3DXMESH,
                 bool,
                 bool,
                 float 
                );
 
   LPD3DXMESH Generate_Normals(LPD3DXMESH );
   LPD3DXMESH mesh_x_rotation(LPD3DXMESH, float, DWORD );
   LPD3DXMESH mesh_y_rotation(LPD3DXMESH, float, DWORD );
   LPD3DXMESH mesh_z_rotation(LPD3DXMESH, float, DWORD );
   LPD3DXMESH update_cursor( LPD3DXMESH  );
   D3DVECTOR Zero_Bounding_Center( LPD3DXMESH ); 
   D3DVECTOR Get_Bounds(LPD3DXMESH );
   D3DVECTOR Get_Bounds_1(LPD3DXMESH );
   mesh_object re_initialize( mesh_object );
   LPD3DXMESH update_mesh_cursor( LPD3DXMESH , D3DVECTOR   );
   LPD3DXMESH  Set_Mesh_Color(LPD3DXMESH );
   LPD3DXMESH  Set_Mesh_Color_WHITE(LPD3DXMESH );
   LPD3DXMESH move_mesh( LPD3DXMESH , float , float , float  );
   LPD3DXMESH  inializeColors( LPD3DXMESH p_mesh,
                                 int pointStep );
   bool CheckOrientation( LPD3DXMESH );
   LPD3DXMESH Get_Face( LPD3DXMESH ,  DWORD );
   DWORD Select_face(LPD3DXMESH , HWND, LPDIRECT3DDEVICE9&  );
   DWORD Get_Next_Face( LPD3DXMESH p_mesh,  DWORD dw_index  );
   LPD3DXMESH GetLastVertex( LPD3DXMESH, DWORD, DWORD );
   WORD* mesh_object::GetIndex( LPD3DXMESH p_mesh, WORD* wrd_ptr );
   LPD3DXMESH SetAffineLocators( LPD3DXMESH, WORD* );
   LPD3DXMESH GetAffineCenter(LPDIRECT3DDEVICE9&,
                               LPD3DXMESH, 
                               LPD3DXMESH,
                               WORD* );
 
 
    LPD3DXMESH SetAffineCenter( LPDIRECT3DDEVICE9& p_device,
                                LPD3DXMESH p_mesh, 
                                LPD3DXMESH e_mesh,
                                WORD* w_ptr );
 
    D3DVECTOR GetBounds(LPD3DXMESH, int );
    DWORD PreProcessQuad( LPD3DXMESH p_mesh  );
 
    LPD3DXMESH create_mesh_from_index( LPDIRECT3DDEVICE9& p_device,   
                                       WORD* p_Index,      
                                       LPD3DXMESH p_mesh,      
                                       int n_point );
    FLOAT_QUAD Get_Mid_Point_RC( LPD3DXMESH , DWORD   );
    D3DVECTOR* getLocatorVectors( LPD3DXMESH,
                                   FLOAT_QUAD , 
                                   D3DVECTOR* 
                                 );
 
    D3DVECTOR GetspecificVector( D3DVECTOR, 
                                 LPD3DXMESH ,
                                 int  );
 
    LPD3DXMESH CloneAffineObject( LPDIRECT3DDEVICE9& p_device, 
                                  LPD3DXMESH, 
                                  WORD* );
 
 
 
};
 
class arbitrary_transform
{
public:
 
    bool b_affine_scale;
    D3DXVECTOR3* v_normal;
    D3DXVECTOR3* v_rotation_normal;
    D3DXVECTOR3* v_rotation_normal_A;
    D3DXVECTOR3* v_normal_II;
    D3DXVECTOR3* v_cursor_normal;
    D3DXVECTOR3* v_edit_object_normal;
    D3DXVECTOR3* Align_Vec;
    D3DXMATRIX arb_transform;
    D3DXMATRIX normal_transform;
    D3DXMATRIX edit_object_transform;
    D3DXMATRIX re_rotation;
    D3DXMATRIX re_rotation_A;
    D3DXMATRIX local_Transform;
    D3DXMATRIX ROTATION_B;
    D3DXMATRIX ROTATION_A;
    D3DXMATRIX  secondary_transform;
    D3DXMATRIX scale_Transform;
    D3DXVECTOR3* aX ;
    D3DXVECTOR3* aY ;
    D3DXVECTOR3* aZ ;
    D3DXVECTOR3*  v1;
    D3DXVECTOR3*   v2;
    bool bdrawAffineSelect ;
    D3DXVECTOR3* local_Normal ;
    DWORD dw_transform_type ;
    D3DXMATRIX arb_transform_A;
    D3DXMATRIX mat_center;
    D3DVECTOR v_mid_point;
    float m_Angle;
 
public:
 
    arbitrary_transform() ;
 
    ~arbitrary_transform() { }
 
    D3DXVECTOR3* GetAffineMidPointNormal( LPD3DXMESH, D3DXVECTOR3* );
 
    D3DXMATRIX create_arbitary_axis( D3DXVECTOR3*  );
 
    D3DXMATRIX create_non_affine_axis( D3DXVECTOR3*  );
 
    D3DXMATRIX CreateLocalAxis( D3DXVECTOR3* ,
                                D3DXVECTOR3*,
                                D3DXVECTOR3* );
 
    LPD3DXMESH Mesh_Transform_ArbObject( LPD3DXMESH, 
                                    D3DXMATRIX 
       );
 
    LPD3DXMESH RotatateSubMeshOrthogonal( LPD3DXMESH, D3DXVECTOR3* );
 
    int GetDotRotationDir( LPD3DXMESH );
 
    float GetDotRotationValue( LPD3DXMESH );
 
    float GetDotRotationValueYW( LPD3DXMESH );
 
    DWORD Get_Quad_Orientation( LPD3DXMESH  );
 
    float GetDotRotationValue_YW_2( LPD3DXMESH );
 
 
 
    LPD3DXMESH Mesh_Transform_Arb( LPD3DXMESH, 
                                    D3DXMATRIX );
 
 
    LPD3DXMESH Mesh_Transform_Arb2( LPD3DXMESH, 
                                    D3DXMATRIX );
 
    LPD3DXMESH TransformVerticesAndNormalsArb( LPD3DXMESH ,
                                               D3DXMATRIX  ,         
                                               D3DXMATRIX );
 
    D3DXVECTOR3* Get_Arb_Normal( LPD3DXMESH, WORD*, DWORD );
    D3DVECTOR Get_Arb_Vertex( LPD3DXMESH, WORD*, DWORD );
    D3DXVECTOR3* GetAX( LPD3DXMESH, D3DXVECTOR3* );
    D3DXVECTOR3* GetAY( LPD3DXMESH, D3DXVECTOR3* );
    D3DXVECTOR3* GetAZ( LPD3DXMESH, D3DXVECTOR3* );
    LPD3DXMESH Transform_Coordinate_Plane( LPD3DXMESH,
                                       D3DVECTOR );
 
    D3DXVECTOR3* GetAYCrossAY( D3DXVECTOR3*, D3DXVECTOR3*, D3DXVECTOR3* );
 
   LPD3DXMESH  InsertAffineMidPoint( D3DVECTOR, 
                                     LPD3DXMESH ,
                                     LPDIRECT3DDEVICE9&,
                                     LPD3DXMESH );
 
    D3DVECTOR* GetConstantvectors( WORD*,
                                   LPD3DXMESH,
                                   D3DVECTOR, 
                                   DWORD
                                 );
 
    LPD3DXMESH Set_Arb_Vertex_Color( LPD3DXMESH , WORD* , DWORD  );
    LPD3DXMESH Translate_Arb_Mat( LPD3DXMESH, D3DXMATRIX, D3DVECTOR );
    LPD3DXMESH translate_mesh( LPD3DXMESH , float , float , float  );
 
};
Advertisement

General advice: please use "" tags and (probably comes with that) proper intendation. Otherwise its very unlikely anyone is going to read through all this.

I went ahead and edited your post to add code tags, and cleaned up the formatting a bit.

Some criticisms:

  • Why is everything public?
  • Getters and Setters are often a code-smell or unnecessary java-ism.
  • Why is everything public and you also have many getters and setters?
  • Your naming conventions are all over the place, I see lowercase-underscore, PascalCase, and camelCase all mixed together.
  • Your naming choices for member functions and member variables are unintuitive and unclear.
  • I don't see 'const' used anywhere -- const-correctness is an integral part of maintaining the invariant state a class is supposed to maintain.
  • I don't see you passing parameters by reference anywhere except where it would be dictated by D3D convention -- I could be wrong about this, as I'm not familiar with all of these data structures you refer to, but my experience says that passing by const reference is usually more prevalent than what I can see here.
  • You have several cases of 'foo1(...)' and 'foo2(...)' that have the same parameter and return types. 1 and 2 doesn't tell me anything -- the function name should tell me what's different about them.
  • You mostly don't name your parameters -- while this is allowed and even sometimes useful, its more-often bad form for a public interface to not describe what its parameters are. When the class members are implemented in a non-header file, clients won't see the names you give them there.
  • It looks like some of your member functions shouldn't be members -- these should probably be non-member-non-friend functions. If they're part of the same logical interface, keep them in the same namespace, otherwise put them in another namespace. For example, 'void createSphere( LPDIRECT3DDEVICE9&)'

throw table_exception("(? ???)? ? ???");

Ok, now thats more readable. So here are a few pointers, since I'm not sure if you are only asking technically or also about code-structure, so I'm going to give you both (not ordered by any particular weight or severity):

- Consistensy! Especially in naming. Which you don't have, that is. (was that correct english?) Nevermind. Get a convention and use it straight-through. Some of your class-variables have "m_" as prefix, others don't. Some use CamelCase, others don't. Some use "_" while others don't. CamelCase + "_" IMHO should be banned, but thats not important. Stick to one convention and keep it. Makes your code more readable (for anyone, including you) and less of a quessing game to how variables have to be written. I use m_variableName, but thats entirely up to you. Also applies to function names. And class names. And so on.

- Name parameters in function-declarations. ALL OF THEM, EVER. You seem to be doing it sometimes, but its very important to keep this all the time, since otherwise you yourself are going to wonder about one week later "Awww... whats the 3rd mesh paramter in the render function for again?". Also, consitensy, again.

- Meaningful names. What is "v1" and "v2"? You seem to have that one right, with exceptions.

- Use access control. Everything you have right now is public. You should always try to hide as much behind private as possible. Protected is kind of like public, yet a little less bad (just as drinking urin is not as bad as driking acid). If you have problems with this, start by putting everything that is not called externally right now behind public. If too much is called or accessed (especially variables!) there is probably a flaw in your design, and you likely to have a hard time.

- "LPDIRECT3DDEVICE9&" as a function parameter makes not much sense, since LPDIRECT3DDEVICE9 is a pointer to a IDirect3DDevice9, which means you can just use LPDIRECT3DDEVICE9 and be fine.

This is just standard stuff. Some might tell you should not worry about this right now, but this is not "For beginners", and you asked for it. Here is some advanced stuff that might prove useful, but is probably overkill:

- Your mesh class appears way to big. You might consider splitting it up a bit. For example, put all bounding-box related stuff into a seperate BoundingBox-class and have the mesh hold an instance of it. Stuff like this.

- Const-correctnes. Don't know if you've seen or heard of it yet, if not, google for it (or ask here again if you have troubles understanding anything). If you don know about it, then this advice was start applying it. Wherever possible. To variables that won't change again, to function paramters that require read-only-access, and to class methods that don't modify the classes state (read: variables). This helps you from screwing something up unwillingly (e.g. changing a vector when all you wanted is to calculate the normal), but might be difficult/time consuming to apply depending on how big your codebase is.

Thats it for the interface, I can't really say anything about the implementation, or the usage of these classes without seeing how they are implemented (duh) or used. But I get the feeling you asked for how to improve the classes themselfs, in the way I gave you advice - in any case, don't mistake me for being rude, just some straight-out tips for you as a ground for improvement.

EDIT:

Ninja'd :/ Should finally learn to write more direct and less all-over-the-place^^

Thanks Juliean there are some very useful comments there I shall add the tidy up to my to do list

Thank you !

.

Some criticisms:

  • Why is everything public?
  • Getters and Setters are often a code-smell or unnecessary java-ism.
  • Why is everything public and you also have many getters and setters?
  • Your naming conventions are all over the place, I see lowercase-underscore, PascalCase, and camelCase all mixed together.
  • Your naming choices for member functions and member variables are unintuitive and unclear.
  • I don't see 'const' used anywhere -- const-correctness is an integral part of maintaining the invariant state a class is supposed to maintain.
  • I don't see you passing parameters by reference anywhere except where it would be dictated by D3D convention -- I could be wrong about this, as I'm not familiar with all of these data structures you refer to, but my experience says that passing by const reference is usually more prevalent than what I can see here.
  • You have several cases of 'foo1(...)' and 'foo2(...)' that have the same parameter and return types. 1 and 2 doesn't tell me anything -- the function name should tell me what's different about them.
  • You mostly don't name your parameters -- while this is allowed and even sometimes useful, its more-often bad form for a public interface to not describe what its parameters are. When the class members are implemented in a non-header file, clients won't see the names you give them there.
  • It looks like some of your member functions shouldn't be members -- these should probably be non-member-non-friend functions. If they're part of the same logical interface, keep them in the same namespace, otherwise put them in another namespace. For example, 'void createSphere( LPDIRECT3DDEVICE9&)'

Interesting comments, I shall attend to them.

Thanks for your input.

This topic is closed to new replies.

Advertisement