Like Tree1Likes
  • 1 Post By grumpy

Critique please

This is a discussion on Critique please within the Windows Programming forums, part of the Platform Specific Boards category; Hi all. this is my first post. I think (and you will tend to agree with me later) that I ...

  1. #1
    Registered User
    Join Date
    Jan 2012
    Posts
    1

    Critique please

    Hi all. this is my first post. I think (and you will tend to agree with me later) that I really need to work on my programming technique . I will need to do some coding in python for my MS later this year and want to be in the proper programming frame of mind for that.
    Have a look at my snippet code and please critique me as heavily as possible.

    Background: I have network shares for lecturers to upgrade their documents to. Since some of them are not really interested in computers, this prog loads their shares as mapped drives.
    Two pcs, public pc houses the database and is accessible without providing credentials, and privatepc houses their documents and accessible only with credentials. Mapped drives start at u: and end at y:.

    EDIT: Forgot to add. I am using console command
    Code:
    net use h: \\server\share /user:shareuser
    to map \\server\share to h: using the credentials of hypothetical user "shareuser"
    C++ code:
    Code:
    #include <iostream>
    #include <string>
    #include <fstream>
    #include <conio.h>
    
    using namespace std;
    
    int main()
    {   
        string userName, line="", link="net use ";             //line - to get a line from database file.
                                                            /*link - string adding in c needs oper-
                                                            ation of the form string = string + whatever.
                                                            so system call at ** needs proper string operation */
    
        int i=117;                                          /*mapped drives start at u: going to y: depending on
                                                            number of drives to be mapped. char (117) = u*/
        fstream courses;                                    //for smiple database text file
        
        system ("copy \\\\publicpc\\general\\courses"); //location of database file "courses"
        courses.open ("courses");
        cout << "Please enter your username: ";
        cin >> userName;
        
        if (courses.is_open())
        {
           while (userName!=line&&!courses.eof())
           {
                 getline(courses, line);
           }
           if (userName==line)
           {
                 do 
                 {
                       system((link + (char (i)) + ": \\\\privatepc\\"+line+" /persistent:no /user:"+userName).c_str()); //**
                       i++;
                       getline (courses, line);
                 } while (line!="*" && i<122);            // * signifies end of shares for that user, 122 = char (z)
                 courses.close();
           }
           else cout << "It seems that you are not in the lecturer database. Please contact the administrator" << endl;
        }
        else cout << "Cannot open database. Please inform the administrator!";
        system ("del courses");                          
        while (!kbhit()) {}                              //minimise system calls
        return 0;
    }
    database file
    Code:
    lecturer1
    maths100
    maths200
    *
    lecturer2
    apm100
    apm200
    *
    lecturer3
    num100
    *
    etc.
    Last edited by sooloong; 01-26-2012 at 01:39 AM.

  2. #2
    Registered User
    Join Date
    Jun 2005
    Posts
    5,851
    1) Some of your comments are unnecessary. Try using better naming of variables to make it obvious what each variable is being used for .... so you can reduce the number of comments.

    2) Don't use a magic number like 117 to represent the letter 'u' or 122 to represent the letter 'z'. Also, name the variable i to something useful. For example, "char first_mapped_drive = 'u';". There are also win32 API functions that you can use to find the valid drives on your system (which means you can avoid hard-coding your program around a specific system setup).

    3) Look up win32 API functions as alternatives to the system() call. For example, there is a win32 API function named CopyFile() for copying files, and another named DeleteFile() for deleting files. Using such functions means it is harder to maliciously hack into your program and your program can do error checking (for example, detecting if there was a problem copying the file). It is a bit harder to write code to provide functionality of the "net use" command, but it is possible with a bit of work (again using win32 api functions).

    4) Don't use <conio.h> or kbhit(). They are non-standard.

    5) If errors occur, it is more conventional to report them to std::cerr rather than std::cout.

    6) You haven't done any error checking of things like opening files, etc.

    7) It would probably be better if your program checked user credentials and system security settings, rather than just looking to see if they are in your "database". Basically, that means you assume nobody will misuse your program (accidentally or deliberately). That means, it is possible for anyone to misuse your program (accidentally or deliberately) without your program being able to detect or recover from such things.
    stahta01 likes this.
    Right 98% of the time, and don't care about the other 3%.

  3. #3
    train spotter
    Join Date
    Aug 2001
    Location
    near a computer
    Posts
    3,855
    In addition to grumpy's comments;

    Create functions;
    Break the code down into smaller reuseable pieces.
    ie functions to open, close and read a file
    Then create a function to validate the user, which calls the file functions and returns valid or error (not valid credentials is an error).

    ERROR CHECK!
    Report meaningful messages to the user. Which file failed to open? How can the user fix the issue if they do not know where the program thinks the file should be?

    <<"It seems that you are not in the lecturer database. Please contact the administrator">>
    Not a good error msg.

    Initialise your variables.
    The amount of bugs I have fixed in apps because the coder did not init their variables....

    Use enums, const variables or #DEFINE for values like '117' and '122'

    I don't mind the comments, better than alot of 'professional' code I have delt with (who are often overly confident and so very sparse with the comments).

    I can read the code to see WHAT you are doing, your comments should tell me WHY (or what you intend the code to do).
    "Man alone suffers so excruciatingly in the world that he was compelled to invent laughter."
    Friedrich Nietzsche

    "I spent a lot of my money on booze, birds and fast cars......the rest I squandered."
    George Best

    "If you are going through hell....keep going."
    Winston Churchill

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Another K&R Exercise... Looking for a critique
    By edw211 in forum C Programming
    Replies: 8
    Last Post: 06-08-2011, 09:41 PM
  2. Critique my mini-AES
    By Hughesz93 in forum C Programming
    Replies: 7
    Last Post: 05-04-2011, 03:49 PM
  3. Rationale for C first: please critique
    By LowWaterMark in forum C Programming
    Replies: 6
    Last Post: 08-10-2008, 08:57 AM
  4. C 101 critique, please?
    By adobephile in forum C Programming
    Replies: 13
    Last Post: 01-01-2003, 06:05 PM
  5. critique me please
    By ober in forum A Brief History of Cprogramming.com
    Replies: 13
    Last Post: 10-02-2002, 01:59 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21