Stack implementation in Python 3 I'm learning Python by implementing commonly used data structures. My primary language is Java, thus would like to know if I'm being Pythonic enough. Any means to better up will be appreciated.

class Node:
    def __init__(self, value=None, next=None):
        self.value = value
        self.next = next

from node import Node

class Stack:
    def __init__(self):
        self.top = None
        self.size = 0

    def push(self, item):
        curr = Node(item)
        self.size += 1
        if self.top is None:
            self.top = curr
        else:
            curr.next = self.top
            self.top = curr

    def peek(self):
        return self.top.value

    def pop(self):
        if self.top is None:
            raise Exception("Nothing to pop.")
        curr = self.top
        self.top = self.top.next
        self.size -= 1
        return curr

    def __sizeof__(self):
        return self.size

    def is_empty(self):
        return self.size == 0;

    def __str__(self):
        curr = self.top
        while curr:
            print(curr.value, end=' ')
            curr = curr.next

The easiest way to create Stack that I found was this:

class Stack:
    def __init__(self):
        self.stack = []
    def push(self,data):
        self.stack.append(data)
    def pop(self):
        if self.isEmpty():
            raise Exception("nothing to pop")
        return self.stack.pop(len(self.stack)-1)
    def peek(self):
        if self.isEmpty():
            raise Exception("Nothing to peek")
        return self.stack[len(self.stack)-1]
    def __sizeOf__(self):
        return len(self.stack)
    def isEmpty(self):
        return len(self.stack) == 0
    def __str__(self):
        for x in self.stack.reverse(): #Reverse list, so when printing last pushed value is first and the oldest is printed last.
            print(x)

What it does is when creating Stack Variable:

it makes new list called stack.
When you push something to Stack, it is added at the end of list.
When you pop something from Stack, it is taken away form list (method pop() will not only return value from list, but also delete it)
When you peek something from Stack, it returns last value on list without deleting it.
When you want to check size of stack, it measures list size.
When you want to use __str__ you print all variables from list.

I have reversed the list using
for x in stack:. This was done because otherwise x would be equivalent to stack[0] in first run, stack[1] in second etc. However we must not forget that stack[0] is the oldest value placed in stack, and the newest is the value with the highest index in list. Reversing the list allows to print from the newest value to the oldest.

A little bit more of explanation:

self.stack.pop(len(self.stack)-1) - This code will pop last value from list. It contains -1 because length of stack is started from 1 while indexing is started from 0. Function len() returns (in this case) length of list
if self.isEmpty(): raise Exception("Nothing to peek") - This code will run its function isEmpty() from its own class, which returns Boolean value (which is perfect for our if statement).
self.stack[len(self.stack)-1] while using list you will use list name (in this case stack) and then index you want to access, e.g. stack[1] (will return second value in list, dont forget about 0-indexing).


Javacript/jQuery animation makes text appear/disappear as if typed On Eric Amodio's website and JS.ORG, there is a Javascript animation which makes text appear/disappear as if it is being typed.
I tried to replicate something similar but far more basic on JSFiddle.
In my experiment, the words Code review appear typed, are hidden by toggle(200), and the words Code review appears again.
My final result was a mess of  tags and massive blocks of consequent setTimeout() JS functions.

 |

p {
  line-height: 10px;
}

setTimeout(function() {
  $("#bar").before("C");
}, 100);

setTimeout(function() {
  $("#bar").before("o");
}, 200);

setTimeout(function() {
  $("#bar").before("d");
}, 300);

setTimeout(function() {
  $("#bar").before("e");
}, 400);

setTimeout(function() {
  $("#bar").before(" ");
}, 500);

setTimeout(function() {
  $("#bar").before("R");
}, 600);

setTimeout(function() {
  $("#bar").before("e");
}, 700);

setTimeout(function() {
  $("#bar").before("v");
}, 800);

setTimeout(function() {
  $("#bar").before("i");
}, 900);

setTimeout(function() {
  $("#bar").before("e");
}, 1000);

setTimeout(function() {
  $("#bar").before("w");
}, 1100);

/* HIDE .TYPED-ANIMATION */

setTimeout(function() {
  $(".typed-letter").toggle(200);
}, 1300);

/* SECOND ITERATION */

setTimeout(function() {
  $("#bar").before("C");
}, 1600);

setTimeout(function() {
  $("#bar").before("o");
}, 1700);

setTimeout(function() {
  $("#bar").before("d");
}, 1800);

setTimeout(function() {
  $("#bar").before("e");
}, 1900);

setTimeout(function() {
  $("#bar").before(" ");
}, 2000);

setTimeout(function() {
  $("#bar").before("R");
}, 2100);

setTimeout(function() {
  $("#bar").before("e");
}, 2200);

setTimeout(function() {
  $("#bar").before("v");
}, 2300);

setTimeout(function() {
  $("#bar").before("i");
}, 2400);

setTimeout(function() {
  $("#bar").before("e");
}, 2500);

setTimeout(function() {
  $("#bar").before("w");
}, 2600);

Fiddle

How can I use functions, arrays and loops in my JS/jQuery instead of massive blocks of consequent setTimeout functions to simplify my code and make it more efficient?
How can I make it so that the text also disappears in a 'typed' way, instead of using toggle(200) to hide the text again?

For those who dont like typed.js, Modified code for the user code is.

Here,

content -> is the text that shows the typing effect.
speed ->  is the typing effect speed.   i  -> this will help in
iterating over the content till it reaches the content length.
timer  -> we use this variable to assign the setTimeout(), so
that after the text content is looped, we can use this variable to
clear timer. This  will prevent memory leakage.
intervalCount  ->
this variable is used to initialise how many times the content need
to toggle.

const content = 'Code Review'; // text content to show typing effect
const speed = 1000; // this is the typing speed
var i = 0;  // this is used to iterate over the text content
var timer = 0;  // this is used to set and clear timer. This will prevent memory leakage
var intervalCount = 0; // this is used to set no. of times typing effect should loop.

Here initially i will be 0, then it checks with the content length. If i < content.length, then it will add content[i] to the dom. After that i is incremented by 1, then a setTimeout() function is called. This will invoke the typeSetting once again, at this time i will 1 and it will print content[1]. This will loop untill i become the content length.

This condition will iterate over the text content and displays them.

if (i < content.length) {
        el.before(`<span class='typed-letter'>${content[i]}</span>`);
        i++;
        timer = setTimeout(function() {
          typeSetting(intervalCount)
        }, speed);
      }

Now the else part, here first it will clear the timer ( Which will prevent the memory leakage ), then it decrement the intervalCount, check for the intervalCount is  > 0, then set i = 0 ( this will help in printing the string from begining ). Again in a setTimeout() is called. At this time, the printed content is removed and starts printing again. Again the if loop explained above is executed. This will continue untill the intervalCount becomes 0.

else {
clearTimeout(timer);
intervalCount = intervalCount ? intervalCount - 1 : 0;
if (intervalCount) {
    i = 0;
    timer = setTimeout(function() {
    $(".typed-letter").empty();
    typeSetting(intervalCount);
    }, 300);
}
}

For more realistic typing effect, we can add a blinking animation to the |.

