4
\$\begingroup\$

I got a file with lots of ASCII characters. No extension, and has the Type Of file attribute 190000 File.

I have many characters like L¿ö in the file however I required to remove lines from a word PYGR to another word MCG. It has several lines in between.

I tried the following code. And got everything working. Indexes are 3 and 19001 respectively,

VB:

Dim Load_File = System.IO.File.ReadAllText("C:\Documents and Settings\Fousu.s\Desktop\Files\SYSI091512.190000")
 Dim EditedString = ""
 Dim IndexofPYGR = Load_File.IndexOf("PYGR")
 Dim indexOfMCG = Load_File.LastIndexOf("MCG")
 Dim LengthMCG = "MCG"
 'System.Console.ReadKey()
 If ((IndexofPYGR > -1) And (indexOfMCG > -1)) Then
 EditedString = Load_File.Remove(IndexofPYGR, (indexOfMCG - (LengthMCG.Length + 1)))
 System.IO.File.WriteAllText("C:\Documents and Settings\Fousu.s\Desktop\Files\SYSI091512.190000V1", EditedString)
 End If

c#

//Getting the file as a string.
 string Load_File = System.IO.File.ReadAllText(@"C:\Documents and Settings\Fousu.s\Desktop\Files\SYSI091512.190000");
 string EditedString;
 int IndexofPYGR = Load_File.IndexOf("PYGR");
 int indexOfMCG = Load_File.IndexOf("MCG");
 string LengthMCG = "MCG";
 //Console.WriteLine("Index of PYGR is : {0}", IndexofPYGR);
 //Console.WriteLine("Index of MCG is : {0}", indexOfMCG);
 //System.Console.ReadKey(); 
 if ((IndexofPYGR != -1) && (indexOfMCG != -1))
 {
 EditedString = Load_File.Remove(IndexofPYGR, (indexOfMCG - (LengthMCG.Length + 1))); 
 System.IO.File.WriteAllText(@"C:\Documents and Settings\Fousu.s\Desktop\Files\SYSI091512.190000V1", EditedString);

Would like to request a review of the above code, or is there a better way to achieve this? and also would like to know how to get the Type Of File attribute to 190000.

asked Aug 24, 2012 at 13:02
\$\endgroup\$
3
  • 5
    \$\begingroup\$ Those aren't ASCII characters... \$\endgroup\$ Commented Aug 24, 2012 at 13:10
  • 2
    \$\begingroup\$ Yes. This isn't an ASCII file, like you say in the question title. It does have a file extension (of ".190000"). Your approach seems fine for the task in hand, but the inconsistencies in the question are worrying. \$\endgroup\$ Commented Aug 24, 2012 at 13:24
  • \$\begingroup\$ Thanks jon Skeet and Jon Hanna, Actually I have been told that it is an ASCII file. But however I believe its rather an encrypted file. Thanks for pointing that out!! \$\endgroup\$ Commented Aug 27, 2012 at 5:46

2 Answers 2

8
\$\begingroup\$

Some clarifications

I got a file with lots of ASCII characters. [...] I have many characters like L¿ö in the file [...]

As others have pointed out, these are not ASCII characters.

No extension, and has the Type Of file attribute 190000 File.

Configure Windows Explorer to show file extensions.

I required to remove lines from a word PYGR to another word MCG. It has several lines in between.

Your code tells us otherwise. You are not removing the lines from PYGR to MCG; your code removes the word PYGR and every character that follows up to one character before MCG. I assume that is not quite what you were trying to achieve?

(削除) If you update your question to explain if you are trying to remove everything between (excluding) PYGR and MCG, including one of them or including both of them, I'll gladly adapt my answer. For now, I will assume the latter. (削除ここまで)

Edit: All right, in accordance with your comment I have changed my answer to replace all text from PYGR (inclusive) to MCG (exclusive).

Reviewing your code

Readability

  • Please use the well-established C# naming conventions: local variables are camelCase
  • Format your code with the correct indentation to make it more readable
  • Start your file with using System.IO to avoid repeating yourself
  • Get rid of unnecessary parenthesis
  • Choose proper names
  • Join declaration and usage of variables where possible

For example:

string EditedString;
// ...
EditedString = Load_File.Remove(IndexofPYGR, (indexOfMCG - (LengthMCG.Length + 1))); 

should be more like

string editedContent = fileContent.Remove(startIndex, endIndex - end.Length); 

Maintainability

Hard-coding the paths is something that should be avoided even during development. You can use Environment.GetFolderPath with Environment.SpecialFolder.Desktop to retrieve the path to the current user's desktop and add the file name with Path.Combine.

Regex

I really don't like the heave use of indexOf: your code seems to be all about how the replacement is done, instead of what is going on (i.e. it is imperative, not declarative).

Using a Regex, we can arrive at a more declarative style. You may need to adjust this to suit your needs (your question is ambiguous).


Refactored

string desktop = Environment.GetFolderPath(Environment.SpecialFolder.Desktop);
string inFilePath = Path.Combine(desktop, "SYSI091512.190000");
string outFilePath = Path.Combine(desktop, "SYSI091512.190000V1"); 
// you would probably pass the paths into your method as parameters
var fileContent = File.ReadAllText(inFilePath);
var result = Regex.Replace(fileContent, "PYGR.*MCG", "MCG", RegexOptions.Singleline);
File.WriteAllText(outFilePath, result);
answered Aug 24, 2012 at 14:35
\$\endgroup\$
1
  • \$\begingroup\$ First of all thanks a lot for this great suggestions, and it will really help beginners like me. Initially I have been told that it is an ASCII file, however I doubt it is just an encrypted file. No extension for the file, however when I right click the file and have 190000 in the file type field. Yes I would like to remove the all characters or lines including the word PYGR to word MCg (Exclude). However a little doubt how to specify the folder name in the desktop where the file is residing? Thanks for this answer. \$\endgroup\$ Commented Aug 27, 2012 at 5:54
2
\$\begingroup\$

For your VB code specifically:

In general, though some might disagree, I would much prefer to see type specification when I see Dim.

i.e.

Dim EditedString As String = ""
Dim IndexofPYGR As Long = Load_File.IndexOf("PYGR")
Dim indexOfMCG As Long = Load_File.LastIndexOf("MCG")
Dim LengthMCG As String = "MCG"

I don't see anything very wrong with this block, and it actually leaves you open for future expansion (with regard to the EditedString variable), but if you wanted to simplify, you could...

Change this:

If ((IndexofPYGR > -1) And (indexOfMCG > -1)) Then
 EditedString = Load_File.Remove(IndexofPYGR, (indexOfMCG - (LengthMCG.Length + 1)))
 System.IO.File.WriteAllText("C:\Documents and Settings\Fousu.s\Desktop\Files\SYSI091512.190000V1", EditedString)
End If

To:

If ((IndexofPYGR > -1) And (indexOfMCG > -1)) Then
 System.IO.File.WriteAllText("C:\Documents and Settings\Fousu.s\Desktop\Files\SYSI091512.190000V1", Load_File.Remove(IndexofPYGR, (indexOfMCG - (LengthMCG.Length + 1))))
End If

However, I would consider converting the path string to a variable and place it elsewhere earlier in your code. This allows you to use it in more than one place while requiring you to make only one change in your code later if that path changes.

Taking my last suggestion, I would even break that down more to allow for more possible file names (if that would be useful for your case specifically or not is up to you to decide). I did not make this change in the code below.

My final result, based on these suggestions would be the following:

Dim PathAndFilename As String = "C:\Documents and Settings\Fousu.s\Desktop\Files\SYSI091512.190000"
Dim Load_File = System.IO.File.ReadAllText(PathAndFilename)
Dim IndexofPYGR As String = Load_File.IndexOf("PYGR")
Dim indexOfMCG As Long = Load_File.LastIndexOf("MCG")
Dim LengthMCG As String = "MCG"
'System.Console.ReadKey()
If ((IndexofPYGR > -1) And (indexOfMCG > -1)) Then
 System.IO.File.WriteAllText(PathAndFilename & "V1", Load_File.Remove(IndexofPYGR, (indexOfMCG - (LengthMCG.Length + 1))))
End If
answered Aug 24, 2012 at 22:19
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thanks for this valuable suggestions, will try my best to follow this standards better in my future codes. I am unable to vote up any answers or comments as I don't have enough reputations !! \$\endgroup\$ Commented Aug 27, 2012 at 5:56
  • 2
    \$\begingroup\$ No worries; you can always come back and up-vote later if you wish. Glad I could help! \$\endgroup\$ Commented Aug 27, 2012 at 11:18

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.