Practice Problem, File move

[vidit_X]

Question by ~s.o.s~
Write a program which will perform the job of moving the file from one location to another. The source and destination path will be entered by the user. Perform the required error checking and handle the exceptions accordingly. (Intermediate)

#include<fstream>
#include<iostream>
#include<cstdio>
using namespace std;
int main(){ 
 fstream a,b;
 char f1[100],f2[100],x[100];
 cout<<"Enter file1 path";
 cin.getline(f1,100);
 cout<<"Enter file2 path";
 cin.getline(f2,100);
 try {
	 a.open(f1,ios::in|ios::binary);
	 if(!a)
		 throw 1;
	 try {
		 b.open(f2,ios::out|ios::binary);
		 if(!b)
			 throw 1;
		 while(!a.eof()){
			 a.getline(x,100);
                         b<<x<<"\n";
		 }
	 }catch(int i) {
		 cout<<"Error Writing file";
		 cin.get();
		 cin.get();
		 return -1;
	 }
 }catch(int i) {
	 cout<<"Error opening File";
	 cin.get();
	 cin.get();
	 return -1;
 }
 a.close();
 b.close();
 try{
	 if(remove(f1))
		 throw 1;
	 cout<<"File Moved";
 }catch(int i) {
	 cout<<"Error moving file";
	 cin.get();
	 cin.get();
	 return -1;
 }
 cin.get();
 cin.get();
 return 0;
}

What is the complexity of the above code?
Any corrections, comments or suggestions are welcomed :)

[Ancient Dragon]

1) include stdlib.h and use _MAX_PATH macro to declare the size of the source and destination buffers. On MS-Windows its value is 260. In c++ programs it will be better to use std::string variables instead of char arrays. If you do that then you don't need stdlib.h.

line 21: The file is opened in binary mode. getline() works on files opened in text mode. either change the open methos (not recommended) or call read() instead of getline().

line 22: file b has not been opened. If the input file is opened in binary mode then the output file should also be opened in binary mode, and use write() instead of << operator which is for files opened in text mode.

[mrnutty]

That code is pretty bad. Its congested with unneeded try and catch
statement. You naming convention is very bad. You code is not easy on
the eyes. You are using a char arrays which is a bug in my book.
Why open it in binary? You do not comment on your code.
You do not make use of functions to make code more readable.
Prefer getline(...) over istream.getline().
Overall I would burn that code, sorry and give you a C.

P.S : You spelled practice wrong in your tag.

[vidit_X]

I accept this code is not good though its working, catching exceptions all as asked in question.

Members here are experts, so helping and friendly. I'm sure I'll improve my code as well as coding here.

P.S: How to edit the thread title? Don't know how I typed it wrong :(

[mrnutty]

1) Rename everything to something appropriate.
2) Don't worry about speed right now
3) Make function to read and write to a file
4) Remove all try and catch statements
5) Terminate process if an error occurs
6) Use std::string

Fix those and get back.

[vidit_X]

Here's a little modified code using std::string.

#include<iostream>
#include<fstream>
#include<string>

using namespace std;

int mvfile(const string&,const string&);

int main(){
 string source,dest;
 cout<<"Enter FilePath, source -";
 cin>>source;
 cout<<"Enter FilePath, Destination -";
 cin>>dest;
 try{
         mvfile(source,dest);
 }catch(char er[]){
	 cout<<er;
	 return -1;
 }
 return 0;
}

int mvfile(const string &src,const string &dest){
 fstream file1,file2;
 fstream::pos_type size;
 char *data;
 file1.open(src.c_str(),ios::in|ios::binary|ios::ate);
 file2.open(dest.c_str(),ios::out|ios::binary);
 if(!file1.is_open()){
	 throw "Error Opening File";
	 return -1;
 }
 if(!file2.is_open()){
	 throw "Error Writing File";
	 return -1;
 }
 size=file1.tellg();
 data=new char[size];
 file1.seekg(ios::beg);
 file1.read(data,size);
 file2.write(data,size);
 file1.close();
 file2.close();
 delete[] data;
 if(remove(src.c_str())){
   throw "Error Moving File";
   return -1;
 }
 return 0;
}

Hows this one? Any suggestions or comments are welcome.

[vidit_X]

Anyone..

[WaltP]

Your formatting is bad.

[vidit_X]

I try to format as well as I can, but I never get it the right way.
Can I get a model code, so that I can learn the proper way of formatting?

Thanks WaltP. Any other suggestions or corrections are welcome.

[WaltP]

Formatting

[vidit_X]

#include<iostream>
#include<fstream>
#include<string>

using namespace std;

int mv(const string&,const string&);

int main()
{
	string source, dest;

	cout<<"Enter FilePath, source -";
	cin>>source;
	cout<<"Enter FilePath, Destination -";
	cin>>dest;
	
	try{
		mv( source, dest );
	}catch( char er[] ) {
		cout<<er;
		return -1;
	}

	cin.get();
	return 0;
}

