What is a real world example of bad code or programming practice

Question

I just want to know some bad programming practice or code I should avoid to make sure it does not exist in my code. I use c# asp.net but the example you give can be in any language since the idea is still the same. I have follow many suggestions on SO that really cleaned my code up. Now I looking to launch my application and want to make sure its ready for prime time.

Answer 1

God Object

This is from a real project I wrote that is responsible for filtering/collating/printing data from the filesystem. What follows is merely a signature for that piece of junk. (This class is doing entirely too much... )

#include <string>
#include <vector>
#include <boost/shared_ptr.hpp>
#include <boost/scoped_array.hpp>
#define CRYPTOPP_ENABLE_NAMESPACE_WEAK 1
#include <cryptopp/md5.h>
#include <cryptopp/sha.h>
#include "utility.h"

class OldFileData
{
    //Defines for the individual bits in the bitset containing properties for this filedata object
    enum
    {
        ARCHIVE =               0x00000001,  // Standard Win32 Attributes
        COMPRESSED =            0x00000002,
        DIRECTORY =             0x00000004,
        FILE =                  0x00000008,
        HIDDEN =                0x00000010,
        READONLY =              0x00000020,
        SYSTEM =                0x00000040,
        VOLLABEL =              0x00000080,
        WRITABLE =              0x00000100,
        REPARSE =               0x00000200,
        TEMPORARY =             0x00000400,
        WIN32ENUMD =            0x00000800,
        // END Standard Win32 Attributes
        // Signature Attributes
        SIGPRESENT =            0x00001000,
        SIGVALID =              0x00002000,
        // Executable Attribtutes
        DLL =                   0x00004000,
        DEBUG =                 0x00008000,
        ISPE =                  0x00010000,
        ISNE =                  0x00020000,
        ISLE =                  0x00040000,
        ISMZ =                  0x00080000,
        //Internal state attributes
        //These are used to check if the more time consuming attributes need to be enumerated.
        PEENUMERATED =          0x00100000,
        PECHKSUM =              0x00200000,
        SIGENUMERATED =         0x00400000,
        CRYPTSVCERROR =         0x00800000,
        VERSIONINFOCHECKED =    0x01000000
    };

    mutable DWORD bits; //Container for the bits in the enum above
                                      //This is mutable because constant functions
                                      //use the bitset to cache their responses

    //Filename
    std::wstring fileName;

    //PE Information
    mutable FILETIME headerTime;
    mutable DWORD headerSum;
    mutable DWORD calcSum;

    //Version information block
    mutable std::vector<BYTE> versionInformationBlock;
    struct LANGANDCODEPAGE {
        WORD wLanguage;
        WORD wCodePage;
        bool operator==(LANGANDCODEPAGE &rhs)
        {
            return wLanguage == rhs.wLanguage && wCodePage == rhs.wCodePage;
        };
        bool operator<(LANGANDCODEPAGE &rhs)
        {
            if (wLanguage != rhs.wLanguage)
                return wLanguage < rhs.wLanguage;
            return wCodePage < rhs.wCodePage;
        };
    };
    mutable std::vector<LANGANDCODEPAGE> versionTranslations;

    //Hex managment 
    boost::shared_ptr<std::vector<std::pair<unsigned int, bool> > > hexStorage;

    //Enumeration functions
    //When the results aren't cached in the bitset bits, these functions calculate
    //the correct values and place them into the bitset.
    void initPortableExecutable() const;
    void sigVerify() const;
    void enumVersionInformationBlock() const;

    //Group set functions
    //These functions set a large number of items according to an external data structure
    void setAttributesAccordingToDWORD(DWORD win32Attribs) const;

    //Internal calculation functions
    void inline appendAttributeCharacter(std::basic_string<TCHAR> &result, const TCHAR attributeCharacter, const size_t curBit) const;
    std::wstring getVersionInformationString(const std::wstring&) const;
    template <typename hashType> std::wstring getHash() const;

    //PE Checksum functions (from Code Project)
    WORD ChkSum(WORD oldChk, USHORT * ptr, DWORD len) const;
    DWORD GetPEChkSum(LPCTSTR filename) const;

    //SFC Safe Mode Fix functions
    static std::vector<std::wstring> sfcFileStrings;
    enum {
        NOT_CHECKED,
        NO_SFCFILES_DLL,
        ENUMERATED
    } SFCStates;
    static unsigned int sfcState;
    void buildSfcList() const;

