Shoutbox

quote:

---

Originally posted by Zahid™
Already a script that does this: http://www.msgpluslive.net/scripts/browse/index.php?act=view&id=35

---

I don't call that a script tbh. It consist of only 1 line, doesn't check for anything at all. It will screw up commands, etc, etc. Never should have been put in the database IMO. Even JimboDude's first draft of his first script is better than that.

---

JimboDude

I'll comment on various things so you know where things can be improved and why, so you can apply those things in your future scripts too.

As further reference I suggest to read the Official JScript 5.6 reference in the msdn library and/or download the Official Scripting Documentation from Windows.

Terms in italic in this post can be found in those references, look them up


Anyways:

code:

---

if (Message.substr((Message.length)-('?'.length)) == '?' || Message.substr((Message.length)-('!'.length)) == '!' || Message.substr((Message.length)-(':'.length)) == ':' || Message.substr((Message.length)-('.'.length)) == '.' || Message.substr((Message.length)-(';'.length)) == ';' || Message.substr((Message.length)-(','.length)) == ',') {

---

You can simplify all that....

No need for calculating the length of "?", "!", etc... it's always 1 anyways
thus:

code:

---

Message.substr((Message.length)-1)

---

"something-1" can be written as "--something", thus:

code:

---

Message.substr(--Message.length)

---

Don't use substr to grab 1 character inside a string, use charAt instead:

code:

---

Message.charAt(--Message.length)

---

Instead of grabbing the last character each time again, assign the last character to a variable and compare that variable with whatever you need to compare it with:

code:

---

var lastChar = Message.charAt(--Message.length);
if (lastChar == '?' || lastChar == '!' || lastChar == ':' || lastChar == '.' lastChar == ';' || lastChar == ',') {

---

Since you compare that same last character again and again with another character, you can turn this all around and check if that last character can be found into the string "?!:.;,". If so, you have a match if not you don't have a match. Use indexOf for this:

code:

---

if ("?!:.;,".indexOf(Message.charAt(--Message.length)) != -1) {

---

So now

code:

---

if (Message.substr((Message.length)-('?'.length)) == '?' || Message.substr((Message.length)-('!'.length)) == '!' || Message.substr((Message.length)-(':'.length)) == ':' || Message.substr((Message.length)-('.'.length)) == '.' || Message.substr((Message.length)-(';'.length)) == ';' || Message.substr((Message.length)-(','.length)) == ',') {

---

is reduced to:

code:

---

if ("?!:.;,".indexOf(Message.charAt(--Message.length)) != -1) {

---

And if you know regular expressions this can be simplified even more....:

code:

---

if (/[?!:.;,]$/.test(Message)) {

---

Other stuff:
- remove that "Message = null" line. It is not needed. And luckally it is not executed either as that will otherwise produce an error. See this post for more info (and of course the Plus! Scripting Documentation).

- the case where (caps=="on" && dot=="on") doesn't check for already existing dots, commas, question marks, etc

Suggestions:
- Instead of having 4 IF THEN ELSE structures, split them up and examine exactly what each function does with the sentence in what case. You'll find that you will only need 2 IF THEN ELSE structures since you will only actually do something when a setting is on, not when it is off. You also do not need the ELSE case at all.

Result:
highlight to see it

code:

---

function OnEvent_ChatWndSendMessage(ChatWnd, Message) {
        var firstChar = Message.charAt(0);
        var restOfMessage = Message.substr(1);
        if (caps == "on") firstChar = firstChar.toUpperCase();
        if (dot == "on" && /[?!:.;,]$/.test(Message) == false) restOfMessage += ".";
        return firstChar + restOfMessage;
}

---

Bugs still to fix:
- You need to check if the message is not a command (messages beginning with "/" or even "!").
- Check individual lines (you could do this by splitting your message up into an array using the method split with delimiter "\n")
- If you define an ActiveX object, make sure it is always defined. Your code contains several occasion where you use an ActiveX object (WSH), but which isn't defined since you have placed it inside an IF THEN ELSE structure (see OnEvent_MenuClicked function).


More things you can make better:

code:

---

  if(MenuId=="caps"){
(...)
  if(MenuId=="dots"){
(...)
  if(MenuId=="about"){
(...)
  if(MenuId=="config"){
(...)

---

For something like this you better use the switch statement.

code:

---

if(caps=="on") {
      caps="off";
      WSH.RegWrite(MsgPlus.ScriptRegPath + Messenger.MyUserId + "\\" + "caps",caps);
}
else if(caps=="off") {
      caps="on";
      WSH.RegWrite(MsgPlus.ScriptRegPath + Messenger.MyUserId + "\\" + "caps",caps);
}

---

Put the same common lines outside the IF THEN ELSE structure. Then you'll also see that you can reduce the IF THEN ELSE structure to only 1 line using the conditional (ternary) operator. eg: to switch between two states:
myvar = (myvar == "1") ? "2" : "1"

But instead of using strings to indicate the states of the options, use booleans. They are way easier to work with and will reduce the code even further. eg: to switch between two states:
myvar = !myvar

Using booleans will extremely shorten all your code if you use it wisely.