[RexGrammer]

This is a notepad I am making (that is it isn't finished), so I was wondering if you would review it both the source and the overall interface, sorry I haven't commented anything (bad habbit) will do it soon....

It is to have the same capabilities as notepad, and then I will add some to it...

PROJECT RexPad

If you're up to it submit modifications to it... I'll gladly mention you as one of the developers...

All advice appreciated...

This post has been edited by RexGrammer: 01 December 2011 - 03:03 PM

[Curtis Rutland]

One note, I'd suggest structuring your projects more similarly to mine in Github. For example:

https://github.com/c...aBindingExample

Notice the ".gitignore" file. It lets you exclude files based on wildcards from your commits. I like it this way better, since you don't have to create your own .zip files for your source. This maintains the .sln and the .csproj files, and github itself offers you a .zip download option (so you don't have to do it yourself).

That's just a first option I'd suggest. It'll make it easier for others to use your code, since they can just download using the interface that Github exposes and then load the solution.

I'll give it a once-over and comment again later.

[tlhIn`toq]

I just downloaded the zip on your hub. There is no visual studio solution.

If you want people to review it you at least have to make it easy on them.

Take the entire solution folder and .zip it and attach it.

[janne_panne]

At a really quick glance, here are my small suggestions/tips:

This:

if (file != null && file != "")

Can be replaced by:

if (string.IsNullOrEmpty(file))
// or, depending on framework version:
if (string.IsNullOrWhiteSpace(file))

And I use streams like this so they get disposed, but I'm not sure if that's necessary:

using (StreamWriter myWriter = new StreamWriter(file)) 
{
  myWriter.Write(textBox1.Text);
  myWriter.Flush();
  myWriter.Close();
}

[Curtis Rutland]

@janne_panne, it is smart to use using statements for Streams. However, it's not necessary to manually flush and close.

http://msdn.microsof...y/ms227422.aspx

Quote

This method disposes the stream, by writing any changes to the backing store and closing the stream to release resources.

...

...this method calls Close, which then calls Stream.Dispose(Boolean).

Close handles flushing, so there's no need to do anything other than create and use the Streams.

using (StreamWriter sr = new StreamWriter(path)){
  sr.Write(textBox1.Text);
}

[janne_panne]

Thanks Curtis, I'll be able make my code cleaner now without flushing and closing the streams.

[tlhIn`toq]

Find dialog and a Find/Replace dialog. I wouldn't have made this separate dialogs. For one you now have to keep them synchronized for features. For example your find has an ()up ()down search feature that find/replace does not. All the find functionality shouldn't be duplicated in find/replace.

---