.blinking{
    animation:blinkingText 1.2s infinite;
}
@keyframes blinkingText{
    0%{     color: #000;    }
    49%{    color: #000; }
    60%{    color: transparent; }
    99%{    color:transparent;  }
    100%{   color: #000;    }
}

const content = 'Code Review';
const speed = 100;
var i = 0;
var timer = 0;
var intervalCount = 0;

function typeSetting(interval) {
  let el = $('#bar');
  if (interval) intervalCount = interval;
  if (i < content.length) {
    el.before(`<span class='typed-letter'>${content[i]}</span>`);
    i++;
    timer = setTimeout(function() {
      typeSetting(intervalCount)
    }, speed);
  } else {
    clearTimeout(timer);
    intervalCount = intervalCount ? intervalCount - 1 : 0;
    if (intervalCount) {
      i = 0;
      timer = setTimeout(function() {
        $(".typed-letter").empty();
        typeSetting(intervalCount);
      }, 300);
    }
  }
}

(function() {
  typeSetting(2); // may be you are using some kind of repetition count, set the count. If no params then will show 1 time.
})();
p {
  line-height: 10px;
}
.blinking{
    animation:blinkingText 1.2s infinite;
}
@keyframes blinkingText{
    0%{     color: #000;    }
    49%{    color: #000; }
    60%{    color: transparent; }
    99%{    color:transparent;  }
    100%{   color: #000;    }
}

 |

Here no external packages are used, no libraries. Its pure javascript.


Tree node filtering with List I had a question about filtering tree view and returning just first match.

I've tried to create list<treenode> and change the code.  Is it possible improve the below code for finding nodes and child nodes that match my criteria?

private List<TreeNode> FindNodeByValue(TreeNodeCollection nodes, string searchstring)
{

    // Loop through the tree node collection
    List<TreeNode> nodelist=new List<TreeNode>();

    foreach (TreeNode node in nodes)
    {
        // Does the value match the search string?
        if (node.Value.ToUpper().Contains (searchstring.ToUpper()))
            // Yes it does match - return it
            nodelist.Add(node);
        //  return nodelist;
        else
        {
            // No it does not match - search any child nodes of this node
            List<TreeNode> childNode = SearchChildNodes(node, searchstring);
            // If the childNode is not null it was a match
            if (childNode != null)
                // Return the matching node
                return childNode;
        }
    }
    // If the matching node is not found return null
    return null;
}

/// <summary>
/// This method searches a node's ChildNodes collection to find a matching value
/// with the incoming search string
/// It will iteratively call itself as it drills into each nodes child nodes (if present)
/// </summary>
/// <param name="parentNode">Parent node to search for a match</param>
/// <param name="searchstring">string to be matched with the Nodes Value property</param>
/// <returns>Treenode of the matching node if found.  If not found it will be null</returns>
private List<TreeNode> SearchChildNodes(TreeNode parentNode, string searchstring)
{
    List<TreeNode> nodelist2 = new List<TreeNode>();
    // Loop through the child nodes of the parentNode passed in
    foreach (TreeNode node in parentNode.ChildNodes)
    {
        // Does the value match the search string?
        if (node.Value.ToUpper().Contains(searchstring.ToUpper()))
            // Yes it does match - return it
            nodelist2.Add(node);
            //return nodelist2;
            //return node;
        else
        {
            // No it does not match - recursively search any child nodes of this node
            List<TreeNode> childNode = SearchChildNodes(node, searchstring);
            // If the childNode is not null it was a match
            if (childNode != null)
                childNode.Add(node);
                // Return the matching node
                return childNode;
        }
    }
    // If the matching node is not found OR if there were no child nodes then return null
    return null;
}

protected void Button1_Click(object sender, EventArgs e)
{
    TreeNode trnode=FindNodeByValue(TreeView1.Nodes, fieldFilterTxtBx.Text);
    if (trnode != null)
    {
        TreeView1.Nodes.Clear();
       // TreeNode newnode = new TreeNode("Detail Engineering");
       // TreeView1.Nodes.Add(newnode);
        TreeView1.Nodes.Add(trnode);
        TreeView1.ExpandAll();
    }
    else
    {
        Label1.Text = "No file found";
    }
}

Currently you do not search sub nodes it a matching parent is found. This code fixes that problem and also simplifies the code by removing the unnecessary SearchChildNodes function. If you only care about the first match use var node = FindNodeByValue(TreeView1.Nodes, fieldFilterTxtBx.Text).FirstOrDefault();

private IEnumerable<TreeNode> FindNodeByValue(TreeNodeCollection nodes, string searchstring)
{
    foreach (TreeNode node in nodes)
    {
        if (node.Value.IndexOf(searchstring,
              StringComparison.CurrentCultureIgnoreCase) >= 0)
            yield return node;
        else
        {
            foreach (var subNode in FindNodeByValue(node.ChildNodes, searchstring))
                yield return subNode;
        }
    }
}


while(user == gullible) The goal for this project was to:

  Write a program that continues to asks the user to enter any number other than 5 until the user enters the number 5.
  Then tell the user "Hey! you weren't supposed to enter 5!" and exit the program.

  ★ Modify the program so that after 10 iterations if the user still hasn't entered 5 will tell the user "Wow, you're more patient then I am, you win." and exit.

  Requires:

  variables, data types, and numerical operators
  basic input/output
  logic (if statements, switch statements)
  loops (for, while, do-while)

(I'm not doing the 2 stars one yet)

My questions are:

How can I optimize my code in every possible way? (efficiency, readability, etc)
How can I improve the code syntax?

#include <iostream>
using std::cout;
using std::cin;

int main()
{

    int UserNumber = 0;
    int k = 1;
    while (k < 11)
    {
        cout << "\nEnter any number other than 5: ";
        cin >> UserNumber;

        if (UserNumber == 5)
        {
            cout << "\n\n\nHey! You weren't supposed to enter 5!\n\n\n";
            exit(0);
        }

        else if (k == 10)
        {
            cout << "Wow, you're more patient then I am, you win.\n\n\n";
            exit(0);
        }
        k++;
    }

}

A simple rule I (and other) try to apply is : define things in the smallest possible scope.

For instance, int UserNumber = 0; can be moved inside the loop.

Similarly, k can be moved once you've made your loop a for loop : for(int k = 1; k < 11; k++).

Then, computer people love counting from 0. If you write : for(int k = 0; k < 10; k++), I am used to this and I know straight-away that there will be 10 iterations. If you re-index your loop, you'll need to convert else if (k == 10) into a else if (k == 9).

Then, you'll realise that there is no need for this check, you can just move this out of the loop.


Refresh webpage on change in folder I want to refresh my web page every time a file is uploaded to the folder.

I have a web service written in flask having the following handler

@app.route('/getlatest/')
def getlatest():
    import os
    import glob
    newset = max(glob.iglob('static/*'),key=os.path.getctime)
    Return newest;

This gives me the name of the latest file in the folder.

I have an Ajax call in my JS (client side) to constantly get data from above function.

function GetLatest()
            {
                $.ajax({
                    url: "http://localhost:5000/getlatest",
                    success: function(result)
                    {
                        if(previousName != result){
                            previousName = result;
                            $("#image").attr("src","/"+previousName);
                        }
                    }
                });
            }

function calling server every second.

(function myLoop (i) {
    setTimeout(function () {
    GetLatest();
    if (--i) myLoop(i);
       }, 1000)
})(100);

This Works fine (well almost). My Question is: Is there any better way to do it (there must be)?

I'm open to technology choices what every they may be (node, angualr etc).

Server side:

There's no point in importing os and glob inside the function. Local import is used for very specific cases and this is not one of them.
You're defining newset and then returning newest, did you test your code?
There's no point in defining newest (after fixing the typo), and then returning it in a separate line. Just return it in one line.
Add a space before key=..., as per Python conventions.

Client side:

What you've implemented here isn't very smart. You're constantly asking the server for updates, instead of having the server push updates to you when ever it detects them. You've essentially implemented a REST API, but for the wrong reasons. Take a look at websockets (flask-socketio is pretty good).


Refactor for using ChannelFactory I use VS 2008 and .NET 3.5

I have this code using CustomChannelFactory and Proxy ServiceReference. I would like refactor using lambdas, Action, ...

any suggestions ?

ServiceDependencias.IDependencias svcDependencias = null;

public void Dependencias(List<MfaFicheroForm> listaFicherosFormXML, string entorno)
{
    CustomChannelFactory<ServiceDependencias.IDependencias> customChannelFactory = null;

    if (!string.IsNullOrEmpty(FicheroConfiguracionWCFService)
        && !string.IsNullOrEmpty(EndpointNameWCFService))
    {
        if (!File.Exists(FicheroConfiguracionWCFService))
            throw new FileNotFoundException("Fichero de configuración WCF no encontrado", FicheroConfiguracionWCFService);

        customChannelFactory = new CustomChannelFactory<ServiceDependencias.IDependencias>(EndpointNameWCFService, FicheroConfiguracionWCFService);

        var iDependencias = customChannelFactory.CreateChannel();
        svcDependencias = iDependencias;
    }

    if (svcDependencias == null)
    {
        var depClient = new DependenciasClient();
        svcDependencias = depClient;
    }

    GenerarDependenciasDeFormulariosFmb(listaFicherosFormXML, entorno);

    if (svcDependencias is DependenciasClient)
    {
        ((DependenciasClient)svcDependencias).Close();
        return;
    }

    customChannelFactory.Close();
}

I don't know any Spanish so it was a bit difficult to understand exactly what was going on (I recommend you provide English translations/descriptions of non-English class names and string literals as comments next time).

Why do you create a temporary variable that is only use to set another variable on the very next line instead of just setting it directly? (iDependencias & depClient).
You close the channel if it's a DependenciasClient but don't clear the reference to it. svcDependencias is visible outside this method and having it hold a reference to a closed connection may be a problem.
You don't check to see if svcDependencias is already set before setting it to a new connection. I don't know if you can re-use the connection, but you should at least close the existing connection before you loose the reference to it.
I noticed that you only close the channel factory if the channel is not a DependanciasClient. While this will probably work ok if the channel factory never returns a DependanciasClient, I think a null check would be a clearer solution.

if(customChanelFactory != null)
{
    customChanelFactory.Close();
}

Instead of using the 'is' operator followed by a cast, you should use the 'as' operator and check for null.

DependenciasClient client = svcDependencias as DependenciasClient;

if (client != null)
{
    client.Close();
}

If svcDependencias only exists at the class level for the benefit of GenerarDependenciasDeFormulariosFmb then personally I would prefer to pass it in as an argument rather then use a class field.


Keeping a search history in localStorage, with a length limit I'm having a problem figuring out how I can make this particular part of my code DRY. Currently, the code is repeating itself but with minor changes in each case of a switch.

SearchParam is a string that the user has searched for in the application that this code is from. The function creates a new li element with searchParam as the text. This value is then added to localStorage as 'search1'. Dependent on how many li elements there are already, the function uses a switch to add the new li element to the beginning of the list. If there are already 3 items, then the last item in the list will be deleted and removed.

function searchHistory(searchParam) {
  var liElements = $('#history').children();
  var recentSearch = liElements.first();

  var newItem = `

${searchParam}

  `;

  switch($('#history').children().length) {
     case 0:
        $('#history').append(newItem);
        localStorage.setItem('search1', searchParam);
        break;

     case 1:
        var secondSearch = liElements.children()[1].innerHTML;

        recentSearch.before(newItem);
        localStorage.setItem('search1', searchParam);
        localStorage.setItem('search2', secondSearch);
        break;

     case 2:
        var secondSearch = liElements.children()[1].innerHTML;
        var thirdSearch = liElements.children()[3].innerHTML;

        recentSearch.before(newItem);
        localStorage.setItem('search1', searchParam);
        localStorage.setItem('search2', secondSearch);
        localStorage.setItem('search3', thirdSearch);
        break;

     default:
        var oldSearch = liElements.last();
        var secondSearch = liElements.children()[1].innerHTML;
        var thirdSearch = liElements.children()[3].innerHTML;

        localStorage.setItem('search1', searchParam);
        localStorage.setItem('search2', secondSearch);
        localStorage.setItem('search3', thirdSearch);

        recentSearch.before(newItem);
        oldSearch.remove();
  }
};

So if I understand the code correctly:

You're keeping track of search.
You're adding the search items to local storage.
You're keeping the history to a max of 3 items.
You're displaying them in the UI.

I would suggest to leave out the UI logic out of the storage logic. The storage logic should only deal with storage and retrieval from local storage. UI updates should be separate.

Next is to not depend on the UI for maintaining state. That's the job of your data. The UI should merely be a representation of the data. That means, you shouldn't be depending on innerHTML to keep that history.

The search history is simply an array which you push in stuff, and shift out stuff when it goes beyond 3. You do not need an item in the storage for each. Since localStorage only keeps strings, you'll need to use JSON.stringify and JSON.parse to serialize and deserialize the data, respectively.

I would split the functions, one for updating the stored data, and the other to update the UI based on the existing data.

function searchHistory(searchParam) {
  const maxHistoryLength = 3;
  const history = JSON.parse(localStorage.getItem('searchHistory') || '[]');
  const isHistoryMaxed = history.length === maxHistoryLength;
  const workingHistory = isHistoryMaxed ? history.slice(1) : history;
  const updatedHistory = workingHistory.concat(searchParam);

  localStorage.setItem('searchHistory', JSON.stringify(updatedHistory));
}

function updateSearchHistoryUi(){
  const history = JSON.parse(localStorage.getItem('searchHistory') || '[]');

  $('#history').empty().append(history.map(v => `

${v}

  `).join(''));
}


A S.I.N checker, using Luhn's Algorithm, and created in swing. I have created a program to validate a Canadian S.I.N. using Luhn's Algorithm, and was wondering if there was any way to make it more efficient. From what i can tell, it is already extremely efficient, but any tips or advice will be greatly appreciated.

import javax.swing.*;
import java.awt.*;
import java.awt.event.*;

class SinChecker extends JFrame
{ //naming variables
JTextField t1;
private JLabel j, j1, j2, j3;
ButtonListener bl1;
ButtonListener2 bl2;

public SinChecker ()
{ //Get the container
Container c = getContentPane ();

//Set absolute layout
c.setLayout (null);

//Set Background Color
c.setBackground (Color.WHITE);

//Creating label Guess my number text
JLabel j = new JLabel ("S.I.N. Validation Checker");
j.setForeground (Color.BLUE);
j.setFont (new Font ("tunga", Font.BOLD, 24));
j.setSize (270, 20);
j.setLocation (30, 35);

//Creating label Enter a number.....
j1 = new JLabel ("Enter your S.I.N. below.");
j1.setFont (new Font ("tunga", Font.PLAIN, 17));
j1.setSize (270, 20);
j1.setLocation (66, 60);

//Creating a label Instuctions
JLabel j2 = new JLabel ("Enter a 9-digit Social Insurance Number");
j2.setFont (new Font ("tunga", Font.PLAIN, 17));
j2.setSize (270, 20);
j2.setLocation (10, 165);

//Creating a label Instuctions
JLabel j3 = new JLabel ("with no spaces between the digits please.");
j3.setFont (new Font ("tunga", Font.PLAIN, 17));
j3.setSize (270, 20);
j3.setLocation (10, 180);

//Creating TextField for x input guess
t1 = new JTextField (10);
t1.setSize (70, 30);
t1.setLocation (100, 80);

//creating 2 buttons
JButton b1 = new JButton ("Proceed");
b1.setSize (120, 30);
b1.setLocation (70, 200);
bl1 = new ButtonListener ();
b1.addActionListener (bl1);

JButton b2 = new JButton ("Re-enter");
b2.setSize (120, 30);
b2.setLocation (70, 250);
bl2 = new ButtonListener2 ();
b2.addActionListener (bl2);

//Place the components in the pane
c.add (j);
c.add (j1);
c.add (j2);
c.add (j3);

c.add (t1);
c.add (b1);
c.add (b2);

//Set the title of the window
setTitle ("Social Insurance Number Checker");

//Set the size of the window and display it
setSize (300, 350);
setVisible (true);
setDefaultCloseOperation (EXIT_ON_CLOSE);
}

//implement  first action listener
private class ButtonListener implements ActionListener
{

public void actionPerformed (ActionEvent e)
{
    int a = Integer.parseInt (t1.getText ());
    boolean evenDigit = false;   //alternates between true and false
    int sum = 0; //accumulates the sum of the digits (as modified)

    while (a > 0)
    {
        int nextDigit = a % 10; //grab the last digit
        a = a / 10; //discard that digit
        if (evenDigit)
        {
            //double it, then add the two digits of the result
            nextDigit = 2 * nextDigit;
            nextDigit = (nextDigit / 10) + (nextDigit % 10);
        } // if(evenDigit)
        sum = sum + nextDigit;
        evenDigit = !evenDigit; //toggle the flag each time
    } // end while

    if (0 == sum % 10)
    {
        j1.setText ("That is a valid S.I.N.");
    }
    else
    {
        j1.setText ("That is not a valid S.I.N.");

    }
    t1.requestFocus ();
    t1.selectAll ();
}
   }

 private class ButtonListener2 implements ActionListener
 {
   public void actionPerformed (ActionEvent e)
   {
    //reset the label messages
    t1.setText ("");
    j1.setText ("Enter your S.I.N. below.");

    t1.requestFocus ();
    t1.selectAll ();

}
}

public static void main (String[] args)
{
  new SinChecker ();
 } }

User interface

For portability, you should use a layout manager, not absolutely positioned elements.  Here's what it looks like on OS X:

I don't see a reason for two sets of instructions; the instructional text should be placed together.

"Validation Checker" is redundant.  It's either a "SIN validator" or "SIN checker".

"Valid SIN" could be construed as meaning a number that has been actually been issued by Service Canada to a person.  Perhaps you should clarify that it's merely a "possibly valid SIN".

Why prohibit spaces?  Canadian Social Insurance Numbers are conventionally rendered as three groups of three digits, with spaces or hyphens between groups.  Your program could easily be made more lenient and human-friendly, accepting and ignoring the superfluous punctuation.

To facilitate data entry, you should allow number keypad-only usage and perform the validation when Enter is pressed.  (Currently, you have to press Tab and Space, or click Proceed.)  For that matter, you could automatically perform the validation as soon as nine digits have been entered, without even requiring Enter.

Automatically returning focus to the number field and selecting its contents after validating was a nice touch.

Validation quality

"0" is accepted as a valid SIN.

"-9" is accepted as a valid SIN.

"123456789012345" or "hello" cause a NumberFormatException which is invisible in the UI.  It just looks like either a valid SIN or an invalid SIN, depending on whether the previous validation passed or failed.

Implementation

You should implement the Luhn algorithm check in its own function (if not its own class) rather than in the event handler.  That lets you reuse the code in other contexts such as a text-based UI, a tax-preparation program, or a unit test.

These lines of code are not as clear as they could be:

bl1 = new ButtonListener ();
b1.addActionListener (bl1);

Rather, I would prefer to see the button and text field identified with descriptive variable names.  The ActionListener should be simplified and could be made into an anonymous inner class:

proceedButton.addActionListener(new ActionListener() {
    public void actionPerformed(ActionEvent e) {
        boolean valid = SinChecker.isValid(sinTextField.getText());
        SinChecker.this.announceValid(valid);
        sinTextField.requestFocus();
        sinTextField.selectAll();
    }
});

Since your SinChecker class extends JFrame, I would expect its constructor to behave a specialized JFrame.  In particular, a JFrame constructor would not make the newly instantiated frame visible, nor would it set exit-on-close.  Therefore, your SinChecker constructor shouldn't do those things, either.  If your SinChecker is incorporated as a component within some larger program, exit-on-close might not be appropriate.

Speaking of reusability, consider making your SinChecker a JPanel instead of a JFrame, so that you have the flexibility to use it as a widget that is not its own window.  In that case, your widget should offer a getSin() method that returns a guaranteed valid SIN.

In accordance with the documentation, Swing code must run in the Event Dispatch Thread, not the main thread.

public class SinChecker extends JPanel {
    public SinChecker() {
        this.setBackground(Color.WHITE);
        …
        this.setPreferredSize(…);  // Ideally automatically determined by layout manager
    }

    …

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                JFrame f = new JFrame("Social Insurance Number Checker");
                f.setLayout(new BorderLayout());
                f.add(new SinChecker());
                f.pack();
                f.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                f.setVisible(true);
            }
        });
    }
}


Better to use string concatenation using std::string::operator+() or std::string::reserve() and std::string::append()? I'm doing some code review and I saw something like the following:

std::ifstream dataFile(filename);
if (dataFile.is_open())
{
    std::deque<std::string> lines;
    std::string line, line_batch;
    while (std::getline(dataFile, line))
    {
        if (!line.empty())
            lines.push_back(line);
    }
    dataFile.close();

    for (size_t i = 0; i < lines.size();)
    {
        if (i < MAX_BATCH_SIZE)
        {
            line_batch += "{" + lines.at(i) + "},";
            i++;
        }

I'm thinking that having many small strings (1000 lines equating to around 3000 strings) that will be concatenated together, that it would be beneficial to calculate the final string size ahead of time, reserve it and then do a concatenation using string::append as this would reduce fragmentation, reallocation and increase speed (the last one I don't think would be that big of a deal).

A read in line would, at a guess, be between 10-1000 characters.  Max size of line_batch should be under 20MB. The application can get pretty big during execution.  Big enough to force us to move from a 32 to a 64 bit memory scheme.  The amount of times that this will happen depends on the user's environment, but if I were to guess, maybe between 0 and 100 times in a session.  Could be more.

I'm a little on the fence on this.  Am I being too nit picky?  Is the added complexity worth it?  If it matters, we're compiling with VS2019, though I don't think we're using it as a fully compliant c++17 compiler.

This question is a bit too general. And I believe you are nitpicking but not for the reason you mentioned.

In the code it first reads all 1000+ lines and stores them into a deque. Each line is a potential allocation and deque also allocates linearly in the number of objects (one allocation for each 4k bytes and each strings takes 32 bytes... surely these numbers depend on platform and implementation and it works differently for large objects). Wouldn't that be a much much bigger source of memory fragmentation than just resizing 10 times one poor big string? IIRC string's capacity is in increased via the same policy as vectors, i.e., exponentially - thus it is just a few resizes.

I find the issue of storing the string as separate entities to be a much bigger problem than the big string. To deal with it, one needs to read each line and process it immediately (put it into the batchline and hopefully getline will reuse reserved data). At least I hope its enough. In this case one cannot figure out the final size of the string batch without reading the file twice... and nobody wants that. So you can either increase its size by appending or reserve at the start its maximal size and add elements till it hits the capacity and then forward it. Ofc, there is a small nuance of dealing with extra large strings bigger than the reserved size - but it is never too hard to fix. Just make sure you deal with it.

Disclaimer: I don't fully understand what brings more problems to memory fragmentation - it might be that deleting those strings at the end fixes most problems but if it so then why worry about the batch_string?


Creating a database, a table within the database and inserting some values into it in one go I've written a script which creates a new database, a table within the database, insert some values into it and fetch the values in one go. The way I've created the script below to achieve what I just said seems not to be an ideal way as there are too many repetitions in there. To be specific this two functions create_database() and connect() are almost identical. Moreover, I had to use mycursor = conn.cursor() twice within main function.
import mysql.connector

def create_database():
    mydb = mysql.connector.connect(
      host="localhost",
      user="root",
      passwd = "",
      database=""
    )
    return mydb

def connect(databasename):
    mydb = mysql.connector.connect(
      host="localhost",
      user="root",
      passwd = "",
      database=databasename
    )
    return mydb

def store_data(item_name,item_link):
    mycursor.execute("INSERT INTO webdata (item_name,item_link) VALUES (%s,%s)", (item_name,item_link))
    conn.commit()

if __name__ == '__main__':
    db_name = "newdatabase"
    conn = create_database()
    mycursor = conn.cursor()
    try:
        mycursor.execute(f"CREATE DATABASE {db_name}")
    except mysql.connector.errors.DatabaseError:
        pass
    conn = connect(db_name)
    mycursor = conn.cursor()
    mycursor.execute("DROP TABLE if exists webdata")
    mycursor.execute("CREATE TABLE if not exists webdata (item_name VARCHAR(255), item_link VARCHAR(255))")
    store_data("all questions","https://stackoverflow.com/questions/tagged/web-scraping")
    mycursor.execute("SELECT * FROM webdata")
    for item in mycursor.fetchall():
        print(item)

What it prints (expected result):
('all questions', 'https://stackoverflow.com/questions/tagged/web-scraping')

From the title alone, this sounds like something that should not be done, particularly since this is not a lightweight database such as SQLite. Creating the database, its tables, columns, constraints etc. is not, and should not, be the job of the application - but rather an external setup script with administrative permissions. The application, then, should not have such permissions and should only be able to insert/select/update/delete.
For some reason I thought autocommit is enabled by default for the MySQL Python connector, but apparently it's not:
https://dev.mysql.com/doc/connector-python/en/connector-python-connectargs.html
So your explicit call to commit is fine.
I can't tell from the documentation whether the cursor and connection objects support context management. Try putting both of those in with statements, and if it complains that there's no __enter__, switch to a try/finally that guarantees cursor and connection closure.


Return a tuple from MySQL database with statements I have a database from which I'd like to return a tuple of two elements (ID and URL) with check digit to see if operation was successful.

Is correct this code to return this tuple and control?

public function getManufacturer($id) {
    $stmtManufacturer = $this->dbConnection->prepare("SELECT ID, URL FROM ERP.MANUFACTURER WHERE ID = ?");
    $stmtManufacturer->bind_param("i", $id);
    $stmtManufacturer->execute();
    $stmtManufacturer->store_result();
    $stmtManufacturer->bind_result($id, $url);
    $stmtManufacturer->fetch();
    $stmtManufacturer->close();
    return [
                ACTION_RESULT => $this->processServerResponse($stmtManufacturer),
                "data" =>
                [
                    "id" => $id,
                    "url" => $url
                ]
            ];
}

ACTION_RESULT is defined as a constant in the class.

I think function processServerResponse() could be improved:

public function processServerResponse($stmt) {
    if ($stmt->errno || $stmt->affected_rows === 0) {
        $_actionResult = 0;
    } else if ($stmt->affected_rows !== 0) {
        $_actionResult = 1;
    }
    return $_actionResult;
}

The whole processServerResponse() method looks rather alien to me. I never seen anything like that. A server response shouldn't be so much tightly coupled with such a specific stuff like a statement class for one of PHP extensions. A server  response should be from a server, not a statement.

Besides, you are mixing two response codes here, "error occurred" and "zero results found". It should be different codes.

Usually, errors are checked by a distinct error handler, while number of results found can be checked within the same routine that returns the data.

So I would make it this way

public function getManufacturer($id) {
    $stmt = $this->dbConnection->prepare("SELECT ID, URL FROM ERP.MANUFACTURER WHERE ID = ?");
    $stmt->bind_param("i", $id);
    $stmt->execute();
    $stmt->store_result();
    $stmt->bind_result($id, $url);
    $found = $stmt->fetch();
    return [
                ACTION_RESULT => (int)$found,
                "data" =>
                [
                    "id" => $id,
                    "url" => $url
                ]
            ];
}

while errors should be checked with a site-wide handler like this (taken from my article on writing Db wrappers)

set_error_handler("myErrorHandler");
function myErrorHandler($errno, $errstr, $errfile, $errline)
{
    error_log("[$errno] $errstr in $errfile:$errline");
    header('HTTP/1.1 500 Internal Server Error', TRUE, 500);
    header('Content-Type: application/json');
    echo json_encode(["error" => 'Server error']);
    exit;
}

So this way you will get different responses for the different scenarios.

On a side note, an obligatory suggestion: consider to use PDO instead of mysqli. This alone will do a huge refactoring for your code:

public function getManufacturer($id) {
    $stmt = $this->pdo->prepare("SELECT ID id, URL url FROM ERP.MANUFACTURER WHERE ID = ?");
    $stmt->execute([$id]);
    $data = $stmt->fetch();
    return [
                ACTION_RESULT => (int)(bool)$data,
                "data" => $data;
            ];
}


Object for generating HTML for hyperlinks I have a function that generates HTML for hyperlinks. It's arguments may be string literals or variables read from the database. An example call to this function looks like:

hLink('http://example.com', 'website', 'section', 'input=0', '_blank');

which returns:

'website'

The first argument is required, and the remaining four have defaults.

In my first foray into object-oriented programming, I've written a replacement for this function by a method defined in a class, so the function calls are replaced with calls to that method acting on an object from that class. I've written two versions:

In the first version, each link works by creating a new instance of the class, with the required arguments of hLink() handled by the class constructor, and optional arguments handled by setting properties of the object, and then an output method hLink() finally acting on those properties to generate the HTML link. If you want to see the code for this version, it's in the edit history of this question, but I've removed it in favor of the second version, which I think is probably better.
In the second version, the class has no constructor. One instance of the class is created and all links are generated by this one object. The class has a method clear() that resets all the properties to empty, as well as additional methods to set the values of any properties that are needed, and finally an output method hLink() to generate the HTML link.

I wrote the second version because it seemed that the first was wasteful by creating an object for each link, and those objects were never used again after generating the HTML. So the second version reuses the same object repeatedly for this task.

The code for the second version is as follows: (Pardon my formatting. I know it's unconventional, but I find this easier to read.)

<?php declare(strict_types = 1);
$oLink = new link;
class link
    {private $sURL;
     private $sText;
     private $sAnchor;
     private $sQuery;
     private $sWindow;
     public function clear()
        {$this->sURL    = '';
         $this->sText   = '';
         $this->sAnchor = '';
         $this->sQuery  = '';
         $this->sWindow = '';
         return $this;
         }
     public function URL(string $sURL)       {$this->sURL    = $sURL;    return $this;}
     public function text(string $sText)     {$this->sText   = $sText;   return $this;}
     public function anchor(string $sAnchor) {$this->sAnchor = $sAnchor; return $this;}
     public function query(string $sQuery)   {$this->sQuery  = $sQuery;  return $this;}
     public function window(string $sWindow) {$this->sWindow = $sWindow; return $this;}
     public function hLink()
        {return('sURL .
                              ($this->sQuery  == '' ? '' : '?' . $this->sQuery) .
                              ($this->sAnchor == '' ? '' : '#' . $this->sAnchor) .
                        '"' . ($this->sWindow == '' ? '' : ' target="' . $this->sWindow . '"') .
                 '>' . $this->sText . '');
         }
     }
echo('
Link to ' . $oLink->clear()->URL('http://example.com')->text('website')->anchor('section')->query('input=0')->window('_blank')->hLink() . ' in text
');

I've tested the above code as a freestanding script, and it does output the intended:

Link to website in text

I'd like to know if I'm doing this right, or even if I should be doing this at all (i.e., if this is a sensible use of objects). Is this the best way to generate HTML for hyperlinks embedded in text in PHP?

For the suggested use case having an object is an overkill.

As far as I can tell from the code provided, every link is generated only once, which makes class variables; a dedicated method to clean the state; and a code to instantiate an object all unnecessary. So a function would be more appropriate here.

Besides, as it seems that a code is more intended to create an HTML <a> tag dynamically than to create an URL, and also in order to follow the Single responsibility principle, I would change the list of parameters to just

hLink('http://example.com/?input=0#section', 'website', '_blank');

which will make parameters more sensible and consistent.


C++ enums - which one is better? I have a large namespace, but I don't want enum constants to be a part of it. In cases where an enum is related with a class, I coded enums inside the class so I can reach it using the class name. However, if an enum is related with namespace functions, what is the best way to do it?

Use another namespace and put the enum inside, so from the outside it will be the same as others with different implementations?
Continue and create a class with private constructors so it will be as same as others?

Classes:

class A {
public:
   enum Type {
      Aa,
      Ab
   };

   Type GetType();
};

class B {
public:
   enum Type {
      Ba,
      Bb
   };

private:
   B();
};

B::Type DoSomething();

Namespace:

namespace B {
   enum Type {
       Ba,
      Bb
   };
}

B::Type DoSomething();

The first case seems to abuse classes, but it can be extended later to use same coding method as the others. I am in favor of namespace, but what are your opinions?

Do not use "enum in class" for public usage. You should use namespace for the purpose. (If your compiler supports C++0x, then use enum class.)

The problem with "enum in wrapper class" is header dependency. When declaration code wants to use some "enum in wrapper class" type, then it should also references the whole class information. Because you cannot physically separate enumeration from the wrapper class (one definition rule), it'll introduces heavier header dependency.

Moreover, because in C++, there can't be two separate definitions for one class so you cannot use forward declaration technique with "enum in wrapper class" like below.

// a.h
namespace A {
  enum Type {
    a,
    b
  };
}

// b.h
namespace A {
  enum Type; // forward declaration
}
A::Type function();


Better writing JS - from general variables to object This might be a dumb question for you advanced developers.

Below is a script on my own website that I want to modify, in order to avoid declaring same variables in each function. I want to avoid declaring variables in the main script structure.

Below code works, still wanted to know if there is a more elegant way to do this.

As Answered below, it's best to enclose everything in an object.

var var1 = document.getElementById("div1");
var var2 = document.getElementById("div2");

function foo1(){
div1.style.display = "none";
}

function foo2(){
div2.style.display = "block";
}

You can enclose your variables (and functions) in a closure to restrict your scope, although you then need someway to make those functions callable outside of that closure, e.g.:

(function() {
    var a;

    function increment() {
        a += 1;
    }

    function decrement() {
        a -= 1;
    }

    window.increment = increment;
    window.decrement = decrement;
})();

It's pretty ugly, though.  It would be better to enclose the whole thing as an object, e.g.:

function Counter() {
    var count = 0;

    return {
        increment: function() {
            return count++;
        },
        decrement: function() {
            return count--;
        },
        value: function() {
            return count;
        }
    };
};


Binary tree implementation in Scala This is my implementation of a binary tree in Scala. How can this be made better?

abstract class BinaryTreeNode[+A]

case class MyBinaryTreeNode[A](var data: A,var left: BinaryTreeNode[A],var right: BinaryTreeNode[A]) (implicit ordering: Ordering[A]) extends BinaryTreeNode[A] {

    override def toString = data + " " + left.toString + " " + right.toString
}

case object MyNilNode extends BinaryTreeNode[Nothing]

class MyBinaryTree[A](var head: BinaryTreeNode[A]) {

    def isBST(implicit order: Ordering[A]): Boolean = {
        this.isBSTHelper(head)._1
    }

    def isBSTHelper(node: BinaryTreeNode[A])(implicit order: Ordering[A]): (Boolean, A, A) = {
        node match {
            case MyNilNode =>
                return (true, null.asInstanceOf[A], null.asInstanceOf[A])
            case MyBinaryTreeNode(data, lNode, rNode) =>
                if(lNode == MyNilNode && rNode == MyNilNode) {
                    return (true, data, data)
                }

                val leftResult = this.isBSTHelper(lNode)(order)
                val rightResult = this.isBSTHelper(rNode)(order)
                return (leftResult._1 && rightResult._1 && data >= leftResult._3 && data <= rightResult._2,
                        leftResult._2,
                        rightResult._3)
        }
    }
}

You haven't actually implemented a binary tree.  But I'll make some points about your existing code before explaining what is missing.

That's not a node, it's a tree

Your base class is a node when it should be a tree
Wrapping the nodes in a tree class which knows nothing other than it contains nodes is artificial and obstructive

Like many algebraic data types, a binary tree is a recursive type.  A node is a tree (one which can hold other trees).  Nil/Empty is a tree.

Instead of working with that, you have an inflexible wrapper class Tree and a set of case classes which implements nodes.  That incurs many penalties.  For a start, you have no representation in your type system of the empty tree.  Secondly, you lose the ability to create simple, elegantly recursive methods for your case classes.

Look how much simpler your code can be if you work with that, rather than fight it:

abstract class BinaryTree[+A] {
  def isEmpty: Boolean
  def isValid: Boolean
}

case object EmptyTree extends BinaryTree[Nothing] {
  def isEmpty = true
  def isValid = true
}

case class NonEmptyTree[A](
    var data: A,
    var left: BinaryTree[A],
    var right: BinaryTree[A])
    (implicit ord: Ordering[A]) extends BinaryTree[A] {
  def isEmpty = false
  def isValid: Boolean = {
    import ord._
    def isValidWith(f: A => Boolean, t: BinaryTree[A]): Boolean = t match {
      case NonEmptyTree(that, _, _) => f(that) && t.isValid
      case EmptyTree => true
    }
    isValidWith(data < _, left) && isValidWith(data > _, right)
  }
}

See how this works with the actual class hierarchy to build comprehensive tree functions simply and recursively? It should be fairly obvious how you could add depth or size methods.

You don't need that wrapper class.  What I do recommend is

Make the abstract class sealed (improves pattern matching performance and keeps the type sound).
Create a BinaryTree companion object containing an apply factory method for creating trees.
Make the constructors for BinaryTree and EmptyTree private (if the user can create them directly, they might create invalid trees).
Make the class immutable.  Turn each var into a val.

You may balk at that last point but modifying the data held at any one node is a dangerous and pointless thing to do.  The way binary trees work, it doesn't even make sense.  When adding a value, the only places you would want to change left or right is at the node where you insert the value in place of an empty tree (possibly having to move the left branch to the right branch, to preserve ordering).  Adding a value to an immutable tree is only marginally more complicated than to a mutable one and much safer.  Also, you regain the option of variance (if you want it, although I recommend against).

My final recommendation: throw away that isBST/isValid method.  It's pointless...

That's not an implementation

Nothing in your code actually implements a binary tree. All you've done is provide a shell into which data has to be manually inserted, with a validation method to check whether the manually-constructed tree looks like a binary tree.

But the rules for adding values to an ordered binary tree are simple (OK, more complex if it's a balanced tree). Your type should know how to do that. If you think about it, it shouldn't be hard to create an add method along the same lines as isValid.  I'll start it off for you

sealed abstract class BinaryTree[+A] {
  def isEmpty: Boolean
  def isValid: Boolean
  def add(a: A): BinaryTree[A]
}

case object EmptyTree extends BinaryTree[Nothing] {
  def isEmpty = true
  def isValid = true
  def add[A](a: A): BinaryTree[A] = NonEmptyTree(a, EmptyTree, EmptyTree)
}

case class NonEmptyTree[A](
// Fill in the rest here...

Assuming a non-balanced tree, for NonEmptyTree's version of add, you only need to consider

How would I add a value to a node where left and right are empty?
How would I add a value to a node where right is empty?
If neither left nor right is empty, on which of those do I call the add method?

Once you have implemented add you can throw away isBST because your tree can't be created invalidly (Well, rather than throw it away entirely, move it into your tests).

Other methods you need to consider implementing:

contains
size
depth (vital for balanced trees)
delete (rather trickier than add)
toList
toSet/toSortedSet
toString (OK, I know you've done one but it fails if, for example, type A is a string and could contain spaces).

If you add those, you can also make EmptyTree and NonEmptyTree private, further protecting the soundness of your module.


Highest number in a list with Racket I am trying to build a function to find the highest number in a list without using the built-in functions of "apply" and "max".

I created the following code:

(define (highest-num lst)
  (define (iter lst accu)
    (cond ((null? lst) accu)
          ((< accu (first lst)) (iter (rest lst) (+ accu (- (first lst) accu))))
          (else (iter (rest lst) accu))))
  (iter lst 0))

However, I bet this could be better. Any suggestions?

Remarks

Using an internal function, recursion, and a trampoline is a good way to structure recursive procedures on a lists.
The code fragment (+ accu (- (first lst) accu))) is the same as (first lst).
The name accu suggests an accumulator, however, the value it stores is the maximum, so max might be better.
Because of the letter l can be confused with 1, lst can be read as slang for 'first'. I prefer xs as the name for a list of x's. The nomenclature has become more common in Lisps over the past few decades due to the influence of other functional languages.
No matter how it is dressed up, there's a max procedure in the code somewhere. And it has to be written. It is better to be explicit about it than to try to hide max by embedding it in some other function.

Alternative

For procedures that try to find a property of a list, folding is often a good place to start. This code uses foldl. Usually I find foldl rather than foldr is what I want, for whatever that's worth.

#lang racket
(define (highest-number xs)
  (define (max x1 x2)
    (if (> x1 x2) x1 x2))
  (foldl max (first xs) (rest xs)))

Maybe it's worth pointing out, that the starting point for writing the highest-number was wishful thinking where I wished I had max. Then at the next lower level of abstraction down, I wrote it. Credit goes to Ableson and Sussman's SICP lectures.


Replacing consecutive equal elements in a list with a new element In the code below I will replace all consecutive equal elements in a list with a new element. In this case I have chosen the new element to simply be the concatenation of all the equal elements, but I want the code to remain adaptable to other situations.

I have the following example:

  Input: ["a", "a", "b", "c", "d", "d", "d", "e", "f", "f", "f", "f", "g", "g"]
  Output: ["aa", "b", "c", "ddd", "e", "ffff", "gg"]

I furthermore want to perform this replacement in a single-pass, meaning in \$\mathcal{O}(n)\$ time complexity and also with \$\mathcal{O}(1)\$ storage complexity.

In the code below I have implemented a solution to this problem, I believe it still satisfies \$\mathcal{O}(n)\$ but I'm not exactly sure because it also iterates back through the list to remove the elements.

private static void replaceConsecutiveEqualElements(final List<String> list) {
    ListIterator<String> listIterator = list.listIterator();
    String previousElement = null;
    int consecutiveMatches = 1;

    // Loop through list
    while (listIterator.hasNext()) {
        String element = listIterator.next();
        String newElement = null;

        if (element.equals(previousElement)) {
            consecutiveMatches++;
        }
        else {
            // Replace consecutive elements with new element
            if (consecutiveMatches >= 2) {
                listIterator.previous();
                for (int i = 0; i < consecutiveMatches; i++) {
                    listIterator.previous();
                    listIterator.remove();
                }
                newElement = String.join("", Collections.nCopies(consecutiveMatches, previousElement));
                listIterator.add(newElement);
            }
            consecutiveMatches = 1;
        }

        previousElement = (newElement == null) ? element : newElement;
    }

    // Special case for last element
    // Replace consecutive elements with new element
    if (consecutiveMatches >= 2) {
        for (int i = 0; i < consecutiveMatches; i++) {
            listIterator.previous();
            listIterator.remove();
        }
        String newElement = String.join("", Collections.nCopies(consecutiveMatches, previousElement));
        listIterator.add(newElement);
    }
}

Called like:

List<String> data = new ArrayList<>(Arrays.asList("a", "a", "b", "c", "d", "d", "d", "e", "f", "f", "f", "f", "g", "g"));
replaceConsecutiveEqualElements(data);

I'm not happy at all with this code and am looking for feedback.

I have also decided to implement this in a functional programming language, in my case Clojure, though I have removed the time and storage complexity constraints.

(defn replace-consecutive-equal [seq]
  (->> seq
       (partition-by identity)
       (map (partial apply str))))

(println (replace-consecutive-equal '("a", "a", "b", "c", "d", "d", "d", "e", "f", "f", "f", "f", "g", "g")))

The Clojure code roughly performs the following steps:

Take the input: ["a", "a", "b", "c", "d", "d", "d", "e", "f", "f", "f", "f", "g", "g"]
Partition it by the identify function: [["a", "a"], ["b"], ["c"], ["d", "d", "d"], ["e"], ["f", "f", "f", "f"], ["g", "g"]]
Apply the str function on the elements of the inner lists via a map on the outer list, the str function effectively joins strings: ["aa", "b", "c", "ddd", "e", "ffff", "gg"]

As you can see this would have made the implementation a whole lot easier, unfortunately this is not possible due to the listed constraints.

Note: I am aware that this may be a case of premature optimization, but I feel that it is important to know how to implement such an algorithm efficiently.

You can remove the elements directly while iterating to avoid the back-iterating which would remove most of the method body. The add-call can be combined with one of the remove-calls to allow using set instead.

To reduce the amount of data to copy in case that your list is an ArrayList, the iterator should iterate the list backwards (assuming that you don't want to have a separated version for random-access lists):

private static void replaceConsecutiveEqualElements(
        final List<String> list) {
    ////
    for (final ListIterator<String> it = list.listIterator(list.size()); it.hasPrevious();) {
        final String sentinel = it.previous();
        int c = 0;
        for (; it.hasPrevious(); c++, it.remove()) {
            if (!it.previous().equals(sentinel)) {
                it.next();
                break;
            }
        }
        if (c != 0) {
            it.next();
            it.set(String.join("", Collections.nCopies(c + 1, sentinel)));
            it.previous();
        }
    }
}


Printing a directory's contents in a loop I wrote a bash script to print contents of files in a given directory in a loop.

How can I improve command line parameters handling to the script? I feel the parameter processing as of now uses too much code and it is quite clumsy. I think I had to write too much code to validate the the option arguments are numeric.

The script takes either -c numeric_choice_id and/or -l time_in_seconds and keep printing the contents of the files.

only -c number: it cats the file and ends
only -l number: repeatedly cats all the files
-c num1 and -l num2: repeatedly cats that particular file every num2 seconds

#!/bin/bash

        SYSPATH="$PWD/"

        FILES="$PWD/file1
        $PWD/file2
        $PWD/file3"

        count=0
        for i in $FILES
        do
            (( count++ ))
            menu="${menu}\n${count}. `basename $i`"
            list="${list} `basename $i`"
        done

testNumeric()
{
        if ! echo "$1" | grep -q '^[0-9]\+$'
        then
                echo "$0 Non numeric argument supplied to choice or time"
                exit
        fi

}

usage()
{
        echo -e "`basename $0` -c numeric_choice [ -l  time in seconds ]\n
-c      numeric id for the choice
-l      print o/p every time seconds
-h      print this menu

choice could be: \n$menu"

        exit
}

# Test if any arguments have been provided
if [ "x$1" = "x" ] || [ "$1" = "-h" ]
then

        usage
fi

# Allow getopt to parse the options and report error if needed
args=`getopt "c:l:" "$@"`

if [ "$?" -ne "0" ]
then
        usage
fi

# Set positional parameters
set -- $args

for i;
do
  case "$i" in
    -c  )
          choice=$2
          testNumeric $choice
          shift
          shift ;;

    -l  ) repeatTime=$2
          testNumeric "$repeatTime"
          shift
          shift  ;;

    --  )
          break ;;
  esac
done

[ "x$choice" = "x" ] && choice=`seq $count`
[ "x$repeatTime" = "x" ] && repeatTime=0

set -- $list

while [ true ]
do

        if [ "$repeatTime" = "0" ]
        then
                for a in $choice
                do
                eval temp=\${${a}}
                        cat $SYSPATH${temp}
                done

                break;
        else
                for a in $choice
                do
                eval temp=\${${a}}
                        cat $SYSPATH${temp}
                done

                sleep $repeatTime
        fi

done

Storing a list of file names and iterating over it

FILES="$PWD/file1
$PWD/file2
$PWD/file3"

So you're setting FILES to a newline-separated list of files. That's a bad idea because your script will choke if a file name contains a newline. In some circumstances, where you control the file names, that can be ok, but don't do it unless you really have no better way. Since you're writing a bash script, you never need to do this. Store the file names in an array instead:

FILES=("$PWD/file1" "$PWD/file2" "$PWD/file3")

Then, to iterate over the array:

for i in "${FILES[@]}"; do …

If you had a shell without arrays and decided to use a newline-separated list of file names, then you would need some extra effort to split the lines at newlines only. The IFS variable specifies where to split words in unquoted variable substitutions, so you need to set it to contain only a newline (the default value also contains the ordinary space character and the tab character). Also, the shell performs globbing (filename generation from wildcards) on the results of unquoted variable substitutions, so you need to turn that feature off. Here's a POSIX-compliant way of iterating over the pieces of a newline-delimited string:

IFS='
'
set -g
for i in $FILES; do …

Since you never change directories, you could store just the files' base names instead of full paths. If you control the file names, you then don't need to be so careful with special characters in the names.

Always use double quotes around variable substitutions

I see a lot of places where you write unquoted variable substitutions, e.g. basename $i. Do you really want to split $i into words, then perform globbing on the result? If you don't, then use double quotes: "$i". Only leave off the double quotes if you intend to perform word splitting and filename generation (as in the for loop example above), and then be mindful of the assumptions you're making on the value of the variable.

On the same note, if there was a risk that $i began with a -, you would need to call basename -- "$i" to avoid $i being interpreted by basename as an option. This is not going to happen with your script since $i is an absolute path, beginning with /.

I also recommend not using the backquote command susbsitution, which behaves strangely with nested quotes sometimes. Use $(…), which has an intuitive syntax.

    menu="${menu}\n${count}. $(basename -- "$i")"

Incidentally, note that this puts the characters \n into menu, not a newline. The \n is expanded by echo -e below.

Testing for a number

        if ! echo "$1" | grep -q '^[0-9]\+$'

No need to call grep for that! Here's a simple bash construct:

        if [[ $1 = *[^0-9]* ]]; then … # non-numeric

Here's a POSIX construct:

        case $1 in
          *[!0-9]*) …;; # non-numeric
        esac

Here documents

To insert a multiline string in a script, use a here document. It's more readable.

usage()
{
     cat <<EOF
$(basename -- "$0") -c ID [-l TIME]
-c ID     numeric id for the choice
-l TIME   print output every TIME seconds
-h        print this menu

ID could be:
$menu
EOF
}

It's conventional to use upper case for argument descriptions in the symbolic syntax. Clearly indicate the options that take an argument in the left-hand column. Don't use obscure abbreviations that barely save a few characters like o/p (I think you meant “output”, but I'm not sure).

Parsing the command line

Use the shell builtin getopts rather than the external command getopt. If you have bash, you have getopts. If you don't have getopts, then you're running an embedded shell which may lack other features, so check that the other features you want to use are supported.

To test if there are any arguments, check the number of arguments, in $#.

if [ $# -eq 0 ] || [ "$1" = "-h"]; then …

Actually, you don't need any special handling for -h. Treat it like any other option.

I'm not going to go over getopts usage, there are plenty of examples around.

Before the main loop

set -- $args
…
set -- $list

Remember what I wrote about unquoted substitutions above? Each of these commands performs word splitting and filename generation.

[ "x$choice" = "x" ] && choice=`seq $count`
[ "x$repeatTime" = "x" ] && repeatTime=0

You need to initialize choice and repeatTime before the argument parsing, unless you intended to allow them to be pulled from the environment.

The main loop

while [ true ]

This is a strange way of writing while true. You could also write while [ false ] or while ! [ ! true ]. Keep it simple.

                eval temp=\${${a}}
                cat $SYSPATH${temp}

In bash, you can avoid the use of eval. Also, remember to double quote variable substitutions unless you have a good reason not to.

                cat "$SYSPATH${!a}"


Returning items that meet a minimum value I'm trying to find an alternative way to write this code such as using different ways to achieve checking if it's a subset and insertion.

def candidates(table, candidates, minsupp):
    count, supported = {}, {}
    for item in table:
        for c in candidates:
            if c.issubset(item):
                count[c] =  count.get(c, 0)
                count[c] = count[c] + 1

    total = float(len(table))
    l_sub_one = []
    for c in count:
        support = count[c] / total
        if support >= minsupp:
            l_sub_one.insert(0, c)
        supported[c] = support
    return l_sub_one, supported

As @Alan suggested in his comment, this method is doing 2 different things that can be separated into 2 methods.

This looks strange:

            count[c] =  count.get(c, 0)
            count[c] = count[c] + 1

Why not:

            count[c] = count.get(c, 0) + 1


"Property Container" design-pattern I've tried to write my Property Container design-pattern implementation.

Could anybody, please, tell me, if this code is really what I intended to write (follows the Property Container design-pattern rules)? Is there anything that can be improved?

<?php

class PropertyContainer
{
    private $PropertyContainer = array();

    public function __construct() { }

    public function addProperty($k, $v)
    {
        for($i = 0; $i < count($this->PropertyContainer); $i++) { }
        $this->PropertyContainer[$k] = $v;
    }

    public function setProperty($k, $v)
    {
        while($this->PropertyContainer)
        {
            if(key($this->PropertyContainer) == $k) {
                $this->PropertyContainer[$k] = $v;
                return;
            }
            next($this->PropertyContainer);
        }
        echo "Key was not found";
    }

    public function getProperty($k)
    {
        //var_dump($this->PropertyContainer);
        foreach($this->PropertyContainer as $key => $val)
        {
            if($key == $k) { return $val; }
        }
        echo "Key was not found";
        return;
    }
}

$pc1 = new PropertyContainer();
$pc1->addProperty("myProperty1", 31);
$pc1->addProperty("myProperty2", 32);
$pc1->addProperty("myProperty3", 33);
$pc1->setProperty("myProperty2", 7);
echo $pc1->getProperty("myProperty1") . "
";
echo $pc1->getProperty("myProperty2") . "
";
echo $pc1->getProperty("myProperty3") . "
";

echo "
";

$pc2 = new PropertyContainer();
$pc2->addProperty("myProp1", 11);
$pc2->addProperty("myProp2", 11);
$pc2->addProperty("myProperty3", "Some String");
$pc2->setProperty("myProp2", 12);
echo $pc2->getProperty("myProp1") . "
";
echo $pc2->getProperty("myProp2") . "
";
echo $pc2->getProperty("myProperty3") . "
";

echo "
" . $pc2->getProperty("myProperty5") . "
";

?>

Yes, you are using this patter correctly here. Its use in PHP (especially as a generic implementation) is greatly reduced though. As the answers in your programmers.stackexchange.com point out: basically it is a hashmap. Now, PHP implements arrays as some form of hash map already. I see this pattern as an anti-pattern too. There are some use-cases of this pattern when you register validators along each property this property has to fullfil when setting.

The php class stdClass basically does what you want to achieve it a more direct way:

$object = new stdClass();
$object->key = $value;

Though I'd recommend not to use this code in production, it is a good way to learn of course.

So my review for your code:

$PropertyContainer: It is uncommon in PHP to have variables start in upper case. Most code I do read either starts in lower-case and follow Camel-Case or is snake casing.
public function __construct() { } just fills up space. Not required, therefore remove it.
You should have a closer look at how to work with arrays in PHP. Your current approach is highly inefficient. Many of the array functions do not relay on the array iterator's state. Currently you force PHP to iterate over the array. If you use dedicated methods instead, PHP doesn't have to iterate (which is much faster of course :))

To add an entry to your array there is no need to iterate over it without doing it first. You can either array_push it at the end or just set it by $this->PropertyContainer[$k] = $v.
Same goes for checking of existence. Use isset instead.
And for accessing you can use $this->PropertyContainer[$k] instead of iterating

$k and $v are bad parameter names. While they usually are recognized as key and value, there is no harm to call them this way. But this can be improved: you are talking about properties and not keys. So better name them $propertyName and $value. Shorthands should be used in local scope only (if at all).
While I suppose they are just there for debugging: echo'ing on error is a bad idea. Either return a error code or even better, throw an exception. Of course this requires a method to check if a property exists (e.g. hasProperty).
return; without any value is the default anyway at the end of a method. You can remove this, shorting the code. Elsish after some time another developer might wonder why it was written once: was it located at some other line? was there some return value previously?


Iterating over two iterables I've written a little function that iterates over two iterables. The first one returns an object which is used to convert an object of the second iterable. I've got a working implementation, but would like to get some feedback.

A simple use case would look like this:

list(serialize([String(), String()], [1, 2]))    # should work
list(serialize([String(), String()], range(3)))  # should fail, too many values
list(serialize([String(), String()], range(1)))  # should fail, too few values

But sometimes the number of values is not known but the type is the same. Therefore infinite iterators should be allowed as types argument:

list(serialize(it.repeat(String()), range(3)))   # should work

Of course finite iterators are allowed as well:

list(serialize(it.repeat(String(), times=3), range(3))) # should work
list(serialize(it.repeat(String(), times=4), range(5))) # should fail
# should fail, but probably not that easy to do.
list(serialize(it.repeat(String(), times=4), range(3)))

My working implementation (except for the last case) looks like this:

class String(object):
    def dump(self, value):
        return str(value)

def serialize(types, values):
    class Stop(object):
        """Since None can be valid value we use it as fillvalue."""

    if isinstance(types, collections.Sized):
        # Since None is a valid value, use a different fillvalue.
        tv_pairs = it.izip_longest(types, values, fillvalue=Stop)
    else:
        tv_pairs = it.izip(it.chain(types, (Stop,)), values)

    for type, value in tv_pairs:
        if type is Stop:
            raise ValueError('Too many values.')
        if value is Stop:
            raise ValueError('Too few values.')
        yield type.dump(value)

There is no (general) way of telling the difference between an infinite and a finite iterator.

types might be a generator that stops yielding after a million items have been yielded.  The only fool-proof way of telling would be to consume the generator up to the point where it stops yielding items.  Of course, even if we've consumed a billion items and it hasn't stopped, we can't be sure it wouldn't stop at a later point.

This is why I recommend you change the specification for your function.  The new version will yield items until values have been up.  If there are too few types, it raises ValueError.

def serialize(types, values):
    types_iter = iter(types)
    for value in values:
        try:
            type = next(types_iter)
        except StopIteration:
            raise ValueError("Too few types.")
        yield type.dump(value)

If you really want to, you can check with len() that the lengths of types and values are the same, but as you have said, this will only work for some iterables.

def serialize(types, values):
    try:
        len_types, len_values = len(types), len(values)
    except TypeError:
        pass
    else:
        if len_values > len_types:
            raise ValueError("Too many values.")
        if len_values < len_types:
            raise ValueError('Too few values.')

        for type, value in itertools.izip(types, values):
            yield type.dump(value)
        return

    types_iter = iter(types)
    for value in values:
        try:
            type = next(types_iter)
        except StopIteration:
            raise ValueError("Too many values.")
        yield type.dump(value)


Intel® TinyCrypt: AES-128/CTR to encryption/decryption of 2D arrays I need to encrypt/decrypt 2D arrays (double pointers) using AES-128/CTR using Intel TinyCrypt (written in C). The following are two helper methods to simplify the library usage. Any comments would be highly appreciated. Thanks in advance.

#include <tinycrypt/constants.h>
#include <tinycrypt/ctr_mode.h>
#include <tinycrypt/aes.h>

#define AES_128_KEY_LENGTH 16
#define AES_128_CTR_LENGTH 16

typedef struct aes_128_ctr_params_t{
  byte key[AES_128_KEY_LENGTH];
  byte ctr[AES_128_CTR_LENGTH];
} aes_128_ctr_params_t;

//---------------------------------------------------------------------------
inline int32_t encrypt(uint8_t const * const * const plaintext,
                       uint8_t * const * const cihpertext,
                       size_t const height,
                       size_t const width,
                       aes_128_ctr_params_t params) {
  //TODO: Do some validation here!

  struct tc_aes_key_sched_struct sched;
  uint32_t result = TC_CRYPTO_SUCCESS;
  result = tc_aes128_set_encrypt_key(&sched, params.key);
  if (result != TC_CRYPTO_SUCCESS)
    return result;

  size_t const row_size_in_bytes = sizeof(uint8_t) * width;
  for (size_t row_index = 0; row_index < height; ++row_index) {
    result = tc_ctr_mode(cihpertext[row_index], row_size_in_bytes,
      plaintext[row_index], row_size_in_bytes, params.ctr, &sched);

    if (result != TC_CRYPTO_SUCCESS)
      return result;
  }
}

//---------------------------------------------------------------------------
inline int32_t decrypt(uint8_t const * const * const cihpertext,
                       uint8_t * const * const plaintext,
                       size_t const height,
                       size_t const width,
                       aes_128_ctr_params_t params) {
  //TODO: Do some validation here!

  struct tc_aes_key_sched_struct sched;
  uint32_t result = TC_CRYPTO_SUCCESS;
  result = tc_aes128_set_encrypt_key(&sched, params.key);
  if (result != TC_CRYPTO_SUCCESS)
    return result;

  size_t const row_size_in_bytes = sizeof(uint8_t) * width;
  for (size_t row_index = 0; row_index < height; ++row_index) {
    result = tc_ctr_mode(plaintext[row_index], row_size_in_bytes,
      cihpertext[row_index], row_size_in_bytes, params.ctr, &sched);

    if (result != TC_CRYPTO_SUCCESS)
      return result;
  }
}

I find your code a bit dense and hard to read overall.

Your identifier names are compound words but all lower case.

plaintext

I prefer plainText others would prefer plain_text (and a lot of the code uses this second C like style). But either is preferable to your current style.

This seems redundant.

uint32_t result = TC_CRYPTO_SUCCESS;
result = tc_aes128_set_encrypt_key(&sched, params.key);

Just use one line:

uint32_t result = tc_aes128_set_encrypt_key(&sched, params.key);

Technically both functions exhibit undefined behavior (in C++ not sure about C). There is no return on successful completion.

    if (result != TC_CRYPTO_SUCCESS)
      return result;
  }
  // Add the following line
  return result;
}


Artificial Neural Network Classifier in Matlab I am trying to build a neural network classifier. I have created a neural network with 1 hidden layer (25 neurons) and 1 output layer (1 neuron/binary classification).

The dataset I am using has the following dimensions:

size(X_Train): 125973 x 122
size(Y_Train): 125973 x 1
size(X_Test): 22543 x 122
size(Y_test): 22543 x 1

My overall goal is to compare different training functions. But, I would like first to get your feedback about my code and how I improve it.

% Neural Network Binary-classification

clear ; close all; clc

%% =========== Part 1: Loading Data =============

%% Load Training Data
fprintf('Loading Data ...\n');

load('dataset.mat'); % training data stored in arrays X, y
X_training=X_training';
Y_training=Y_training';
X_testing=X_testing';
Y_testing=Y_testing';

%% Create the neural network
% 1, 2: ONE input, TWO layers (one hidden layer and one output layer)
% [1; 1]: both 1st and 2nd layer have a bias node
% [1; 0]: the input is a source for the 1st layer
% [0 0; 1 0]: the 1st layer is a source for the 2nd layer
% [0 1]: the 2nd layer is a source for your output
net = network(1, 2, [1; 1], [1; 0], [0 0; 1 0], [0 1]);
net.inputs{1}.size = 122; % input size
net.layers{1}.size = 25; % hidden layer size
net.layers{2}.size = 1; % output layer size

%% Transfer function in layers
net.layers{1}.transferFcn = 'logsig';
net.layers{2}.transferFcn = 'logsig';

net.layers{1}.initFcn = 'initnw';
net.layers{2}.initFcn = 'initnw';

net=init(net);

%% divide data into training and test
net.divideFcn= 'dividerand';
net.divideParam.trainRatio = 60/100; % 80% training
net.divideParam.valRatio = 20/100; % 20% validation set
net.divideParam.testRatio = 20/100; % 20% validation set

net.performFcn = 'crossentropy';

%% Training functions
net.trainFcn = 'trainscg'; %Scaled conjugate gradient backpropagation

%% Train the neural network
[net,tr] = train(net,X_training,Y_training); % return the network and training record

%% Test the Neural Network on the training set
outputs = net(X_training);
errors = gsubtract(Y_training,outputs);
performance = perform(net,Y_training,outputs);

%% Plots  (%training)
figure, plotperform(tr)
figure, plottrainstate(tr)

%% Test the Neural Network on the testing test
outputs1 = net(X_testing);
errors1 = gsubtract(Y_testing,outputs1);
performance1 = perform(net,Y_testing,outputs1);

figure, plotconfusion(Y_testing,outputs1)
figure, ploterrhist(errors1)

Below if the validation curve.

Confusion Matrix (Training set)

Confusion Matrix (Testing set)

Any remarks?

Edit:

I have used feature scaling or normalization:

net.performParam.normalization = 'standard';

which has improved the overall accuracy:

For more information, I have added the error histogram:

I'm not a code reviewer, but I'd like to comment on the design of your network, which you certainly seem to be doing just fine.

It might be rather difficult to make any judgment, given that the application is undefined, while it seems you are designing a neural-network based detector.
Numerically speaking, you might focus on your validation performance, by constantly redesigning your network architecture (e.g., number of hidden layers, number of hidden neurons, reducing and increasing batch sizes, training functions/methods as you mentioned, etc.), input preprocessing (e.g., smoothing, input interpolation or extrapolation in case possible, artifact removal, etc.).
Not knowing what your datasets might be and how sophisticated that may be, you may focus on 10^-3 to 10^-6 convergence range. It might increase the performance (e.g., confusion matrices) of your network.

Overall, it seems your input is pretty stochastic. Input preprocessing may be worth looking into.


Converting Roman numerals to integers and vice versa def int_to_roman (integer):

    returnstring=''
    table=[['M',1000],['CM',900],['D',500],['CD',400],['C',100],['XC',90],['L',50],['XL',40],['X',10],['IX',9],['V',5],['IV',4],['I',1]]

    for pair in table:

        while integer-pair[1]>=0:

            integer-=pair[1]
            returnstring+=pair[0]

    return returnstring

def rom_to_int(string):

    table=[['M',1000],['CM',900],['D',500],['CD',400],['C',100],['XC',90],['L',50],['XL',40],['X',10],['IX',9],['V',5],['IV',4],['I',1]]
    returnint=0
    for pair in table:

        continueyes=True

        while continueyes:
            if len(string)>=len(pair[0]):

                if string[0:len(pair[0])]==pair[0]:
                    returnint+=pair[1]
                    string=string[len(pair[0]):]

                else: continueyes=False
            else: continueyes=False

    return returnint

def int_to_roman (integer):

    returnstring=''
    table=[['M',1000],['CM',900],['D',500],['CD',400],['C',100],['XC',90],['L',50],['XL',40],['X',10],['IX',9],['V',5],['IV',4],['I',1]]

The element in your list should really be tuples not lists. It should also be a global constant so that you can reuse across both functions.

    for pair in table:

Use for letter, value in table: rather then indexing the tuples.

        while integer-pair[1]>=0:

I think the code looks better with spacing around binary operators. Also why this instead of: while integer >= pair[1]:?

            integer-=pair[1]
            returnstring+=pair[0]

It'll probably be better to create and append to list and then join the list elements together at the end.

    return returnstring

def rom_to_int(string):

    table=[['M',1000],['CM',900],['D',500],['CD',400],['C',100],['XC',90],['L',50],['XL',40],['X',10],['IX',9],['V',5],['IV',4],['I',1]]
    returnint=0
    for pair in table:

        continueyes=True

Whenever I use a logical flag like this, I think: it should be removed. I figure that flags like this only serve to confuse the logic of what you are doing. i think a break is clearer then setting a flag.

        while continueyes:
            if len(string)>=len(pair[0]):

                if string[0:len(pair[0])]==pair[0]:

strings have a funciton: startswith that does this. You should use it here. There is also need to check the length. If you take a slice past the end of a string in python, you just get a shorter string.

                    returnint+=pair[1]
                    string=string[len(pair[0]):]

                else: continueyes=False
            else: continueyes=False

    return returnint

My version of your code:

def int_to_roman (integer):
    parts = []
    for letter, value in TABLE:
        while value <= integer:
            integer -= value
            parts.append(letter)
    return ''.join(parts)

def rom_to_int(string):
    result = 0
    for letter, value in table:
        while string.startswith(letter):
            result += value
            string = string[len(pairs[0]):]
    return result

One last thought. Your rom_to_int doesn't handle the case where an invalid string is passed. You might want to consider having it throw an exception or something in that case.


Ordering Gallery images by custom order I wrote some code a few weeks ago, to take a string similar to

  seq[]=8&seq[]=7&seq[]=5&seq[]=1&seq[]=4&seq[]=3&seq[]=6&seq[]=0&seq[]=2

and put it into a gallery, to be ordered by jQuery UI's Sortable, which works all good, however, I'm concerned about the amount of times I .Replace the values in the string.

This code has been bugging me, because I'm employed to rewrite unethical code, and the few times I actually write code from scratch, it looks like this. So, basically, I would love your opinions on this code.

To clarify, the imgMgr is to get the JSON string, and the nodelist is for getting an XML doc of images from the imgMgr.

#region BuildGallery
private void BuildGallery()
{

    StringBuilder sb = new StringBuilder();

    sb.Append("
<button class=\"js-serialize\" style=\"border: 0;height: 25px;background-color: #F4FA58;margin-bottom: 8px;\">Save Layout</button>

");
    int pos = 0;
    string str = "";
    ImageManagerV1 imgMgr = new ImageManagerV1();
    str = imgMgr.galleryOrder(LocalConfig.ImageManagerAuthCode, Convert.ToInt32(hdnListingId.Value), "");
  //  str = "seq[]=8&seq[]=7&seq[]=5&seq[]=1&seq[]=4&seq[]=3&seq[]=6&seq[]=0&seq[]=2";
    str.Replace("seq[]=","-").Split('-');
    XmlNodeList nodelist = xmlDoc.SelectNodes("image");
    str = str.Replace("&", "");
    if (str == "") {
        for (int i = 0; i < nodelist.Count; i++) {
            str += "seq[]=" + i;
        }
    }
    string[] strNew = str.Replace("seq[]=", "-").Split('-');
        str = "";
        for (int i = 0; i < strNew.Length; i++) {
            if (strNew[i] != "") {
                str += "-" + strNew[i];
            }
        }
    if (nodelist.Count > strNew.Length) {
        for (int f = strNew.Length; f < nodelist.Count; f++) {
            str += "-" + f;
        }
    }
    int val2 = 0;
    string[] stringArray = str.Split('-');
    foreach (string num in stringArray) {
        var newNum = num.ToString();
        if (newNum.Length > 0) {
            val2 = Convert.ToInt32(newNum);
            XmlNode image = nodelist[val2];
            string thumbpath = image.Attributes["thumb"].Value;
            string detailpath = image.Attributes["detail"].Value;
            string fileName = detailpath.Substring(detailpath.LastIndexOf(@"/") + 1);
            sb.AppendFormat("
", val2);
            sb.AppendFormat("Make HeroDel", thumbpath, detailpath, listing.ListingName, pos, fileName);
            sb.Append("
");
        }
    }
    sb.Append("

");
        if (sb.Length > 0)
        {
            litGallery.Text = string.Format("
{0}
", sb.ToString());
        }
    }
#endregion

string str = "";

There's no sense in assigning str to an empty string just to over write it two lines later. This would be much more terse.

ImageManagerV1 imgMgr = new ImageManagerV1();
string str = imgMgr.galleryOrder(LocalConfig.ImageManagerAuthCode, Convert.ToInt32(hdnListingId.Value), "");

str.Replace("seq[]=","-").Split('-');

Why not just split on "seq[]="? The Replace feels unnecessary here. Actually, the more I look at this, why are you splitting? Split returns an array, but you're not making an assignment here. I really think you need to decide what this line is supposed to do.

str = str.Replace("&", "");
if (str == "") {
    for (int i = 0; i < nodelist.Count; i++) {
        str += "seq[]=" + i;
    }
}

Two things. As a maintainer of the code, I've no idea why you're stripping out the ampersands. A comment here is in order. Secondly, this loop needs a StringBuilder. StringBuilder is a mutable string of characters and thus, more efficient in this situation.

string[] strNew = str.Replace("seq[]=", "-").Split('-');

Okay, now you can skip the Replace and split directly on "seq[]=". Speaking of, that string seems to show up an awful lot. You should replace it with a constant.

string[] separators = new string[] {"seq[]="};
strNew = str.Split(separators, SplitStringOptions.None);

    for (int i = 0; i < strNew.Length; i++) {
        if (strNew[i] != "") {
            str += "-" + strNew[i];

At first glance, I want to tell you to use a string builder again, but I honestly don't have the foggiest idea why you just split the string apart, just to put it back together. Forget what I just said about splitting, and just Replace("seq[]=","-"). Don't split. It looks to be unecessary.


Is this use of JavaScript prototype functions for attaching event listeners going to meet design goals? Please provide a sanity check on my below JavaScript pattern to be used for a custom edit control. The goals are:

Centralized code
Avoid name collisions
Minimize memory use (putting functions on the prototype chain to accomplish?)
Ease of use for other developers

My specific question is, are there any "gotchas" in below pattern that I may have overlooked or anything that I'm doing that is just plain wrong?

var CustomEditor = function (id) {
    'use strict';
    var evnt = null,
        funct = null;
    this.id = id;
    this.init();
};

CustomEditor.prototype.onCustomBlur = function () {
    'use strict';
    // alert("blur event handler");
    this.style.backgroundColor = "#fff";
    this.style.color = "#f00";
};

CustomEditor.prototype.onCustomKeyPress = function () {
    'use strict';
    // alert("keypress event handler");
    this.style.backgroundColor = "#000";
    this.style.color = "#ff0";
};

CustomEditor.prototype.onCustomFocus = function () {
    'use strict';
    // alert("focus event handler");
    this.style.backgroundColor = "#000";
    this.style.color = "#0ff";
};

CustomEditor.prototype.addEvent = function () {
    'use strict';
    var element = document.getElementById(this.id);
    // old IE work around
    if (element.attachEvent) {
        return element.attachEvent('on' + this.evnt, this.funct);
    }
    return element.addEventListener(this.evnt, this.funct, false);
};

CustomEditor.prototype.init = function () {
    'use strict';
    // initialize a custom editor
    this.evnt = 'blur';
    this.funct = this.onCustomBlur;
    this.addEvent();

    this.evnt = 'keypress';
    this.funct = this.onCustomKeyPress;
    this.addEvent();

    this.evnt = 'focus';
    this.funct = this.onCustomFocus;
    this.addEvent();
};

This code will be JavaScript code in the HTML page to allocate and initialize:

new CustomEditor('myInput1');
new CustomEditor('myInput2');
new CustomEditor('myInput3');

See this JSFiddle to try this pattern or fork as needed to correct.

Looks fine to me; but I think the this.evnt & this.funct abstractions are unnecessary. Would always promote readable properties over shorthand (funct -> function etc), and I think that you should change the addEvent method to accept parameters. You can then clean up the code like so:

var CustomEditor = function(id) {
    'use strict';
    this.id = id;
    this.init();
};

CustomEditor.prototype.addEvent = function(event, callback) {
    'use strict';
    var element = document.getElementById(this.id);
    // old IE work around
    if (element.attachEvent) {
        return element.attachEvent('on' + event, callback);
    }
    return element.addEventListener(event, callback, false);
};

CustomEditor.prototype.init = function() {
    'use strict';
    this.addEvent('blur', this.onCustomBlur);
    this.addEvent('keypress', this.onCustomKeyPress);
    this.addEvent('focus', this.onCustomFocus);
};

See the fiddle.


Calculate min/max values for (un)signed integral number of up to 64 bits This question is a follow up of Calculate min/max values for signed or unsigned integral number with any valid number of bits (up to 64)

I was answering the question Given two int values, return the one closer to 10, and I indicated that something like the following snippet of code could be a different approach for solving the problem.

In the original question, it turned out I had made a mistake, but since there were already some answers, I've decided to post a follow up question with the fixes.

Can this be done in a more efficient or more elegant way?

public class Bits
{
    public static void MinMaxI(ulong _bits, out long _min, out long _max)
    {
        // ? unsigned range with at least one value bit
        if (_bits < 2 || _bits > 64)
        {
            throw new System.ArgumentOutOfRangeException("_bits", _bits,
             String.Format("2 <= _bits <= 64"));
        }

        _min = ~(long)0 << (int)(_bits - 1);
        _max = -1 - _min;
    }

    public static long MinI(ulong _bits)
    {
        // ? unsigned range with at least one value bit
        if (_bits < 2 || _bits > 64)
        {
            throw new System.ArgumentOutOfRangeException("_bits", _bits,
             String.Format("2 <= _bits <= 64"));
        }

        return ~(long)0 << (int)(_bits - 1);
    }

    public static long MaxI(ulong _bits)
    {
        // ? unsigned range with at least one value bit
        if (_bits < 2 || _bits > 64)
        {
            throw new System.ArgumentOutOfRangeException("_bits", _bits,
             String.Format("2 <= _bits <= 64"));
        }

        return -1 - (~(long)0 << (int)(_bits - 1));
    }

    public static ulong MaxN(ulong _bits)
    {
        // ? unsigned range with at least one value bit
        if (_bits < 1 || _bits > 64)
        {
            throw new System.ArgumentOutOfRangeException("_bits", _bits,
             String.Format("1 <= _bits <= 64"));
        }

        return _bits == 64 ? ~(ulong)0 : ~(~(ulong)0 << (int)_bits);
    }
}

Your argument validation could be de-duplicated.

Here, take a look:

MinMaxI:
    if (_bits < 2 || _bits > 64)
    {
        throw new System.ArgumentOutOfRangeException("_bits", _bits,
         String.Format("2 <= _bits <= 64"));
    }

MinI:
    if (_bits < 2 || _bits > 64)
    {
        throw new System.ArgumentOutOfRangeException("_bits", _bits,
         String.Format("2 <= _bits <= 64"));
    }

MaxI:
    if (_bits < 2 || _bits > 64)
    {
        throw new System.ArgumentOutOfRangeException("_bits", _bits,
         String.Format("2 <= _bits <= 64"));
    }

MaxN:
    if (_bits < 1 || _bits > 64)
    {
        throw new System.ArgumentOutOfRangeException("_bits", _bits,
         String.Format("1 <= _bits <= 64"));
    }

They're practically the same!

I'd make a method like this (I'm not really familiar with C# syntax, so let me know if I made a mistake)

private static void validateBitsArgument(ulong _bits, ulong min, ulong max)
{
    if (_bits < min || _bits > max)
    {
        throw new System.ArgumentOutOfRangeException("_bits", _bits,
         String.Format("{0} <= _bits <= {1}", min, max));
    }
}

And then include it like this:

public class Bits
{
    public static void MinMaxI(ulong _bits, out long _min, out long _max)
    {
        validateBitsArgument(_bits, 2, 64);

        _min = ~(long)0 << (int)(_bits - 1);
        _max = -1 - _min;
    }

    public static long MinI(ulong _bits)
    {
        validateBitsArgument(_bits, 2, 64);

        return ~(long)0 << (int)(_bits - 1);
    }

    public static long MaxI(ulong _bits)
    {
        validateBitsArgument(_bits, 2, 64);

        return -1 - (~(long)0 << (int)(_bits - 1));
    }

    public static ulong MaxN(ulong _bits)
    {
        validateBitsArgument(_bits, 1, 64);

        return _bits == 64 ? ~(ulong)0 : ~(~(ulong)0 << (int)_bits);
    }

    private static void validateBitsArgument(ulong _bits, ulong min, ulong max)
    {
        if (_bits < min || _bits > max)
        {
            throw new System.ArgumentOutOfRangeException("_bits", _bits,
         String.Format("{0} <= _bits <= {1}", min, max));
        }
    }
}

And voila, shorter code.