    inline void setupWin32Attributes() const;
public:
    //Returns the Win32 handle for the file
    HANDLE getFileHandle(bool readOnly = true) const;
    //Construct a fileData record using a Win32FindData structure and a search path.
    OldFileData(const WIN32_FIND_DATA &rawData, const std::basic_string<TCHAR>& root);
    //Construct a fileData record using a raw filename
    OldFileData(const std::wstring &fileNameBuild);

    //Type extensions
    //Used for comparisons and for getting implicit conversions
    bool operator<(OldFileData& rhs);
    operator std::wstring () const {return getFileName(); };

    //Accessor methods
    //Return information about the current filedata object

    //Size
    inline unsigned __int64 getSize() const;

    //Filename
    inline const std::basic_string<TCHAR> & getFileName() const;

    //Access times
    inline const FILETIME getLastAccessTime() const;
    inline const FILETIME getLastModTime() const;
    inline const FILETIME getCreationTime() const;
    inline const FILETIME getPEHeaderTime() const;

    //Attributes
    //VFIND style attribute string
    std::wstring getAttributesString() const;
    //PE Attribute string
    std::wstring getPEAttribsString() const;
    //Basic WIN32 Attributes
    inline bool isArchive() const;
    inline bool isCompressed() const;
    inline bool isDirectory() const;
    inline bool isFile() const;
    inline bool isHidden() const;
    inline bool isReadOnly() const;
    inline bool isSystem() const;
    inline bool isVolumeLabel() const;
    inline bool isWritable() const;
    inline bool isTemporary() const;
    inline bool isReparsePoint() const;
    //PE Data Attributes
    inline bool isPE() const;
    inline bool isNE() const;
    inline bool isLE() const;
    inline bool isMZ() const;
    inline bool isStrongExecutable() const;
    inline bool hasAuthenticodeSignature() const;
    inline bool peHeaderChecksumIsValid() const;
    inline bool isDLL() const;
    inline DWORD getPEHeaderCheckSum() const;
    inline DWORD getPECalculatedCheckSum() const;
    void resetPEHeaderCheckSum();
    inline bool peHeaderTimeIsValid() const;
    //Digital Signature Attributes
    inline bool hasValidDigitalSignature() const;
    //Windows File Protection Attributes
    bool isSfcProtected() const;

    //Hashing functions
    std::wstring MD5() const;
    std::wstring SHA1() const;
    std::wstring SHA224() const;
    std::wstring SHA256() const;
    std::wstring SHA384() const;
    std::wstring SHA512() const;

    // Version information functions
    inline std::wstring GetVerCompany() const;
    inline std::wstring GetVerDescription() const;
    inline std::wstring GetVerVersion() const;
    inline std::wstring GetVerProductName() const;
    inline std::wstring GetVerCopyright() const;
    inline std::wstring GetVerOriginalFileName() const;
    inline std::wstring GetVerTrademark() const;
    inline std::wstring GetVerInternalName() const;
    inline std::wstring GetVerComments() const;
    inline std::wstring GetVerPrivateBuild() const;
    inline std::wstring GetVerSpecialBuild() const;

    // Logging function
    void write();
};

Answer 2

Stale Comments:

//this comment says the code does XYZ
void someFunction()
{
   //actually does 123...
}

Non Idiomatic Development (e.g. FORTRAN C++)

// badly named functions and variables
void a()
{
 int i;
 int j;
 int k;
}

Answer 3

public bool IsListconntSmalleThaOne(IList<IContact> listOfResults) {
  if (listOfResults.Count >= 1)
  {
    return false;
  }
  else
  {
    return true;
  }
}

http://thedailywtf.com/Articles/IsListconntSmalleThaOne.aspx

Some of the things wrong with this:

• Inconsistent naming convention
• Inconsistent indent style
• Needlessly long name
• Completely pointless function
• Is actually inefficient

Answer 4

Gas Factory

Same project as before. This started out nice, but as bugfix after bugfix was piled onto it, it's become a nasty mess.