int mv(const string &a,const string &b)
{
	fstream				file1, file2;
	fstream::pos_type	        size;
	char                            *data;

	file1.open( a.c_str(), ios::in | ios::binary | ios::ate );
	file2.open( b.c_str(), ios::out | ios::binary );

	if( !file1.is_open() ) {
		throw "Error Opening File";
		return -1;
	}

	if( !file2.is_open() ) {
		throw "Error Writing File";
		return -1;
	}

	size=file1.tellg();                    //Get the size of file1
	data=new char[size];
	file1.seekg( ios::beg );             //Set the get pointer at ios::beg
	file1.read( data, size );              //read the file1
	file2.write( data, size );             //write the file2
	file1.close();                         //close file1
	file2.close();                         //close file2
	delete[] data;                         //free memory

	if( remove( a.c_str() ) ) {
		throw "Error Moving File";
		return -1;
	}

	return 0;
}

Hows the formatting now?
Any other suggestions or corrections are welcome.

[WaltP]

Hows the formatting now?
Any other suggestions or corrections are welcome.

MUCH better.

You will find all your code will be easier to follow by always formatting. It helps us, too, when you need us to look at your code.

[Ancient Dragon]

P.S: How to edit the thread title? Don't know how I typed it wrong :(

You will have to ask a moderator to do it for you, and tell the moderator what you want for the new thread title.

[vidit_X]

@WaltP - Any corrections for the code?

@AncientDragon - Can you please change the "Practise" to "Practice" in title?

[mitrmkar]

A couple of suggestions;

• Maybe change the signature of int mv(...) to void mv(...) Currently you have the function returning a value, which is completely ignored by the main() function, hence serving no practical use. And in case of errors, you throw exceptions, which makes the 'return -1;' statements to be of no use.

• You could also check that the file is correctly copied before it gets deleted. As of now, there is no checking against that kind of failure.

[mrnutty]

1) Change you function name. What the hell is "mv" ? I wouldn't know
just by looking at the name.
2) There is no need to throw error. It just congests the code.
3) You should not use dynamic memory if memory on stack would do.
4) There is no need to close the file, it is done by the destructor.
5) You could make your code more readable, by seperating the reading
and writing functionality. Make a seperate function that reads the content. Make another function that writes the content. Then use them
in the "mv" function. And Change that function name, ASAP.

But this much better than your first post.

[WaltP]

4) There is no need to close the file, it is done by the destructor.

Bad form. Open a file? Close the file. Period.

Do you also never free dynamic memory because when the program exits it cleans up?

[mrnutty]

>>Bad form. Open a file? Close the file. Period.

Adding extra code when not needed just clutters the code. I see
no point to close the file when the destructor will do the job, its redundant, not Bad Form.

>>Do you also never free dynamic memory because when the program exits it cleans up?

I don't use dynamic memory. I let STL handle those and let their
destructor free it.

[WaltP]

Who the h--l taught you that? He should be shot.

Please don't help anyone else. You are teaching very bad habits -- even worse than instructors teaching Turbo-C and conio.h

[mrnutty]

Who the h--l taught you that? He should be shot.

Please don't help anyone else. You are teaching very bad habits -- even worse than instructors teaching Turbo-C and conio.h

I am not sure behind your rational. Why do something twice, when one
is enough ? Maybe if you give a good example and reasoning then
I might see your point. Until then just saying that its a bad habit
doesn't cut it. Curious on what other people think about this.

P.S :
Just curious, because you disagreed with one of my rational, why do you say not to help anyone else?
Did you witness me "preaching" this to anyone else? Did you find any other advice that I gave bad advice?

[vidit_X]

@mitrmkar - Verifying the move is a good option, but its like an addon to the main program, so I'll do it later. Thanks for the idea. Any other suggestion?

@firstPerson & WaltP - I know that a file's automatically closed by the program but remove() doesn't works if any references to file exists or if the file is currently open. So as to use remove, I closed the files.

I thought of using functions for read and write, they surely will make the program more readable and modular but they increased the code which I felt like is redundant.

About throwing exceptions, I was doing so because it was asked in the problem. I'm trying to make exception throwing efficient and proper.

And should we clear dynamic memory allocated or leave it to the compiler?

Please correct me if I'm wrong anywhere...

[Ancient Dragon]

1) Change you function name. What the hell is "mv" ? I wouldn't know
just by looking at the name.

You apparently have never used *nix. mv is a common *nix program that moves a file (or directory) from one place to another. Lots of *nix program have very cryptic names and arguments.

>>@vidit_x & @firstPerson & WaltP - .

As for closing files -- it depends. Like return 0; in main() you can elect to close it or not. I only close files if there is no other code to be executed, that is, if the function just returns after the file processing is finished. There are obvious times when the file(s) must be explicitly closed, such as when you want to delete the file. But I don't think that is what firstPerson is talking about. I doubt he meant "never" close files, only don't close them unless its otherwise necessary.

[mrnutty]

You apparently have never used *nix. mv is a common *nix program that moves a file (or directory) from one place to another. Lots of *nix program have very cryptic names and arguments.

>>@vidit_x & @firstPerson & WaltP - .

As for closing files -- it depends. Like return 0; in main() you can elect to close it or not. I only close files if there is no other code to be executed, that is, if the function just returns after the file processing is finished. There are obvious times when the file(s) must be explicitly closed, such as when you want to delete the file. But I don't think that is what firstPerson is talking about. I doubt he meant "never" close files, only don't close them unless its otherwise necessary.

No I never used *nix.mv, but he has a chance to rename it
so people like me ( a young one, with only college experience )
can understand it better. Thats why I suggested to rename it
something clearer. Clearly renaming it moveFile rather than mv
sounds better.

As ancient dragon pointed out, I was not saying that to never close
the file explicitly. All I was saying is that when you can let the
destructor close the file then let it.