Need critical input on this
in [MFC]
|
From: RB on 21 Feb 2010 19:37 I have created the below code that actually works and compiles with zero errors and zero warnings. I would like criticism and input on it from all angles, First off is there a better way to do this. Does my code have problems etc. (granted I have left some error checking out to keep it brief) This all started when I wanted to change the font type and size in my FormView Listbox. I always try to keep everything within MFC since I want to concentrate on my stuff and not windows stuff. But everywhere I looked on the net for changing a font in a resource control had stuff setting the Owner Draw style to yes and writing an owner draw function of DrawItem( LPDRAWITEMSTRUCT lpDrawItemStruct ) which I eventually did learn to create and get working BUT in the process I ran across some things that made me think I did not have to write the owner draw function and I could set the font with supplied MFC control functions. And therein became this code. I have a standard MFC appwizard app with the generated view class having the CFormView as the base class Then I created a Listbox in the resource editor (on top of the FormView) with the following styles Selection = None and OwnerDraw = No (since I no longer need DrawItem) Vertical scroll and No integral height are checked, & no extended styles. Then in my FormView class header file I put the New font declaration of ----------header file----------------------------------- class CTry_2View : public CFormView { protected: // create from serialization only CTry_2View(); DECLARE_DYNCREATE(CTry_2View) public: //{{AFX_DATA(CTry_2View) enum { IDD = IDD_TRY_2_FORM }; CListBox m_FormListBoxObj; //}}AFX_DATA // Attributes public: CTry_2Doc* GetDocument(); //// >>>>>HERE IS FUTURE NEW FONT FOR MY LISTBOX <<<<<<<//////// CFont NewFont; // Construct instance of CFont class for new font. // Operations public: etc etc .... --------------cpp file------------------------------------ Then in the destructor of the cpp file I put the DeleteObject CTry_2View::CTry_2View() : CFormView(CTry_2View::IDD) { //{{AFX_DATA_INIT(CTry_2View) // NOTE: the ClassWizard will add member initialization here //}}AFX_DATA_INIT // TODO: add construction code here } CTry_2View::~CTry_2View() // add destructor any code here { ////>>>>> HERE IS MY CLEAN UP OF FONT <<<<<<<</////// NewFont.DeleteObject(); // this CFont NewFont obj is declared in this // view class's header file. } ----------NOW further down in this same cpp file I put a button handler that changes the listbox font when clicked. The button was also created in the resource editor and the skeleton handler was added through the class wizard. I basically came up with all this code my own by experimenting with what I could find in the help files and seeing what intellisense would offer me on each item so I expect some criticism on my novice level creation. And are there still reasons I should go with the owner draw scenario as opposed to this way? //-------------Begin OnChangeFontClick function------------// void CTry_2View::OnChangeFontClick() { // Create instance of CClientDC class to this ( CWnd* ) CClientDC dc(this); //create CFont Ptr to current font obj of listbox //The m_FormListBoxObj is a ClassWizard created listbox variable //of category Control and type CListBox CFont *pOldListBoxFont = m_FormListBoxObj.GetFont(); // Declare old LOGFONT to save just in case I needed it later LOGFONT OldFontLogStruct; LOGFONT* pOldFontLogStruct = &OldFontLogStruct; //create ptr to it. // fill OldFontLogStuct with current font data pOldListBoxFont->GetLogFont(pOldFontLogStruct); // Declare a New LOGFONT struct variable LOGFONT NewFontLogStruct; LOGFONT* pNewFontLogStruct = &NewFontLogStruct; //create ptr to it. // copy the old stuff to new *pNewFontLogStruct = *pOldFontLogStruct; // CFont NewFont is declared in header file and destructed //(DeleteObject) in cpp file of the generated View class. CFont *pNewFont = &NewFont; // create ptr to it. // set the values we are concerned with in new font NewFontLogStruct.lfPitchAndFamily = (FF_MODERN || FIXED_PITCH); // lfHeight is negative int so convert abs. NewFontLogStruct.lfHeight = ( abs( pOldFontLogStruct->lfHeight ) + 4); // Courier New is a fixed space font strcpy(NewFontLogStruct.lfFaceName, "Courier New"); // Create the new font with updated NewFontLogStruct BOOL FontCreation = NewFont.CreateFontIndirect(&NewFontLogStruct); if (FontCreation == 0) { MessageBox("Font creation failed!", "Error", MB_OK | MB_ICONEXCLAMATION); } else { // Now set the font to the listbox control in my formview class m_FormListBoxObj.SetFont(pNewFont, TRUE); } } // end of function
From: Joseph M. Newcomer on 21 Feb 2010 23:23 On Sun, 21 Feb 2010 19:37:13 -0500, "RB" <NoMail(a)NoSpam> wrote: > I have created the below code that actually works and compiles with >zero errors and zero warnings. I would like criticism and input on >it from all angles, First off is there a better way to do this. Does >my code have problems etc. (granted I have left some error checking >out to keep it brief) > This all started when I wanted to change the font type and size in my >FormView Listbox. I always try to keep everything within MFC since I >want to concentrate on my stuff and not windows stuff. But everywhere >I looked on the net for changing a font in a resource control had stuff >setting the Owner Draw style to yes and writing an owner draw function >of DrawItem( LPDRAWITEMSTRUCT lpDrawItemStruct ) which I eventually >did learn to create and get working BUT in the process I ran across some >things that made me think I did not have to write the owner draw function >and I could set the font with supplied MFC control functions. > And therein became this code. I have a standard MFC appwizard app with >the generated view class having the CFormView as the base class >Then I created a Listbox in the resource editor (on top of the FormView) >with the following styles Selection = None and >OwnerDraw = No (since I no longer need DrawItem) >Vertical scroll and No integral height are checked, & no extended styles. > Then in my FormView class header file I put the New font declaration of >----------header file----------------------------------- >class CTry_2View : public CFormView >{ > protected: // create from serialization only > CTry_2View(); > DECLARE_DYNCREATE(CTry_2View) >public: > //{{AFX_DATA(CTry_2View) > enum { IDD = IDD_TRY_2_FORM }; > CListBox m_FormListBoxObj; > //}}AFX_DATA > >// Attributes >public: > CTry_2Doc* GetDocument(); >//// >>>>>HERE IS FUTURE NEW FONT FOR MY LISTBOX <<<<<<<//////// > CFont NewFont; // Construct instance of CFont class for new font. **** Nothing wrong with this except that it makes no sense to make it public. Never expose something as public that does not need to be. Due to a fundamental design error, all message handlers are declared public, and you can't change it, so you have to hand-edit them to protected, unless you are using VS6, where this will break ClassWizard. **** > >// Operations >public: >etc etc .... >--------------cpp file------------------------------------ >Then in the destructor of the cpp file I put the DeleteObject >CTry_2View::CTry_2View() > : CFormView(CTry_2View::IDD) >{ > //{{AFX_DATA_INIT(CTry_2View) > // NOTE: the ClassWizard will add member initialization here > //}}AFX_DATA_INIT > // TODO: add construction code here >} > >CTry_2View::~CTry_2View() // add destructor any code here >{ ////>>>>> HERE IS MY CLEAN UP OF FONT <<<<<<<</////// > NewFont.DeleteObject(); // this CFont NewFont obj is declared in this **** Not needed. It will be deleted automatically when the variable is deleted as part of the deletion of the dialog box object. **** > // view class's header file. >} >----------NOW further down in this same cpp file I put a button handler >that changes the listbox font when clicked. The button was also created >in the resource editor and the skeleton handler was added through the >class wizard. I basically came up with all this code my own by experimenting >with what I could find in the help files and seeing what intellisense would >offer me on each item so I expect some criticism on my novice level creation. >And are there still reasons I should go with the owner draw scenario as >opposed to this way? > >//-------------Begin OnChangeFontClick function------------// >void CTry_2View::OnChangeFontClick() >{ > // Create instance of CClientDC class to this ( CWnd* ) > CClientDC dc(this); **** There seems to be no reason to create the DC because nothing in the rest of the code USES the dc. In fact, the compiler should complain about this. Hint: You should always build at /W4, which is not the default. **** > > //create CFont Ptr to current font obj of listbox > //The m_FormListBoxObj is a ClassWizard created listbox variable > //of category Control and type CListBox > CFont *pOldListBoxFont = m_FormListBoxObj.GetFont(); > > // Declare old LOGFONT to save just in case I needed it later > LOGFONT OldFontLogStruct; > LOGFONT* pOldFontLogStruct = &OldFontLogStruct; //create ptr to it. **** THis is insane. Why do you need a second variable to hold the pointer to the first? There is no conceivable reason to create the pointer variable. Delete it and fix all references to it. **** > > // fill OldFontLogStuct with current font data > pOldListBoxFont->GetLogFont(pOldFontLogStruct); **** So why not write pOldListBoxFont->GetLogFont(&OldFontLogStruct); ? Use the "&" operator! I find it a common error that peoplel think that because the specification of a parameter is LOGFONT * (for example), they think there has to be a VARIABLE of that type. This makes no sense at all. Never did. The only requirement is that the TYPE of the expression must be LOGFONT*. So if you have a LOGFONT variable lf, then &lf evaluates to a LOGFONT*, which satisfies the requirement. In fact, you should NEVER, EVER create gratuitous variables like this. **** > > // Declare a New LOGFONT struct variable > LOGFONT NewFontLogStruct; > LOGFONT* pNewFontLogStruct = &NewFontLogStruct; //create ptr to it. **** Same error! No need to have a pointer variable, ever! **** > > // copy the old stuff to new > *pNewFontLogStruct = *pOldFontLogStruct; **** Silliler. Why not write NewFontLogStruct = OldFontLogStruct; Since the pointer variables do not need to exist, the use of them makes no sense. **** > > // CFont NewFont is declared in header file and destructed > //(DeleteObject) in cpp file of the generated View class. **** No. It is automatically deleted when the object that contains it is deleted. **** > CFont *pNewFont = &NewFont; // create ptr to it. **** There is no conceivable reason that pNewFont should exist, so delete it entirely. This represents really horrible practice. The introduction of these unnecessary variables is extremely bad style. It adds names and confusion, and makes the programs harder to read, reason about, and maintain. **** > > // set the values we are concerned with in new font > NewFontLogStruct.lfPitchAndFamily = (FF_MODERN || FIXED_PITCH); **** Absolutely, positively, incorrect. || is LOGICAL OR, which means the resulting value is either "0" or "1". The CORRECT operator is |, so you would write FF_MODERN | FIXED_PITCH Never confuse the bitwise operators with the logical operators. So, for example int n = 4 || 2; then the value of n is 1. int n = 4 | 2; then the value of n is 6. ***** > > // lfHeight is negative int so convert abs. > NewFontLogStruct.lfHeight = ( abs( pOldFontLogStruct->lfHeight ) + 4); **** Why are you using a positive value when a negative value should be used? That may even be why you have to add +4. **** > > // Courier New is a fixed space font > strcpy(NewFontLogStruct.lfFaceName, "Courier New"); **** Several errors: (1) Never use strcpy. use only the safe forms Note that if you are not using VS2008 or greater, use strsafe.h (2) Never use 8-bit characters or assume they exist, except in exceedingly rare and exotic circumstances, of which this is most definitely not an example. The correct code would be _tcscpy_s(NewFontLogStruct.lfFaceName, _countof(NewFontLogStruct.lfFaceName), _T("Courier New")); If you are using the strsafe.h library (in older versions of VS) StrCchCopy(NewFontLogStruct.lfFaceName, sizeof(NewFontLogStruct.lfFaceName)/sizeof(TCHAR), _T("Courier New")); ALWAYS program Unicode-aware. strcpy, strcat, and sprintf are dead, dead, DEAD functions and should NEVER be used in real programs. Use either the _s forms or the strsafe.h library. These functions represent abysmally poor programming style that was accepted, erroneously, by the community for many years. Today, any sane company not only forbids their use in real programs, but in some companies using them is a firable offense. We call them "malware paths" and their main value is in allowing carefully structured attacks on your program. Their second purpose is to allow meaningless and hard-to-diagnose crashes. **** > > // Create the new font with updated NewFontLogStruct > BOOL FontCreation = NewFont.CreateFontIndirect(&NewFontLogStruct); > > if (FontCreation == 0) **** Since it is BOOL type, you would write if(!FontCreation) Do not compare boolean results to literal values, especially not to integers. **** > { > MessageBox("Font creation failed!", "Error", MB_OK | MB_ICONEXCLAMATION); > } > else > { // Now set the font to the listbox control in my formview class > m_FormListBoxObj.SetFont(pNewFont, TRUE); **** This should work. You did not report any problem, so it should be correct. However, the correct form would have been m_FormListBoxObj.SetFont(&NewFont); There is no need to provide the second parameter because it defaults to TRUE. And the pointer variable is completely gratuitous and should not exist. Note that it was essental that your variable to be declared, as you show it, in the class. Had it been a local variable, the font would be destroyed when this function exited and therefore this code would have no effect. joe **** > } >} // end of function > Joseph M. Newcomer [MVP] email: newcomer(a)flounder.com Web: http://www.flounder.com MVP Tips: http://www.flounder.com/mvp_tips.htm
From: Giovanni Dicanio on 22 Feb 2010 04:45 "Joseph M. Newcomer" <newcomer(a)flounder.com> ha scritto nel messaggio news:6004o5lmagv4g2buq5h11r0aifshpv6pfn(a)4ax.com... >> // Courier New is a fixed space font >> strcpy(NewFontLogStruct.lfFaceName, "Courier New"); > **** > Several errors: > (1) Never use strcpy. use only the safe forms > Note that if you are not using VS2008 or greater, use strsafe.h I believe the safe forms already existed in VS2005, a.k.a. VC8 (if my memory serves me well, the "Secure CRT" was introduced in VC8). Note also that, if you program in C++ (not pure C), thanks to proper template overloads, the buffer length is automatically inferred, so there is no need to specify the size argument. In other words, in case of static string buffer (like LOGFONT::lfFaceName above) and C++ source code, thanks to "template magic", strcpy would be equivalent to strcpy_s with proper size argument. (Of course, the same is true for the Unicode version wcscpy[_s].) Giovanni
From: RB on 22 Feb 2010 11:13 Hello Joe, first off I want to thank you for your time and effort in this critic of my code. It is not only exactly what I was hoping for but more. I have tried to be brief in my responses (most of which are only acknowledgments) but I did take the time to respond in enough text to do your diligent effort justice. ================================================ >> CFont NewFont; // Construct instance of CFont class for new font. **** >Nothing wrong with this except that it makes no sense to make it public. >Never expose something as public that does not need to be. Thanks, good point. That is something I am still learning the needs of. At some point I need to establish priorties in these areas. When I do create my own class I generally put all my stuff in Protected. Here admittely I just really did not apply much thought to it. ------------------------------------------------------------- >Due to a fundamental design error, all message handlers are declared >public, and you can't change it, so you have to hand-edit them to >protected, unless you are using VS6, where this will break ClassWizard. **** I am using VC Pro ver. 6.x sp5 so then I should not hand edit them correct? -------------------------------------------------------------- >>{ ////>>>>> HERE IS MY CLEAN UP OF FONT <<<<<<<</////// >> NewFont.DeleteObject(); // this CFont NewFont obj is declared in this >Not needed. It will be deleted automatically when the variable is >deleted as part of the deletion of the dialog box object. **** Ah the voice of experience and depth is speaking to me. I would ask at this point if I had created the font a main view window (not a resource control) would I need to DeleteObject there, or rather when would I need to DeleteObject? ----------------------------------------------------------------- >> CClientDC dc(this); **** >There seems to be no reason to create the DC because nothing in the >rest of the code USES the dc. In fact, the compiler should complain >about this. Good catch !, when my code first started experimenting it was needed but as it evolved it's need disappeared and I missed that. >Hint: You should always build at /W4, which is not the default. I will try that, I need all the warnings I can get ------------------------------------------------------- >> LOGFONT OldFontLogStruct; >> LOGFONT* pOldFontLogStruct = &OldFontLogStruct; //create ptr **** > THis is insane. Why do you need a second variable to hold the > pointer to the first? There is no conceivable reason to create the > pointer variable. Delete it and fix all references to it. Well I'm glad you called this. I first created it to satisfy the the pointer parameter of a following function, but in retrospect your are absolutely right. I could have used the & in the arg. --------------------------------------------------- >> pOldListBoxFont->GetLogFont(pOldFontLogStruct); **** >So why not write > pOldListBoxFont->GetLogFont(&OldFontLogStruct); > ? > Use the "&" operator! I find it a common error that people > think that because the specification of a parameter is LOGFONT * > (for example), they think there has to be a VARIABLE of that type. > This makes no sense at all. Never did. The only requirement is > that the TYPE of the expression must be LOGFONT*. So if you have > a LOGFONT variable lf, then &lf evaluates to a LOGFONT*, which > satisfies the requirement. In fact, you should NEVER, EVER create > gratuitous variables like this. Again point taken. Excellent analogy in "gratuitous variables" and I am very grateful for the professional insight here, though I fear this may be a difficult one for me to always remember since my ability enhancement in this level of coding expertise, sometimes takes a lower priority to my code design issues. (very poor excuse but just expressing) Someday when I retire from Engineering Cad work I would like to program more and hone down the non professional attributes I have now. --------------------------------------------- >> // copy the old stuff to new >> *pNewFontLogStruct = *pOldFontLogStruct; **** > Silliler. Why not write > NewFontLogStruct = OldFontLogStruct; Ah I am really learing stuff now. Honestly I did not think the compiler would do this. I thought I would have to initialize with a parenthesis series of the data. Again exposing my ignorance but I am learning new things. My biggest problem is that I don't write code all the time and haven't learned or experimented enough in these areas (being self taught in C++) But as much as I enjoy it I MUST allocate more regular time to it or else forever be a waste of time to generous repliers like yourself. -------------------------- >> // set the values we are concerned with in new font >> NewFontLogStruct.lfPitchAndFamily = (FF_MODERN || FIXED_PITCH); **** > Absolutely, positively, incorrect. || is LOGICAL OR, which means > the resulting value is either "0" or "1". The CORRECT operator is |, > so you would write FF_MODERN | FIXED_PITCH > Never confuse the bitwise operators with the logical operators. ***** Thank you for catching my sloppiness. I knew better here, I have no excuse, I simply rushed by it without being aware enough. I have saved your excellent bit result example with my font reference code. ------------------------------------------------- >> // lfHeight is negative int so convert abs. >> NewFontLogStruct.lfHeight = ( abs( pOldFontLogStruct->lfHeight ) + 4); **** > Why are you using a positive value when a negative value should be used? > That may even be why you have to add +4. **** I put the abs in since my code was not giving me a desired 4 size larger font, and looking at the debugger values it showed the lfHeight being a (-12) and I wanted a 16 font. So then you are saying that I should have just did this ( pOldFontLogStruct->lfHeight ) -4 ); which would be more efficient, except I did not fully understand why the lfHeight was showing a neg value and feared it might be a bit representation thing instead of an actual neg value. ------------------------------------------- >> strcpy(NewFontLogStruct.lfFaceName, "Courier New"); **** >Several errors: >(1) Never use strcpy. use only the safe forms >Note that if you are not using VS2008 or greater, use strsafe.h I will have to do this, since I was unaware of it. I am using VC Pro ver.6.x sp5 >(2) Never use 8-bit characters or assume they exist, except in > exceedingly rare and exotic circumstances, of which this is most > definitely not an example. The correct code would be > _tcscpy_s(NewFontLogStruct.lfFaceName, _countof(NewFontLogStruct.lfFaceName), > _T("Courier New")); Hmmmm... I think I see what you mean and will remember this. I had always thought that since I was not using unicode characters that I did not need to worry. But if I understand you correctly you are saying that my code would load a lfFaceName on a system that is using type _T or TCHAR in "size" and I DO need to worry about size I am manipulating or copying. >If you are using the strsafe.h library (in older versions of VS) >StrCchCopy(NewFontLogStruct.lfFaceName, >sizeof(NewFontLogStruct.lfFaceName)/sizeof(TCHAR), _T("Courier New")); >ALWAYS program Unicode-aware. >strcpy, strcat, and sprintf are dead, dead, DEAD functions and should NEVER be >used in real programs. Use either the _s forms or the strsafe.h library. >These functions represent abysmally poor programming style that was accepted, >erroneously, by the community for many years. Today, any sane company not >only forbids their use in real programs, but in some companies using them is >a firable offense. We call them "malware paths" and their main value is in >allowing carefully structured attacks on your program. >Their second purpose is to allow meaningless and hard-to-diagnose crashes. **** Again I will store this entire reply in my reference folder and I will update my code to reflect this and you have no idea how much I appreciate your generous and diligent effort to correct my errors. --------------------------------------------- >> if (FontCreation == 0) **** > Since it is BOOL type, you would write > if(!FontCreation) > Do not compare boolean results to literal values, especially not to > integers. **** ugh well I will study this since I am still lacking in my conception of BOOL's use. I.e. I see some funcs that return a zero value for a non-error scenario and some return a non-zero for non-error scenario but some of these have a BOOL return type so it left me feeling somewhat unsure of the outcome if I did not specify the actual integer I wanted. ----End------------------------
From: Joseph M. Newcomer on 22 Feb 2010 17:54
See below... On Mon, 22 Feb 2010 11:13:08 -0500, "RB" <NoMail(a)NoSpam> wrote: >Hello Joe, first off I want to thank you for your time and effort in >this critic of my code. It is not only exactly what I was hoping for >but more. I have tried to be brief in my responses (most of which >are only acknowledgments) but I did take the time to respond in >enough text to do your diligent effort justice. >================================================ >>> CFont NewFont; // Construct instance of CFont class for new font. >**** >>Nothing wrong with this except that it makes no sense to make it public. >>Never expose something as public that does not need to be. > >Thanks, good point. That is something I am still learning the needs of. >At some point I need to establish priorties in these areas. When I do >create my own class I generally put all my stuff in Protected. >Here admittely I just really did not apply much thought to it. >------------------------------------------------------------- > >>Due to a fundamental design error, all message handlers are declared >>public, and you can't change it, so you have to hand-edit them to >>protected, unless you are using VS6, where this will break ClassWizard. >**** >I am using VC Pro ver. 6.x sp5 so then I should not hand edit them >correct? **** That is a really, really antique version of the compiler; you should seriously look into upgrading. The compiler has problems with templates, and, for example, won't compile the best Standard C++ Library (and the code that is in the DLL has known bugs). The MFC is antique, also, and lacks the power and flexibility of modern MFC. But yes, you can't edit the handlers or the ClassWizard breaks. It was pretty bad in terms of what it could accept. **** > >-------------------------------------------------------------- >>>{ ////>>>>> HERE IS MY CLEAN UP OF FONT <<<<<<<</////// >>> NewFont.DeleteObject(); // this CFont NewFont obj is declared in this > >>Not needed. It will be deleted automatically when the variable is >>deleted as part of the deletion of the dialog box object. >**** >Ah the voice of experience and depth is speaking to me. I would ask at >this point if I had created the font a main view window (not a resource >control) would I need to DeleteObject there, or rather when would >I need to DeleteObject? **** Essentially, never. Unless you had to re-create the font for some other reason, such as the user selecting a new choice. But otherwise, the automatic deletion will handle it. Destructors of an outer structure invoke destructors on the members before the structure/class itself is destroyed. >----------------------------------------------------------------- > >>> CClientDC dc(this); >**** >>There seems to be no reason to create the DC because nothing in the >>rest of the code USES the dc. In fact, the compiler should complain >>about this. > >Good catch !, when my code first started experimenting it was needed >but as it evolved it's need disappeared and I missed that. > >>Hint: You should always build at /W4, which is not the default. > >I will try that, I need all the warnings I can get >------------------------------------------------------- > >>> LOGFONT OldFontLogStruct; >>> LOGFONT* pOldFontLogStruct = &OldFontLogStruct; //create ptr >**** >> THis is insane. Why do you need a second variable to hold the >> pointer to the first? There is no conceivable reason to create the >> pointer variable. Delete it and fix all references to it. > >Well I'm glad you called this. I first created it to satisfy the >the pointer parameter of a following function, but in retrospect >your are absolutely right. I could have used the & in the arg. >--------------------------------------------------- > >>> pOldListBoxFont->GetLogFont(pOldFontLogStruct); >**** >>So why not write >> pOldListBoxFont->GetLogFont(&OldFontLogStruct); >> ? >> Use the "&" operator! I find it a common error that people >> think that because the specification of a parameter is LOGFONT * >> (for example), they think there has to be a VARIABLE of that type. >> This makes no sense at all. Never did. The only requirement is >> that the TYPE of the expression must be LOGFONT*. So if you have >> a LOGFONT variable lf, then &lf evaluates to a LOGFONT*, which >> satisfies the requirement. In fact, you should NEVER, EVER create >> gratuitous variables like this. > >Again point taken. Excellent analogy in "gratuitous variables" and >I am very grateful for the professional insight here, though I fear >this may be a difficult one for me to always remember since my >ability enhancement in this level of coding expertise, sometimes takes >a lower priority to my code design issues. (very poor excuse but just >expressing) Someday when I retire from Engineering Cad work I >would like to program more and hone down the non professional >attributes I have now. **** Remember the & operator. **** >--------------------------------------------- >>> // copy the old stuff to new >>> *pNewFontLogStruct = *pOldFontLogStruct; >**** >> Silliler. Why not write >> NewFontLogStruct = OldFontLogStruct; > >Ah I am really learing stuff now. Honestly I did not think >the compiler would do this. I thought I would have to initialize >with a parenthesis series of the data. Again exposing my >ignorance but I am learning new things. My biggest problem >is that I don't write code all the time and haven't learned or >experimented enough in these areas (being self taught in C++) > But as much as I enjoy it I MUST allocate more regular time to >it or else forever be a waste of time to generous repliers like >yourself. **** But, as it turns out, if you write *(&A) = *(&B) that is EXACTLY like writing A = B; The fact that you had done the &A by creating an intermediate variable doesn't change the logic. Note also that in the case of classes, you have to have an assignment operator defined for all user-defined types contained in the class. So if I have class X { }; class Y { X thing; X other; } then if I write Y a; Y b; I cannot write b = a unless there is an assignment operator defined for X. Note that for simple types it is spplied by the compiler, and by recursion, if X has only simple types you get a compiler-designed assignment operator. But if it is some complex type, you may have a failure. For example class X { CArray<int> Data; } will cause a compilation error if you write b = a; as above, because CArray has no defined assignment operator. But if you define an assignment operator in X that does the array copy, then your assignment operator will be used for the assignment to accomplish the copy of the X components of the Y values. **** **** >-------------------------- > >>> // set the values we are concerned with in new font >>> NewFontLogStruct.lfPitchAndFamily = (FF_MODERN || FIXED_PITCH); >**** >> Absolutely, positively, incorrect. || is LOGICAL OR, which means >> the resulting value is either "0" or "1". The CORRECT operator is |, >> so you would write FF_MODERN | FIXED_PITCH >> Never confuse the bitwise operators with the logical operators. >***** > >Thank you for catching my sloppiness. I knew better here, I have no >excuse, I simply rushed by it without being aware enough. I have >saved your excellent bit result example with my font reference code. **** No, there's a difference between "sloppy" code, which works but whose style is debatable, and code that is plan wrong. This code is wrong. **** >------------------------------------------------- >>> // lfHeight is negative int so convert abs. >>> NewFontLogStruct.lfHeight = ( abs( pOldFontLogStruct->lfHeight ) + 4); >**** >> Why are you using a positive value when a negative value should be used? >> That may even be why you have to add +4. >**** >I put the abs in since my code was not giving me a desired 4 size larger >font, and looking at the debugger values it showed the lfHeight being a (-12) >and I wanted a 16 font. So then you are saying that I should have just did >this ( pOldFontLogStruct->lfHeight ) -4 ); which would be more efficient, >except I did not fully understand why the lfHeight was showing a neg value >and feared it might be a bit representation thing instead of an actual >neg value. ***** -abs(....) gives you the negative value. **** >------------------------------------------- > >>> strcpy(NewFontLogStruct.lfFaceName, "Courier New"); >**** >>Several errors: >>(1) Never use strcpy. use only the safe forms >>Note that if you are not using VS2008 or greater, use strsafe.h > >I will have to do this, since I was unaware of it. I am >using VC Pro ver.6.x sp5 **** strsafe.h was written specifically so VS6 programmers had safe string operators. I used it in VS6 for years. **** > >>(2) Never use 8-bit characters or assume they exist, except in >> exceedingly rare and exotic circumstances, of which this is most >> definitely not an example. The correct code would be > >> _tcscpy_s(NewFontLogStruct.lfFaceName, _countof(NewFontLogStruct.lfFaceName), >> _T("Courier New")); > >Hmmmm... I think I see what you mean and will remember this. I had always thought >that since I was not using unicode characters that I did not need to worry. But >if I understand you correctly you are saying that my code would load a lfFaceName >on a system that is using type _T or TCHAR in "size" and I DO need to worry about >size I am manipulating or copying. **** What do you want to tell your manager when he/she comes in and says "We just landed a big contract in Korea? How soon can you convert the program?" (a) Give me a few weeks and I'll let you know (b) How soon can you get a Korean translator in here? It happens. So develop good programming style early! **** > >>If you are using the strsafe.h library (in older versions of VS) >>StrCchCopy(NewFontLogStruct.lfFaceName, >>sizeof(NewFontLogStruct.lfFaceName)/sizeof(TCHAR), _T("Courier New")); >>ALWAYS program Unicode-aware. > >>strcpy, strcat, and sprintf are dead, dead, DEAD functions and should NEVER be >>used in real programs. Use either the _s forms or the strsafe.h library. >>These functions represent abysmally poor programming style that was accepted, >>erroneously, by the community for many years. Today, any sane company not >>only forbids their use in real programs, but in some companies using them is >>a firable offense. We call them "malware paths" and their main value is in >>allowing carefully structured attacks on your program. >>Their second purpose is to allow meaningless and hard-to-diagnose crashes. >**** > Again I will store this entire reply in my reference folder and I >will update my code to reflect this and you have no idea how much I >appreciate your generous and diligent effort to correct my errors. **** All C books use these. We learned, the hard way, that programs that use these cause headlines like "Flaw in software product makes 3,000,000 machines vulnerable to the XXXXXXX virus!" **** > >--------------------------------------------- >>> if (FontCreation == 0) >**** >> Since it is BOOL type, you would write >> if(!FontCreation) >> Do not compare boolean results to literal values, especially not to >> integers. >**** >ugh well I will study this since I am still lacking in my conception of >BOOL's use. I.e. I see some funcs that return a zero value for a >non-error scenario and some return a non-zero for non-error scenario >but some of these have a BOOL return type so it left me feeling somewhat >unsure of the outcome if I did not specify the actual integer I wanted. ***** If the data type is BOOL, it should be tested using boolean operations. joe **** > >----End------------------------ > Joseph M. Newcomer [MVP] email: newcomer(a)flounder.com Web: http://www.flounder.com MVP Tips: http://www.flounder.com/mvp_tips.htm |