3
\$\begingroup\$

I am a recent graduate in computer programming and failed so far to get an internship or a job in the business so I am trying my best to learn from online source. I am trying to implement a dir-like command on Windows, going through a folder and list all files and folder.

What I've learned is to avoid using namespace std; and making the includes in alphabetical orders.

In this code, I am obviously mixing c and c++, good or bad? I am also using Digital Mars as a compiler, I'm trying to avoid installing heavy IDEs and compilers like VS.

All thoughts and critiques are welcome.

#include <iostream>
#include <io.h>
#include <string>
#include <sys/stat.h>
#include <sys/types.h>
#include <windows.h>

bool DirectoryExists( const char* absolutePath ){

if( _access( absolutePath, 0 ) == 0 ){

    struct stat status;
    stat( absolutePath, &status );

    return (status.st_mode & S_IFDIR) != 0;
}
return false;
}

char* replace(const char *s){

char* p = new char[strlen(s)+1];
int i=0;

for (i=0;s[i];i++)  
if (s[i]=='\\') 
    p[i] ='/';
else
    p[i]=s[i];

if (p[i-1] == '/')
    p[i]='\0';
else
{   
    p[i]='/';
    p[i+1]='\0';
}

return p;
}

void dirc (const char* destpath){

HANDLE hFind;
WIN32_FIND_DATA FindFileData;
char filename[256];
size_t i=1;

 if((hFind = FindFirstFile(destpath, &FindFileData)) != INVALID_HANDLE_VALUE)
 {
  do    {
        sprintf (filename, "echo %d-%s", i, FindFileData.cFileName);
        system (filename);
        i++;
        }
while(FindNextFile(hFind, &FindFileData));

FindClose(hFind);
}
}

int main(int argc, char**argv) {

const char* path = argv[1];
char* fspath;

if (argv[1] == NULL)
    {
    std::cout <<"No path provided"<<std::endl;
    return 0;
    }

else
    if ( DirectoryExists(path) )
        std::cout <<"Provided path is "<<path<<std::endl;
    else
        {
        std::cout <<"Path doesn't exist"<<std::endl;
        return 0;
        }
 fspath = replace(path);
 char* destpath = (char *) malloc (strlen(fspath)+6);
 destpath = strcat (fspath,"*.*");
 dirc (destpath);
 free (destpath);

return 0;
}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Apr 30 '20 at 13:38
  • \$\begingroup\$ Any reason this is tagged as C? \$\endgroup\$ – JVApen Apr 30 '20 at 18:31
  • \$\begingroup\$ I took into consideration vdaghan comments and converted the whole stuff to C but it was against the rules. \$\endgroup\$ – user10191234 Apr 30 '20 at 19:12
5
\$\begingroup\$

Thous shalt not mix C/C++ unless you have a good reason to do so. Jokes aside, using pure C++ will lead more bug-free code (in terms of memory management etc.) especially since your statement

What I've learned is to avoid using namespace std; and making the includes in alphabetical orders.

implies (to me) you are a newcomer. You're welcome, by the way. That being said, learning C way of doing things is really helpful, especially if you are not familiar with pointers etc. You will experience first hand, say why C++ smart pointers and containers are a thing.

My advice would be to study exactly why

You should not mix C/C++ unless you have a good reason to do so.

What you are doing with this code is nothing but plain C (except iostream methods). I did not check your code thoroughly though. So why do you want to use C++? If you want to use C++, why do you write C code and compile as C++ (again, except iostream, which can be as well stdio.h)?

Since you are using Digital Mars, for which I can't find any standard compliance information, I am not sure if you can use C++17 STL. If you can, there is a filesystem library which can be used to do what you are trying to accomplish with a few lines of code. If you can't, you can try to find a more up-to-date compiler.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks, yes I am a newcomer, you're right, as for digital mars compiler, it is really a small zip file that can be uncompressed and used on any PC without downloading GB of stuff that I have no idea (at least now) what it does. Another question, assume I work at Microsoft and asked to implement such program, is this the way to present it to my manager or each method should be in a separate file? \$\endgroup\$ – user10191234 Apr 29 '20 at 12:26
  • 1
    \$\begingroup\$ No, I wouldn't do that unless explicitly asked for. This format is fine (except indentation), assuming it works. Note that I did not, and won't check platform-specific (essentially) C code and embarrass myself. There are many people who knows much more about these. But I insist on using a "proper" compiler with C++17 support if you can. If not, try to get rid of malloc/free, char* to make your code "more C++". If we are talking about modern C++, get rid of new/delete too. I am emphasizing this because I can see you have new character array in replace() which is not deleted. \$\endgroup\$ – vdaghan Apr 30 '20 at 0:40
2
\$\begingroup\$

Welcome to the C++ community. You will find a lot of people with outspoken opinions about C++, variating from finding compatibility with C very important to those who find it very dangerous (even in a single person). My opinion: Avoid C unless you have a valid reason to do so, in which case, provide a C API and write the other code in pure C.

Before diving into the code, a small remark: C++17 has std::file_system with a better API than the Windows API.

So to the code:

  • const char * is something to avoid, std::string_view and std::string are much better alternatives.
  • Don't write struct stat status;, stat status; is sufficient and will confuse less c++ programmers
  • Don't declare variables at the top of the function (main), just declare them where you need them
  • Use nullptr instead of NULL
  • Don't mix malloc and new, use the latter in C++

(PS: For the details, please search online, they can motivate it much better than myself)

If I would be doing your review for a C++ job, I don't like this kind of code to show up. HOWEVER even with that, you could still be hired, cause those things can be taught. What's more important is that you can show how to think about a problem and that you understand C++ concepts.

My next question to you let you show off your knowledge about how to write classes. Public, protected, private, and when to use it. Member functions and variables. How the this-pointer works. What RAII is ...

\$\endgroup\$
3
  • \$\begingroup\$ I am using C because to the best best of my knowledge both Windows and Unix were written in C and I really love low level programming. The weird thing that new compiled in Digital Mars only using C headers. My online search gave me basically 2 options either declare *p as static or free it in main. Any suggestions? \$\endgroup\$ – user10191234 Apr 30 '20 at 19:19
  • \$\begingroup\$ Switch to LLVM or MSVC as compiler? \$\endgroup\$ – JVApen Apr 30 '20 at 19:56
  • \$\begingroup\$ Why not, but I was reading that I should learn about compilers and their files and structures, it is hard enough with a 10 MB compiler let alone these 2 and their huge installation. \$\endgroup\$ – user10191234 May 1 '20 at 12:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.