You don't seem to be taking into account a lot of the possibilities. For example

        private void Open()
        {
            openFileDialog1.Filter = "Text files (*.txt)|*.txt|All files (*.*)|*.*";
            openFileDialog1.ShowDialog();
            file = openFileDialog1.FileName;

The assumption here is that a file was selected. What if the user clicked [cancel]? You shouldn't be doing anything in that case. openFileDialog.ShowDialog() returns a DialogResult. If that result is not DialogResult.Ok then you shouldn't take action.

---

All of your methods return void. How do you plan to handle errors?

---

You have duplicate logic in a few places. The save and saveas for example. Duplicate code is a maintenance nightmare.

What about this logic for that:
You have a single save function.
If save is called and no filename is pre-specified then get the name/path. Then call the single save function.
For saveas, again get the new name/path, then call the single save method.

---

        private void ResizeTextBox()
        {
            textBox1.Width = this.Width - 16;
            if (statusStrip1.Visible)
                textBox1.Height = this.Height - menuStrip1.Height - 60;
            else if (statusStrip1.Visible == false)
                textBox1.Height = this.Height - menuStrip1.Height - 37;
            textBox1.Top = menuStrip1.Height;
            textBox1.Left = 0;
        }

You don't what to use this. as your size reference because that's the entire form including borders. You want this.ClientRectangle as that is the working space within your form. Hard coding your adjustments like that will bite you in the ass as soon as the user has some custom theme installed with weird border sizes.

You can avoid all of that if you just use the .Anchor property and anchor all four sides. Then they move automatically as the form resizes.

---

This is just... well... excessive I guess is the word.

        private void TextBoxModeChange()
        {
            if (wordWrapToolStripMenuItem.Checked)
            {
                textBox1.WordWrap = false;
                textBox1.ScrollBars = ScrollBars.Both;
                wordWrapToolStripMenuItem.Checked = false;
            }
            else // Stop there. No need to check the opposite of what you just checked above if (wordWrapToolStripMenuItem.Checked == false)
            {
                textBox1.WordWrap = true;
                textBox1.ScrollBars = ScrollBars.Vertical;
                wordWrapToolStripMenuItem.Checked = true;
            }
        }

What do you think of this cleaned up version?

        private void TextBoxModeChange()
        {
            // Set the .Checkonclick propety to true in the designer
            textBox1.WordWrap = wordWrapToolStripMenuItem.Checked;
            textBox1.ScrollBars = wordWrapToolStripMenuItem.Checked ? ScrollBars.Vertical : ScrollBars.Both;
        }

---

You aren't tracking if changes were made. When the user selects exit you just quit without offering to save the file.

Same problem with New file. What if there were unsaved changes on the existing document?

---

This post has been edited by tlhIn`toq: 01 December 2011 - 03:56 PM

[Curtis Rutland]

janne_panne, on 01 December 2011 - 04:41 PM, said:

Thanks Curtis, I'll be able make my code cleaner now without flushing and closing the streams.

I have this handy static class I use:

public static class FileHelper {
    public static string Load(string path) {
        string text;
        using (var sr = new StreamReader(path)) {
            text = sr.ReadToEnd();
        }
        return text;
    }

    public static string[] LoadLines(string path) {
        var lines = new List<string>();
        using (var sr = new StreamReader(path)) {
            while (!sr.EndOfStream) lines.Add(sr.ReadLine());
        }
        return lines.ToArray();
    }

    public static void Write(string text, string path, bool append = false) {
        using(var sw = new StreamWriter(path, append)) sw.Write(text);
    }
}
//...to be invoked like this:

var text = FileHelper.Load(@"c:\dev\test.txt");
var lines = FileHelper.Load(@"c:\dev\test2.txt");
FileHelper.Write("Help, I'm trapped in a text file!", @"c:\dev\SOS.txt");

[RexGrammer]

Thanks gonna fix many things soon (I hope)

Just a quick question about git: Should I include the builds in my git repo?

This post has been edited by RexGrammer: 02 December 2011 - 01:39 PM

[Curtis Rutland]

I usually don't include built files (my .gitignore file excludes both bin and obj directories). I feel that people can compile my code for themselves, if they want. But there's no reason not to, except for space considerations. If you're not worried about that, then it's OK to include a build folder.

[RexGrammer]

@tlhIn`toq Thanks for all your info, just one question: Should I apply the similar to the open method?
And what does

textBox1.ScrollBars = wordWrapToolStripMenuItem.Checked ? ScrollBars.Vertical : ScrollBars.Both;

this mean?
I get that it sets the scorllbars depending of the checked property of the wordWrapToolStripMenuItem but the ? ScrollBars.Vertical : ScrollBars.Both part is just a mistery to me...?
Oh and I was trying to mimic MS Notepad TO THE LETTER and well if it has two different forms for find and find/replace then I will too...

This post has been edited by RexGrammer: 02 December 2011 - 03:04 PM

[Curtis Rutland]

The ?: operator is the inline if statement.

x = a ? b : c;

is equivalent to

if(a)
  x = b;
else
  x = c;

b and c must resolve to the same type as x, and a must resolve to a bool.

[RexGrammer]

Well I did a lot from the time of the last post in this topic, so I am again asking you but not to review it but to start testing it, so that I may fix eventual errors, if you want to review it please do, also please submit all bugs to the issues in my GitHub, so I am again inviting you to my PROJECT RexPad

P.S. Sorry for being such a nuisance this will be the last time (at least for this project) I am inviting you to do something like this

[modi123_1]

Do you have the test plan handy?

[RexGrammer]

Well not but it would be something like this (an extremely short version):

Scope:

Unit, Integration, System, Regression, and Client Acceptance testing approach

• Testing of all functional, application performance, security
  
• Quality requirements and fit metrics.
  
• End-to-end testing and testing of interfaces of all systems that interact with the RexPad.

Methodology:

All people do tests, they report bugs to the GitHub interface

Requirements:

.NET Framework 4

Criteria for pass-fail:

• Any crash, fail
  
• Any bug, fail
  
• Any program error, fail
  
• Stability, pass

Schedule:

Anytime