void recursiveScanner::scan()
{
    HANDLE hFind;
    WIN32_FIND_DATA findData;
    bool fastEcho = (!globalOptions::sortMethod[0]) && globalOptions::zipFileName.empty(); //cache whether we're able to output quickly or not

    //Create a list to hold our results
    std::list<OldFileData> results;

    //This list is a queue of remaining folders to scan. Initialized with the common root of the regexes
    std::list<std::wstring> foldersToScan;
    if (globalOptions::noSubDirectories)
    {
        for (std::vector<boost::shared_ptr<regexClass> >::iterator it = globalOptions::regularExpressions.begin(); it != globalOptions::regularExpressions.end(); it++)
        {
            std::wstring curRegexRoot((*it)->getPathRoot());
            if ((*it)->getPathRoot().empty())
                continue;
            curRegexRoot.append(L"*");
            foldersToScan.push_back(curRegexRoot);
        }
        foldersToScan.sort();
        foldersToScan.erase(std::unique(foldersToScan.begin(), foldersToScan.end()),foldersToScan.end());
        if (foldersToScan.empty())
            foldersToScan.push_back(L"*");
    } else
    {
        std::wstring commonRoot = getRegexesCommonRoot(globalOptions::regularExpressions);
        commonRoot.append(1,L'*');
        foldersToScan.push_front(commonRoot);
    }

    do { //Go until the queue is empty
        disable64.disableFS();
        std::wstring& currentSearchDirectory(foldersToScan.front());
        // Start finding the current directory
        hFind = FindFirstFile(currentSearchDirectory.c_str(),&findData);
        // If for some reason this directory does not exist, skip it but throw no error
        if (hFind == INVALID_HANDLE_VALUE)
        {
            disable64.enableFS();
            foldersToScan.pop_front();
            continue;
        }
        //Remove the \* suffix used for the find functions
        currentSearchDirectory.erase(currentSearchDirectory.length()-1);
        if (globalOptions::showall)
            globalOptions::showall = false; //. and .. should not be shown for recursed subdirectories
        else
        {
            if (findData.cFileName[0] == L'.' && findData.cFileName[1] == NULL)
                if (!FindNextFile(hFind,&findData)) //Skip .
                {
                    disable64.enableFS();
                    foldersToScan.pop_front();
                    continue;
                };
            if (findData.cFileName[0] == L'.' && findData.cFileName[1] == L'.' && findData.cFileName[2] == NULL)
                if (!FindNextFile(hFind,&findData)) //Skip ..
                {
                    disable64.enableFS();
                    foldersToScan.pop_front();
                    continue;
                };
        }
        std::list<std::wstring>::iterator insPos = foldersToScan.begin();
        insPos++;
        do { //Loop through the current directory
            OldFileData currentFile(findData, currentSearchDirectory);
            //If it's a directory and it passes the tree's directory check,
            //add it to the list of directories to search
            if (!globalOptions::noSubDirectories)
            {
                if (currentFile.isDirectory())
                {   
                    if (globalOptions::logicalTree->directoryCheck(currentFile.getFileName()))
                    {
                        std::wstring newDir(currentFile.getFileName());
                        newDir.append(L"\\*");
                        insPos = foldersToScan.insert(insPos,newDir);
                    }
                }
            }
            //If the tree says this file doesn't match, go ahead and check the next one
            if (!globalOptions::logicalTree->include(currentFile))
                continue;
            //If we're sorting, store the file into the results list for sorting later.
            //Otherwise just print it now
            if (fastEcho)
                currentFile.write();
            else
                results.push_back(currentFile);
        } while (FindNextFile(hFind,&findData)); //While there's anything left
        FindClose(hFind);
        disable64.enableFS();
        foldersToScan.pop_front();
    } while(!foldersToScan.empty()); //Go until the queue is empty
    //If we're sorting, sort and print the results
    if (globalOptions::sortMethod[0])
    {
        results.sort();
        for(std::list<OldFileData>::iterator it = results.begin(); it != results.end(); it++)
        {
            it->write();
        }
    }
    if (!globalOptions::zipFileName.empty()) //If there's a zip file name, do the zip.
        zipIt(globalOptions::zipFileName, results);
    printSummary();
}

Answer 5

Learning about bad practices "in general" is a bad practice.

If you are a writer, you don't learn by reading arbitrary bad books. Same here.

On the other hand, if you have a concrete problem at hand, then yes, you might want to learn different solutions of this problem, both good and bad. But as stated, your question is too broad; a pile of unrelated bad code examples will not be useful.