BFS tree traversal Scala I would love some feedback on the following code for BFS traversal in a binary tree. Is there a cleaner way to write this?

abstract class Tree[+T]
case class Node[T](value:T,var left:Tree[T],var right:Tree[T]) extends Tree[T]
case object End extends Tree[Nothing]

def bfs_v2[T](node:Node[T]):List[T] = {
    def enQueueNext(queue: Queue[Node[T]], aNode: Tree[T]) {
      aNode match {
        case internalNode: Node[T] => queue += internalNode
        case End => {}
      }
    }
    def bfs_visit(queue:Queue[Node[T]],visited:List[T]):List[T] = {
      if(queue.isEmpty){
        visited
      }
      else{
        val currentNode = queue.dequeue()
        enQueueNext(queue, currentNode.left)
        enQueueNext(queue,currentNode.right)
        bfs_visit(queue,currentNode.value :: visited )
      }
    }
    val aQueue = new mutable.Queue[Node[T]]()

    bfs_visit(aQueue += node,List[T]()).reverse
  }

As pointed out by @Bob Dalgleish, you mix mutable/immutable.  You also mix pattern matching (enQueueNext) with if/else (bfs_visit).  You also mix camel case notation (enQueueNext) with underscore notation (bfs_visit).

trait would be more appropriate for Tree than abstract class.  You don't really need Tree, as I show in my example, but that is debatable.  You don't really need a queue either.

You can make very succinct code with Scala.  In my example below, the recursive BFS algorithm is basically a single line: bfsLoop(nextLayer.map(_.value) :: accum, nextLayer.flatMap(_.childrenLeftRight)).

object TreeTraversal extends App {

  case class Node[+T](value: T, left: Option[Node[T]], right: Option[Node[T]]) {
    def map[V](f: T => V): Node[V] =
                 Node(f(value), left.map(l => l.map(f)), right.map(r => r.map(f)))
    def childrenLeftRight: List[Node[T]] = List(left, right).flatten
  }
  def terminalNode[T](value: T) = Node(value, None, None)

  def dfs[T](tree: Node[T]): List[T] = {
    var output = List[T]()
    tree.map(t => (output = t :: output))
    output.reverse
  }

  def bfs[T](tree: Node[T]): List[T] = {
    @tailrec
    def bfsLoop(accum: List[List[T]], nextLayer: List[Node[T]]): List[T] = nextLayer match {
      case Nil => accum.reverse.flatten
      case _ => bfsLoop(nextLayer.map(_.value) :: accum, nextLayer.flatMap(_.childrenLeftRight))
    }
    bfsLoop(List[List[T]](), List(tree))
  }

  val tree1 = Node[Int](6, Some(Node(13, Some(terminalNode(101)), None)), Some(Node(42, Some(terminalNode(666)), Some(terminalNode(65)))))
  println("map: " + tree1.map(i => s"Hello$i"))
  println("dfs: " + dfs(tree1))
  println("bfs: " + bfs(tree1))
}

I also put this code for a separate codereview after posting it as an answer.


Custom checkbox validation on submit I have the code below which is working as expected. It is doing a form validation with Vue.js and Vee-Validate. It is checking that at least one checkbox is checked.

I only don't like that I have to manually call validate for oneChecked-Validator on form submission. With-out the call it's not validating on submit.

It would be great if someone could review my code and give me an advice to the following points:

How can I configure Vee Validator so that it evaluates the oneChecked validator with the call of this.$validator.validateAll();?
Is it possible to pass the this.options array into validate method of oneChecked validator? (Would be more re-usable. I've tried passing as array with JSON.stringify but couldn't get it to work.)

If there is more to improve please let me know. I'm using Vee-Validate for the first time.

You can also find the same code at jsfiddle.

const Checkboxes = {
  template: '#checkboxTmpl',
  data() {
    return {
      text: '',
      options: [{
        id: 0,
        checked: false
      }, {
        id: 1,
        checked: false
      }, {
        id: 2,
        checked: false
      }]
    };
  },
  created() {
    this.$validator.extend('oneChecked', {
      getMessage: field => 'At least one ' + field + ' needs to be checked.',
      validate: (value, [testProp]) => {
        const options = this.options;
        // console.log('questions', value, testProp, options.some((option) => option[testProp]));
        return value || options.some((option) => option[testProp]);
      }
    });
  },
  methods: {
    validateBeforeSubmit(e) {
      this.$validator.validateAll(); // why is oneChecked not validated here? --> manually trigger validate below
      this.options.forEach((option) => {
        this.$validator.validate('check0', option.checked, ['checked'])
      });

      console.log('validator', this.errors);
      if (!this.errors.any()) {
        alert('succesfully submitted!');
      }
    }
  }
};

Vue.use(VeeValidate);

const app = new Vue({
  el: '#app',
  render: (h) => h(Checkboxes)
})

How can I configure Vee Validator so that it evaluates the oneChecked validator with the call of this.$validator.validateAll();?

Yes, you can use validateAll(), you're just not passing the fields for it to validate. Here's an example on how to do this:

methods: {
  validateBeforeSubmit(e) {
    this.$validator.validateAll({
      name0: true
    });
    console.log('validator', this.errors);
    if (!this.errors.any()) {
      alert('succesfully submitted!');
    }
  }
}

Taken from here.

  Is it possible to pass the this.options array into validate method of oneChecked validator? (Would be more re-usable. I've tried passing as array with JSON.stringify but couldn't get it to work.)

Why pass it as a parameter when it's already a constant?

Consider this:

created() {
  this.$validator.extend('oneChecked', {
    getMessage: field => 'At least one ' + field + ' needs to be checked.',
    validate: (value, [testProp]) => {
      return value || Checkboxes.data().options.some((option) => option[testProp]);
    }
  });
},

This also provides you some protection for when data().options inevitably has more than 3 elements in it.


Access to a pimpl's members from another pimpl I am writing an audio engine that must be multi-platform (win/linux). Basically, a CSound represents a sound file, and CSoundInstance represents one playing of a CSound. Obviously, playing sound is very different on Windows than it is on linux.

A CSoundInstance needs access to some data from its corresponding CSound. The data is heavily implementation-dependent and cannot really be abstracted.

Here are the API headers for the two classes. I omitted operator= overloads / unique_ptr use etc.

API/Sound.hpp

class CSound
{
public:
    CSound(...file...);
    ~CSound();
private:
    CSoundImpl *impl;
};

API/SoundInstance.hpp

class CSoundInstance
{
public:
    CSoundInstance(CSound *); // going to need access to source voice buffer
    ~CSoundInstance();
    Play();
private:
    CSoundInstanceImpl *impl;
};

I guess it is clear where this is going. You would first instanciate a CSound, then acquire a CSoundInstance and Play it.

I Intend to have a specific XXImpl class for each system and platform. Under Windows, I use XAudio2, which needs several members in CSoundImpl (buffer + waveform descriptor), as well as CSoundInstanceImpl (voice + fx + sends + callbacks).

Here are the problems that I had using the pimpl idiom:

To play a sound, a CSoundInstanceImpl ultimately needs a reference to a CSoundImpl, on Windows and linux, but I cannot write this constructor:

CSoundInstance::CSoundInstance(CSound *sound)
    : impl(sound->impl)
{
    ...
}

Not only is sound->impl private, but it would also mess up unique_ptr use.

Another thing I tried is not using the constructor CSoundInstance(CSound *) at all, and adding a factory method with the signature CSoundInstance CSound::CreateInstance(), like this:

CSoundInstance CSound::CreateInstance()
{
    return this->impl->CreateInstance();
}

CSoundInstance CSoundImpl::CreateInstance()
{
    // here I have access to the CSoundImpl instance, that I need: this
    CSoundInstance instance(this);
}

But that would imply that I have this constructor:

CSoundInstance::CSoundInstance(CSoundImpl *soundImpl)
    : impl(new CSoundInstanceImpl(soundImpl))
{
    ...
}

Using a pointer to an implementation in the constructor of a non-implementation API class doesn't look right to me.

Maybe pimpl is not the right thing for this? Is there a way how I could tackle this problem better?

As you said, if you need something in a pimpl class from another class, then something is wrong. If you can't abstract away the differences, then you can't hide the differences and shouldn't use this idiom.

Don't you think it would be simpler to put Play() in CSound? Each version (Linux/Windows) would have its own pimpl and there would be no need to try to find an abstraction or to try to access private data. Otherwise it gets a bit messy, since your Linux/Windows implementations of CSoundInstances will need to accept only the correct implementation: I don't know how easy it is to do this at compile time.


Precognitive waiting - waiting for something to finish, before it starts I have implemented a class that has a method to wait until "something else" has happened. When this has happened, the current thread stops waiting and returns from the method. This is my current code:

public class InMemoryAviReadyNotifier implements AviReadyNotifier, MessageListener<UrlAviMessage>
{
    private Map<SourceAndFileName, CountDownLatch> m_map = Maps.newConcurrentMap();
    private Map<SourceAndFileName, URL> m_resultMap = Maps.newConcurrentMap();

    @Override
    public void messageReceived( MessageSource messageSource, UrlAviMessage message )
    {
        SourceAndFileName key = new SourceAndFileName( (DetectorMessageSource)messageSource, message.getFileName() );
        CountDownLatch countDownLatch = m_map.get( key );
        if( countDownLatch != null )
        {
            m_resultMap.put( key, message.getLocation() );
            countDownLatch.countDown();
        }
        m_map.remove( key );
    }

    @Override
    public Class<? extends DeviceToServerMessage> responseClass()
    {
        return UrlAviMessage.class;
    }

    @Override
    public InputStream waitForAvi( DetectorMessageSource messageSource, String aviFileName ) throws InterruptedException
    {
        CountDownLatch countDownLatch = new CountDownLatch( 1 );
        SourceAndFileName key = new SourceAndFileName( messageSource, aviFileName );
        m_map.put( key, countDownLatch );
        countDownLatch.await();
        URL url = m_resultMap.get( key );
        // TODO download the avi to a temp local folder so we can add connect and read timeouts on the connection
        return url.openStream();
    }

    private static class SourceAndFileName
    {
        private final DetectorMessageSource m_source;
        private final String m_fileName;

        public SourceAndFileName( DetectorMessageSource source, String fileName )
        {
            m_source = source;
            m_fileName = fileName;
        }

        @Override
        public boolean equals( Object o )
        {
            boolean result = false;

            if (this == o)
            {
                result = true;
            }
            else if (o instanceof SourceAndFileName)
            {
                SourceAndFileName other = (SourceAndFileName)o;

                result = Objects.equals( m_source, other.m_source)
                        && Objects.equals( m_fileName, other.m_fileName );
            }

            return result;
        }

        @Override
        public int hashCode()
        {
            return Objects.hash( m_source, m_fileName );
        }

        @Override
        public String toString()
        {
            return toStringHelper( this )
                    .add( "source", m_source )
                    .add( "fileName", m_fileName )
                    .toString();
        }
    }
}

The waitForAvi method gets called from one thread. It will wait until messageReceived gets called with the good arguments and then it will countdown the CountDownLatch. I need to get the message.getLocation() return value (an URL) in the waitForAvi method. I using 2 Maps like I do here the best option or are there alternatives I could consider?

I am aware that in the current implementation I can potentially wait forever if the messageReceived never happens, but adding timeouts to this code has not been done yet.

You have an interesting problem to solve. Your solution would be helped a lot with a small extension to the SourceAndFileName class. Consider the following version of it:

private static class SourceAndFileName
{
    private final DetectorMessageSource m_source;
    private final String m_fileName;

    private final ReentrantLock lock = new ReentrantLock();
    private final Condition arrived = lock.condition();
    private URL aviURL = null;

    public SourceAndFileName( DetectorMessageSource source, String fileName )
    {
        m_source = source;
        m_fileName = fileName;
    }

    public void setURL(URL url) {
        lock.lock();
        try {
            if (aviURL == null) {
                aviURL = url;
            }
            arrived.signalAll();
        } finally {
            lock.unlock();
        }
    }

    public URL getURL(long timeout) {
        long now = System.currentTimeMillis();
        final long delayto = now + timeout;
        lock.lock();
        try {
            while (aviURL == null && now < delayto) {
                arrived.await(delayto - now, TimeUnit.MILLISECONDS);
                now = System.currentTimeMillis();
            }
            return aviURL
        } finally {
            lock.unlock();
        }

    }

    .... the rest is the same.....
}

OK, so, with the above class, you can get (with a timed delay, if you want) the URL, and you can set it too. This encapsulates the logic for the process well... but, the problem is that you need to have one, and only one instance of this for each source/fileName. This is solved with a single ConcurrentMap... (using the same key/value instances).

private ConcurrentMap<SourceAndFileName, SourceAndFileName> m_map = Maps.newConcurrentMap();

private SourceAndFileName getKey(DetectorMessageSource messageSource, String fileName)
{
    // be optimistic, and assume we are the first in....
    SourceAndFileName key = new SourceAndFileName(messageSource, fileName);
    SourceAndFileName current = m_map.putIfAbsent(key,key);
    if (current != null) {
        // we were wrong, and not the first in, so use the key from the previous one.
        return current;
    }
    return key;
}

@Override
public void messageReceived( MessageSource messageSource, UrlAviMessage message )
{
    // be optimistic, and assume we are first in....
    SourceAndFileName key = getKey((DetectorMessageSource)messageSource, message.getFileName());

    // set the location, which will trigger the get to return, if necessary...
    key.setURL(message.getLocation());

}

@Override
public InputStream waitForAvi( DetectorMessageSource messageSource, String aviFileName ) throws InterruptedException
{
    // be optimistic, and assume we are first in....
    SourceAndFileName key = getKey((DetectorMessageSource)messageSource, message.getFileName());

    URL url = key.get( 5000 ); // 5 second delay....

    // TODO download the avi to a temp local folder so we can add connect and read timeouts on the connection

    return url.openStream();
}

A more general code review is also in order...

The m_... type prefixing of variables is not useful in modern IDE's where it is normally color-coded where variables are from....
Java style guidelines put the { opening brace on the same line as the block condition (while/if/etc.).
your hashCode/equals methods on the key class would benefit from early-return statements instead of carrying the result variable.


Random quote rotator On my website, I created like this banner that rotates quotes from an array I made.

var myarray = []
myarray[0] = "\"To a contrarian like me, constant advice not to do something almost always starts me quickly down the risky, unpopular path.\"
  Micheal Bloomberg.";
myarray[1] = "\"In investing, what is comfortable is rarely profitable.\"
 Robert Arnott";
myarray[2] = "\"Know what you own, and know why you own it.\"
 Peter Lynch";
myarray[3] = "\"Buy when everyone else is selling and hold until everyone else is buying. That’s not just a catchy slogan. It’s the very essence of successful investing.\"
 J. Paul Getty";
myarray[4] = "\"When something is important enough, you do it even if the odds are not in your favor.\"
 Elon Musk";
myarray[5] = "\"I will tell you the secret to getting rich on Wall Street. You try to be greedy when others are fearful. And you try to be fearful when others are greedy.\"
 Warren Buffet";
myarray[6] = "\"Innovation distinguishes between a leader and a follower.\"
 Steve Jobs";
myarray[7] = "\"The only thing that makes sense is to strive for greater collective enlightenment.\"
 Elon Musk"
myarray[8] = "\"The single best piece of advice: Constantly think about how you could be doing things better and questioning yourself.\"
 Elon Musk "
//Quote randomizer I made.

function quote() {
    var x = Math.floor(Math.random() * (myarray.length));
    document.getElementById("quotes").innerHTML = myarray[x];

}

var myVar = setInterval(quote, 15000);

Is there a better way to write this code? I wrote it back in February of this year. Now, I am just curious if there is any way to improve it.

For those who want to see it in action, scroll down past the "meet the team" section after you click here.

Cache DOM selectors

You select the target container over and over again. You should get it once outside the function, cache it in a variable and reuse it:

const target = document.getElementById('quotes');

Naming

Try to use descriptive names instead of myarray, myVar and quote:

const quotes = [];
function changeQuoteRandomly() {}

Style

It's easier to read, if you use single quotes on the strings, as you don't have to escape the double quotes:

quotes.push('"Important words" by Someone');

You sometimes use a semicolon at the end of the line and sometimes you don't – use only one way and be consistent:

var myarray = []
var myVar = setInterval(quote, 15000);

I would prefer using push to add elements to the array instead of setting the index manually. Or you could even add all elements during initialization:

const quotes = [
    '"Important words" by Someone',
    '"Other important words" by Someone'
];

Both ways make it easier to re-arrange elements later as you don't have to keep track of the index yourself.

Semantics

Currently your markup looks like this:

    "The single best piece of advice: Constantly think about how you could be doing things better and questioning yourself."

     Elon Musk

You could improve this, by using blockquote, cite and even figure elements. Here you can find more infos and examples on W3C: 4.4.5. The blockquote element.

User experience

While this is a nice addition on your website, there are a few downsides:

There's no introduction to this section, the visitor has no clue why you're showing them. Try to relate them to your business.
The first quote is shown after 15s, until then only an empty blue container ist visible.
15s between quotes is a really long time. There's no indicator that more is coming, so there's a great chance that a user will not see a second one.


Sort a list in Python My aim is to sort a list of strings where words have to be sorted alphabetically. Words starting with s should be at the start of the list (they should be sorted as well), followed by the other words.

The below function does that for me.

def mysort(words):
    mylist1 = sorted([i for i in words if i[:1] == "s"])
    mylist2 = sorted([i for i in words if i[:1] != "s"])
    list = mylist1 + mylist2
    return list

I am looking for better ways to achieve this, or if anyone can find any issues with the code above.

Some notes about your code:

sorted([i for i in words if i[:1] == "s"]): No need to generate an intermediate list, drop those [ ] and you'll get a lazy generator expression instead.
mylist1. This my prefix looks pretty bad. Do other languages use them?
list: Red flag. Don't overshadow a built-in (at least not one so important) with a variable.
return list. You can directly write return mylist1 + mylist2, no gain in naming the value you're about to return (the name of the function should inform about that).
i[:1] == "s" -> i.startswith("s").

Now what I'd write. Using sorted with the argument key (Schwartzian transform) and taking advantage of the lexicographical order defined by tuples:

def mysort(words):
    return sorted(words, key=lambda word: (0 if word.startswith("s") else 1, word))

If you are familiar with the fact that False < True, it can be simplified a bit more:

sorted(words, key=lambda word: (not word.startswith("s"), word))

Here's another way using the abstraction partition:

words_with_s, other_words = partition(words, lambda word: word.startswith("s"))
sorted(words_with_s) + sorted(other_words)

Writing partition is left as an exercise for the reader :-)


Mesh generation in Unity3D I have a mesh generation script which initialises a mesh with vertices that represent my map tiles as quads. Then I pass the tiles that I wish to display on the mesh by setting my triangles for the vertices related to the tile.

I was wondering if there is a more optimised way to approach this for Unity. I'm still learning about mesh generation but am unsure if I have designed it efficiently.

This is my initial generation of the mesh before I create any tiles:

private void Awake(){
  Generate();
}
private void Generate ()
{
    vertices = new Vector3[mapLength * mapWidth * 4];
    int v = -1;

    for (int j = 0; j < mapLength; j++)
    {
        for (int i = 0; i < mapWidth; i++)
        {
            vertices[++v] = new Vector3(i * gridSize, 0, j * gridSize); // bottom left
            vertices[++v] = new Vector3(i * gridSize + gridSize, 0, j * gridSize); // bottom right
            vertices[++v] = new Vector3(i * gridSize, 0, j * gridSize + gridSize); // top left
            vertices[++v] = new Vector3(i * gridSize + gridSize, 0, j * gridSize + gridSize); //top right
        }
    }
    GetComponent<MeshFilter>().mesh = mesh = new Mesh();
    mesh.name = "Floor Grid";
    mesh.MarkDynamic();
    mesh.vertices = vertices;
}

So, now I have created the mesh with the vertices for my quads. I now apply triangles to it when the user wishes to draw tiles I obtain the triangles array and add to it like so:

private void Add(List<Vector3> tiles)
{
    int[] triangles;
    if (mesh.triangles.Length == 0)
    {
        // there are currently no tiles so create new array match tile count
        triangles = new int[tiles.Count * 6];
    }
    else
    {
        // we already have some tiles so expand the existing array
        triangles = new int[mesh.triangles.Length + (tiles.Count * 6)];
    }

    // get the triangle indices from mesh.triangle and apply them to new array
    int index = 0;
    for (int i = 0; i < mesh.triangles.Length; i++)
    {
        trianglesList.AddRange(mesh.triangles);
        triangles[i] = mesh.triangles[i];
        index = i+1;
    }

    //apply the new triangles for the tiles the user wants
    for (int i = 0; i < tiles.Count; i++)
    {
        tri = (int)(tiles[i].z * mapWidth + tiles[i].x) * 4;
        triangles[index++] = tri;
        triangles[index++] = tri + 2;
        triangles[index++] = tri + 1;

        triangles[index++] = tri + 2;
        triangles[index++] = tri + 3;
        triangles[index++] = tri + 1;
    }

    mesh.triangles = triangles;
    mesh.RecalculateNormals();
}

This code works, but I am no expert at mesh generation and have basically picked up knowledge from others and example scripts over time.

I was wondering if there is any optimising I can do here. Or have I understood how it is done and this is pretty much as good at it gets?

I can't comment on the specifics of how efficient your algorithm is, but i can comment on something that is probably more important - assuming you have enough computing power:

Readability and Understandability

if you didn't know anything about Mesh code above, it is very difficult to understand. this means that if you were to look at it in 12 months - would you understand it? this is very important when writing code.

Code

Generally speaking be very suspicious of "if" statements. Whenever you see one you gotta ask: why is that there? if you don't have a good reason, then it might be better of to extract a class and use polymorphism to do the same thing. that way, conditional statements are somewhat isolated from behaviour. that makes things easier to change and somewhat easier to code, but it does come at a price.

Methods

You can hide away some implementation in methods. you've got some methods that are very long that it's not easy for someone unfamiliar with what you're trying to do to make sense of it.

int[] triangles;
if (mesh.triangles.Length == 0)
{
    // there are currently no tiles so create new array match tile count
    triangles = new int[tiles.Count * 6];
}
else
{
    // we already have some tiles so expand the existing array
    triangles = new int[mesh.triangles.Length + (tiles.Count * 6)];
}

you can turn the above into this:

int[] triangles = GetTriangles();

and hide everything in that method. it would already dramatically improve readability

Update

A picture is worth a million words! I can sort of conceptualise what you are trying to do.

Unfortunately i can't comment on the best OOP way because i have a very incomplete pictures but something like this would certainly give you ideas on how to structure code to make it more readable. Without altering what it does.

Secondly - tests - i'm gonna guess that you have written tests for this? if you did then tests will force you to write code more systematically so i highly recommend adding tests.

here is just some basic cutting and pasting - i hope it helps.

Performance

btw - your algorithm looks good enough. but be warned, this type of looping can slow things done if mapLength and width gets too large.

for (int j = 0; j < mapLength; j++)
    {
        for (int i = 0; i < mapWidth; i++)
        {
.....}}

using System.Collections.Generic;
namespace practiceTest
{
    internal class UnityMesh
    {
        private Mesh mesh;
    private void Awake()
    {
        Mesh mesh = CreateMesh();
        this.mesh = mesh;
    }

    /// <summary>
    /// Generates this instance - i don't like the name of this method. it's way too vague
    /// </summary>
    /// <returns></returns>
    private Mesh CreateMesh()
    {
        Vector3d vertices = GetVertices();
        Mesh mesh = SetVerticesToNewMesh(vertices);
        return mesh;
    }

    private Mesh SetVerticesToNewMesh(Vector3d vertices)
    {
        GetComponent<MeshFilter>().mesh = mesh = new Mesh();
        mesh.name = "Floor Grid";
        mesh.MarkDynamic();
        mesh.vertices = vertices;

        return mesh;
    }

    /// <summary>
    /// Gets the vertices.
    /// </summary>
    /// <returns></returns>
    private Vector3d GetVertices()
    {
        vertices = new Vector3[mapLength * mapWidth * 4];
        int v = -1;

        for (int j = 0; j < mapLength; j++)
        {
            for (int i = 0; i < mapWidth; i++)
            {
                vertices[++v] = new Vector3(i * gridSize, 0, j * gridSize); // bottom left
                vertices[++v] = new Vector3(i * gridSize + gridSize, 0, j * gridSize); // bottom right
                vertices[++v] = new Vector3(i * gridSize, 0, j * gridSize + gridSize); // top left
                vertices[++v] = new Vector3(i * gridSize + gridSize, 0, j * gridSize + gridSize); //top right
            }
        }
    }

    private void Add(List<Vector3> tiles)
    {
        int[] triangles = GetTriangles();

        int index = ApplyTriangleIndices(triangles);

        //apply the new triangles for the tiles the user wants
        index = ApplyNewTriangles(tiles, triangles, index);

        mesh.triangles = triangles;
        mesh.RecalculateNormals();
    }

    private static int ApplyNewTriangles(List<Vector3> tiles, int[] triangles, int index)
    {
        for (int i = 0; i < tiles.Count; i++)
        {
            tri = (int)(tiles[i].z * mapWidth + tiles[i].x) * 4;
            triangles[index++] = tri;
            triangles[index++] = tri + 2;
            triangles[index++] = tri + 1;

            triangles[index++] = tri + 2;
            triangles[index++] = tri + 3;
            triangles[index++] = tri + 1;
        }
        return index;
    }

    private int ApplyTriangleIndices(int[] triangles)
    {
        // get the triangle indices from mesh.triangle and apply them to new array
        int index = 0;
        for (int i = 0; i < mesh.triangles.Length; i++)
        {
            trianglesList.AddRange(mesh.triangles);
            triangles[i] = mesh.triangles[i];
            index = i + 1;
        }
        return index;
    }
}
}


PHP MySQLi Connection File I have recently adjusted the way I create my mysqli connections. Initially I would include the connection file and use if($mysqli->connect_error)... in my main script. Now I check if there is an issue with the connection within the connection file. If there isn't an issue then I just check that the variable name for the connection is set. Before I continue I'm curious if there are any issues with the way I am doing this and if it goes against any 'best-practices'.
The contents of my connection file: (connect_db_login.php)
<?php
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
try{
    $test = new mysqli('localhost:3306','login','password','testdb');
    $test->set_charset('utf8');
}catch(Exception $e){ //$e not used; dead code?
    echo('<script> alert("Connection to database failed!");</script>');
    header("refresh:0; url=../login.php"); //using refresh because location doesnt wait for script
}

Checking that the connection has been made in main script:
<?php
    include('../connect_db_login.php'); // using include not require
    if(isset($test)){
    ...

First of all, there are many things in this code which you did right. Setting the proper error reporting mode is brilliant! And always setting the port explicitly is also a very good thing, saving you from many confusions, on Mac especially. Setting the charset is also often forgotten while being very important.
However, too many things are wrong as well - on the conceptual, technological and the implementation levels. To highlight the main problems:

if an error occurs, a web page should never return an OK signal. It should return an error signal instead. There is a thing called HTTP response code and your page should return different codes in different situations. In case of error it's 500, no 200 which your code currently returns.
with this code you just left yourself without the explanation, why the error occurred. The error message is invaluable source of information that could help you to fix the issue, but you are simply ditching it!
writing a dedicated code to handle every distinct error in your script is a waste. Or, to put the other way - why so much attention to the failed database connection? An include could fail as well. Just like there is no special error reporting code for the every include, there shouldn't be dedicated error reporting code for the DB connection as well. If every module would report its errors in its own manner the code would become a complete mess!
redirecting a user to the login page in case of error just makes no sense. What would they do there? An error page should stay where it is.
testing for the connection right after include is simply redundant. Can't your connection code already handle the failure? It can.
database credentials are hardcoded which makes the code less portable

To solve these problems you must separate the error handling from the rest of your code and make it uniform, providing equal treatment for all errors in your application, no matter in which module or in which situation they occur.
Reporting database errors
Do not report them
I do understand where it's coming from. Every [insert your favorite obscene emphasis word here] example on the Net tells you to test for the connection success. So people just habitually reproduce this behavior. Which - if you think of it - is a nonsense. A database error is no different from any other error. If your script says "Oh my, headers already sent!" you don't wrap a header() function call in a try catch. But for some reason a database connection is followed by this honor guard everywhere. Well, just drop this bad habit. Make your database errors equal to all other errors.
Reporting errors in general
I do also understand why the error reporting is so much confused: every PHP apprentice profusely confuses themselves with a site user. No wonder why: during the development, they are the only site user for the long time, and just cannot imagine other people working with the site. But if you just think of it, the difference become apparent - you just need to distinguish a programmer from a site user, as they both require the completely different treatment. A site user doesn't care whether your database connection failed or a hard disk went nuts. All they need to know is that something went wrong but you are already working on it. Then tell them exactly this.
On the other hand, when your site goes live, you don't sit behind every PC browsing your site. This means any specific error information you are displaying will go for naught. On a live site an error must be logged.
Setting the proper charset.
The older default for MySQL, utf8 charset implements only a limited subset of the original UTF-8 standard. Hence, for the full Unicode support, and to avoid "Incorrect string value" errors caused by a mere emoji, always use utf8mb4 charset.
Storing database credentials
Let's digress for a while from the main subject of error reporting and look at the database credentials. The present code is again the direct product of the "I am working alone on my home PC and it's going to be so forever" mindset. When the time comes, your site will go live. And a live site wold definitely have different credentials so you will need to rewrite them. And when you will need to work a bit more on the code, you will have to rewrite them back... and so on. Which is far from being convenient.
There are many advanced techniques for providing the settings but the simplest one would be just storing them in a separate file. This way you'll be able to keep different credentials files on different servers. So instead of hardcoding  the credentials, just use variables defined in a separate file:
<?php
$host = '127.0.0.1';
$port = 3306;
$db   = 'test';
$user = 'root';
$pass = '';
$charset = 'utf8mb4';

and then just include this file in your DB connection script
Implementing all the above.
In theory, given the connection shouldn't report errors by itself, only the following lines should be enough:
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
require __DIR__.'/db_credentials.php';
$mysqli = new mysqli($host, $user, $pass, $db, $port);
$mysqli->set_charset($charset);
unset($host, $db, $user, $pass, $charset); // we don't need them anymore

It will raise an error that would be handled elsewhere.
But given we are employing exceptions here, and every exception contains a stack trace, and a stack trace contains every function parameter used, there is a slight chance that database credentials could be leaked. To prevent even such a small chance, it's better to catch the exception and then throw a brand new one, which will have no stack trace and hence no database credentials to leak:
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
try {
    require __DIR__.'/db_credentials.php';
    $mysqli = new mysqli($host, $user, $pass, $db, $port);
    $mysqli->set_charset($charset);
} catch (\mysqli_sql_exception $e) {
     throw new \mysqli_sql_exception($e->getMessage(), $e->getCode());
} finally {
    unset($host, $db, $user, $pass, $charset);
}

Now if an en error occurs, the exception will contain only the error message but not a stack trace. Note that stack traces are extremely helpful for debugging and here we are ditching it only as a tradeoff between the security and usability. But as a rule you should preserve as much debugging information as possible.
So this is it for the connection. What about error reporting in general?
Everything said above is a gist of my article on PHP error reporting, where I have a complete basic universal error handling solution. This code does everything what was said above: in case of error it sets the appropriate HTTP response code, and then changes its behavior depends on the user's type: for a site user it logs the error while showing only a generic text, but for a programmer it will show the error message on-screen.
I wouldn't copy the exact code here because it constantly gets improved over time and better to get the latest version.
How to actually use it?
Just have three files somewhere in your codebase, error_handler.php, mysqli.php, db_credentials.php. Then require the first two in your application.


Music note class - v2 Version 2 of Music note class

#!/usr/bin/env python3

from string import ascii_uppercase
from string import ascii_lowercase

class Notes:
    octave_number = 7
    low_notes = ascii_uppercase[:octave_number]
    high_notes = ascii_lowercase[:octave_number]
    notes = low_notes + high_notes
    notes_number = range(octave_number * 2)
    notes_dict = dict(zip(notes, notes_number))
    numbers_dict = dict(zip(notes_number, notes))

    def __init__(self, _note):
        if not _note in Notes.notes:
            raise Exception("Not a valid note")
        self.note = _note
        self.note_number = Notes.notes_dict[self.note]

    def add(self, n):
        if (self.note in Notes.high_notes) and (n > 0):
            raise Exception("You can only add to low notes (ABCDEFG)")
        if (self.note in Notes.low_notes) and (n < 0):
            raise Exception("You can only subtract from notes (abcdefg)")
        if(abs(n) > Notes.octave_number):
            raise Exception("Maximum amount of increase or decrease is %d" % Notes.octave_number)

        new_number = self.note_number + n
        return Notes(Notes.numbers_dict[new_number])

Test:

x = Notes("A")
x.add(7).note
x = Notes("b")
x.add(-6).note

Changes:

Removed minus method
Expanded notes to two octaves
add now returns a Notes object

You should be aware of Python's "magic methods" to implement class behaviours, including numerical methods like __add__ and __sub__. This allows you to write e.g. note3 = note1 + note2, rather than note3 = note1.add(note2). There are also methods for display, including __repr__, so you can easily view the instance. Additionally:

the class name should be singular, as this only holds one note, and constants should be UPPERCASE;
it's easier to handle either uppercase or lowercase and convert all input accordingly;
you can access class attributes via self too, which makes inheritance easier;
you check note in __init__, but there's nothing to protect it afterwards; and
number could be calculated based on note as required, rather than stored as duplicate information.

Here's an alternative implementation with those ideas in.

#!/usr/bin/env python3

from string import ascii_uppercase

class Note:

    NOTE_COUNT = 7
    NUMBERS = dict(enumerate(ascii_uppercase[:NOTE_COUNT]))
    NOTES = {v: k for k, v in NUMBERS.items()}

    def __init__(self, note=None, number=None):
        if note is not None and note.upper() in self.NOTES:
            note = note.upper()
        elif number is not None and number in self.NUMBERS:
            note = self.NUMBERS[number]
        else:
            raise Exception("Not a valid note")
        self._note = note

    def __repr__(self):
        return "Note({._note!r})".format(self)

    @property
    def number(self):
        return self.NOTES[self._note]

    @property
    def note(self):
        return self._note

    @note.setter
    def note(self, new_note):
        new_note = new_note.upper()
        if new_note not in self.NOTES:
            raise Exception("Not a valid note")
        self._note = new_note

    def __add__(self, other):
        try:
            num = other.number
        except AttributeError:
            num = other
        return Note(number=((self.number+num)%self.NOTE_COUNT))

    def __sub__(self, other):
        try:
            num = other.number
        except AttributeError:
            num = other
        return Note(number=((self.number-num)%self.NOTE_COUNT))

In use:

>>> B = Note("B")
>>> C = Note("C")
>>> B + C
Note('D') # not sure this addition makes sense, but it works!
>>> B.note = "X"
Traceback (most recent call last):
  File "<pyshell#38>", line 1, in <module>
    B.note = "X"
  File "<pyshell#34>", line 30, in note
    raise Exception("Not a valid note")
Exception: Not a valid note
>>> B.number
1

Now all you need is octaves and accidentals! You should also add some docstrings explaining what the class does and how to use it.


Tree Template Class Implementation for C++ I have not done any parameter checking, however I think the meat of the class is there.  Let me know what you think.

#include <cstddef>
#include <vector>
using namespace std;

template<class T>
class TreeNode {

private:

    T* data;
    TreeNode<T>* parent;
    vector< TreeNode<T>* > children;

public:

    TreeNode(TreeNode<T>* parent, T data);
    ~TreeNode();

    T& getData() const;
    void setData(const T& data);
    void addChild(const T& data);
    void removeChild(const size_t& indx);
    TreeNode<T>* findChild(const T& data) const;
    TreeNode<T>* getChild(const size_t& indx) const;
    TreeNode<T>* getParent() const;
    int getNumChildren() const;

};

template<class T>
TreeNode<T>::TreeNode(TreeNode<T>* parent, T data) : parent(parent) {
    this->data = new T(data);
}

template<class T>
TreeNode<T>::~TreeNode() {
    delete data;
    for(TreeNode<T>* childNode : children)
        delete childNode;
}

template<class T>
T& TreeNode<T>::getData() const {
    return *this->data;
}

template<class T>
void TreeNode<T>::setData(const T& data) {
    *this->data = data;
}

template<class T>
void TreeNode<T>::addChild(const T& data) {
    children.push_back(new TreeNode<T>(this, data));
}

template<class T>
void TreeNode<T>::removeChild(const size_t& indx) {
    children.erase(children.begin()+indx);
}

template<class T>
TreeNode<T>* TreeNode<T>::findChild(const T& data) const {
    for(int i=0; i<children.size(); i++)
        if(children[i]->getData() == data)
            return children[i];
    return NULL;
}

template<class T>
TreeNode<T>* TreeNode<T>::getChild(const size_t& indx) const {
    return children[indx];
}

template<class T>
TreeNode<T>* TreeNode<T>::getParent() const {
    return parent;
}

template<class T>
int TreeNode<T>::getNumChildren() const {
    return children.size();
}

Do not get in the habit of using using namespace std.
Since you're using <cstddef> for size_t, you should make it std::size_t for C++.
Since you define your own destructor, you should also overload the copy constructor and the assignment operator.  This is known as The Rule of Three in C++.  This is especially important in the case of raw pointers as data members as the compiler-provided copy constructor and assignment operator will only copy the pointer (shallow copy), not the data (deep copy).

You should also follow Dieter's advice (in the comments): use T data instead of T* data, and remove thenew/delete.  Use as little manual memory allocation as possible in C++.
You're using a range-based for-loop in the destructor, which must mean you're using C++11.  Bearing this in mind, one change you should make is using nullptr instead of NULL.


Thread pausing/resuming implementation I saw How to wait current thread execution and execute another thread completedly? on StackOverflow, and I answered it there, but would like more eyes on the code I posted.
The requirement is to pause one thread whilst a different thread is running.
Could you please review and comment on this implementation?
public class App {

    public static void main(String[] args) {
        Thread1 thread1 = new Thread1();
        Thread2 thread2 = new Thread2();

        try {
            thread1.start();
            Thread.sleep(500);
            synchronized (thread1) {
                thread1.waiting = true;
                thread2.start();
                thread2.join();
                thread1.waiting = false;
                thread1.notify();
            }
        } catch (Exception e) {
            //TODO actually handle exception
        }
    }
}

Class Thread1.java
public class Thread1 extends Thread {
    boolean waiting = false;

    public void run() {
        testFun1();
    }

    public void testFun1() {
        for (int i = 1; i < 10; i++) {
            synchronized (this) {
                while (waiting) {
                    try {
                        wait();
                    } catch (Exception e) {
                        //TODO Handle exception
                    }
                }
            }
            try {
                Thread.sleep(100);
                System.out.println("From testFun1() = " + i);
            } catch (Exception e) {
                //TODO Handle exception

            }
        }
    }
}

Class Thread2.java
public class Thread2 extends Thread {

    public void run() {
        testFun2();
    }

    public void testFun2() {
        try {
            for (int i = 20; i <= 25; i++) {
                try {
                    Thread.sleep(100);
                } catch (InterruptedException e) {
                    //catch
                }
                System.out.println("From testFun2() = " + i);
            }

        } catch (Exception e) {
            //TODO do something
        }

    }
}

Output:
C:\Java\jdk1.6.0_25\bin\java -Didea.launcher.port=7543 "-Didea.launcher.bin.path=C:\Program Files\JetBrains\IntelliJ IDEA 10.5.1\bin" -Dfile.encoding=UTF-8 -classpath "C:\Java\jdk1.6.0_25\jre\lib\alt-rt.jar;C:\Java\jdk1.6.0_25\jre\lib\alt-string.jar;C:\Java\jdk1.6.0_25\jre\lib\charsets.jar;C:\Java\jdk1.6.0_25\jre\lib\deploy.jar;C:\Java\jdk1.6.0_25\jre\lib\javaws.jar;C:\Java\jdk1.6.0_25\jre\lib\jce.jar;C:\Java\jdk1.6.0_25\jre\lib\jsse.jar;C:\Java\jdk1.6.0_25\jre\lib\management-agent.jar;C:\Java\jdk1.6.0_25\jre\lib\plugin.jar;C:\Java\jdk1.6.0_25\jre\lib\resources.jar;C:\Java\jdk1.6.0_25\jre\lib\rt.jar;C:\Java\jdk1.6.0_25\jre\lib\ext\dnsns.jar;C:\Java\jdk1.6.0_25\jre\lib\ext\localedata.jar;C:\Java\jdk1.6.0_25\jre\lib\ext\sunjce_provider.jar;C:\Java\jdk1.6.0_25\jre\lib\ext\sunmscapi.jar;C:\Java\jdk1.6.0_25\jre\lib\ext\sunpkcs11.jar;C:\IdeaProjects\PracticeModule\target\classes;C:\Program Files\JetBrains\IntelliJ IDEA 10.5.1\lib\idea_rt.jar" com.intellij.rt.execution.application.AppMain jmhp.core.App
From testFun1() = 1
From testFun1() = 2
From testFun1() = 3
From testFun1() = 4
From testFun1() = 5
From testFun2() = 20
From testFun2() = 21
From testFun2() = 22
From testFun2() = 23
From testFun2() = 24
From testFun2() = 25
From testFun1() = 6
From testFun1() = 7
From testFun1() = 8
From testFun1() = 9

Thanks!

The implementation looks fine. Some small notes:

Consider using Guava's Uninterruptibles class if you don't want to handle InterruptedExceptions.
The default access waiting flag in Thread1 is makes the code hard to read/follow and results tight coupling.
Consider using Runnables instead of Threads. See: Java: “implements Runnable” vs. “extends Thread”
Consider synchronizing on a separate lock object: Avoid synchronized(this) in Java?

Anyway, it would be much easier, less error-prone and more readable with a fair Semaphore (from the concurrency utilities package):

App.java:

public class App {

    public static void main(final String[] args) throws InterruptedException {
        final Semaphore control = new Semaphore(1, true);

        final Thread1 thread1 = new Thread1(control);
        final Thread2 thread2 = new Thread2();

        thread1.start();
        Uninterruptibles.sleepUninterruptibly(500, TimeUnit.MILLISECONDS);
        control.acquireUninterruptibly();
        thread2.start();
        thread2.join();
        control.release();
    }
}

Thread1.java:

public class Thread1 extends Thread {
    private final Semaphore control;

    public Thread1(final Semaphore control) {
        this.control = control;
    }

    @Override
    public void run() {
        for (int i = 1; i < 10; i++) {
            control.acquireUninterruptibly();
            try {
                Uninterruptibles.sleepUninterruptibly(100, TimeUnit.MILLISECONDS);
                System.out.println("From testFun1() = " + i);
            } finally {
                control.release();
            }
        }
    }
}

Thread2.java:

public class Thread2 extends Thread {

    public void run() {
        for (int i = 20; i <= 25; i++) {
            Uninterruptibles.sleepUninterruptibly(100, TimeUnit.MILLISECONDS);
            System.out.println("From testFun2() = " + i);
        }

    }
}


Making a disk usage breakdown I'm looking for a better way of doing this code I made. I work for tech support and one of the biggest questions I am asked is: "Hey, how did my disk get so full in my VPS?"

I am aiming for output like:

home/ is taking up xGB
home/user1 xgb
home/user2 xgb

and so on. So far I have this, which does alright, but I am looking for a prettier way of getting this done.

#!/bin/bash
for i in $(ls -d */ |  grep -v proc);
 do
printf "**** $i has the following breakdown ********\n"

du -h --max-depth=1 $i
done

Use 'shellcheck' to spot common problems

200236.sh:2:12: warning: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames. [SC2010]
200236.sh:2:18: note: Use ./*glob* or -- *glob* so names with dashes won't become options. [SC2035]
200236.sh:4:8: note: Don't use variables in the printf format string. Use printf "..%s.." "$foo". [SC2059]
200236.sh:4:53: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
200236.sh:6:21: note: Double quote to prevent globbing and word splitting. [SC2086]

Don't parse the output of ls

The ls program works well for interactive use, but isn't designed to be parsed by scripts.  The main problem is that whitespace and other shell-significant characters are shown as-is, with no quoting.  You could try to work around this using ls -b, but it's more robust to avoid the problem altogether:

for i in */
do
  if [[ i =~ proc ]]; then continue; fi
  # ...
done

The test for virtual filesystems is too broad

grep proc will pick up names such as processor - do you really want to exclude them?  If you want to avoid inspecting filesystems not backed by disks, there are more reliable means:

case $(stat --file-system --format '%T' "$i") in
  proc|tmpfs|sysfs) continue ;;
esac

Always quote parameter expansions

du -h --max-depth=1 "$i"
#                   ^^^^

Don't expand parameters into a format string

Use echo or printf, but don't confuse the two ($i may contain %):

echo "**** $i has the following breakdown ********"

printf '**** %s has the following breakdown ********\n' "$i"

Constrain du to a single filesystem

I'm not sure whether or not you want du to cross mountpoints.  If not, then add -x / --one-file-system to its options.

Modified code

#!/bin/bash
for i in */
do
    case $(stat --file-system --format '%T' "$i") in
        proc|tmpfs|sysfs) continue ;;
    esac

    echo "**** $i has the following breakdown ********"
    du -h --one-file-system --max-depth=1 "$i"
done


Rails form helper to display email fields and their validation errors I have a repeated pattern throughout my app that looks like this:

<div class="form-group floating-label <%= f.object.errors[:to].any?? "has-error has-feedback" : "" %>">
  <% if f.object.errors[:to].any? %>
    <%= f.label f.object.errors[:to].join(", ") %>
  <% end %>
  <%= f.email_field :to, class: "form-control" %>
  <%= f.label :to %>
</div>

The only thing that changes is the attribute. Instead of the :to attribute, it might be something else. But everything else will remain the same.

I extracted it out into a helper method:

_form.html.erb

  <%= field_view "To", f.email_field(:to, class: "form-control") %>

form_helper.rb

  def field_view(label, *args, &block)
    # label = label.t internalization

    options = args.extract_options!
    field = args.first
    formatter = args.second

    data = capture(&block) if block_given?

    options[:class] = "form-group floating-label #{options[:class]}".strip

    element = formatter ? send(formatter, field) : field

    make_content label, element, options
  end

  def make_content(label, element, options)
    content_tag :div, options do
      element + content_tag(:label, label)
    end
  end

I do not want my form_helper to know anything about the field. Because sometimes it will be f.text_field :to, text_field_tag :to, f.select, etc. Too much change happening to make the form_helper aware of it.

What approach would you suggest in order to incorporate that error checking functionality which is shown in the first example?

I would try this (naming needs improvement):

*_helper.rb:

  def field_wrapper(errors, &block)
    css_classes = %w(form-group floating-label)
    css_classes << 'has-error has-feedback' if errors.any?
    content_tag :div, class: css_classes, &block
  end

  def error_label(form, errors)
    form.label errors.join(', ') if errors.any?
  end

*.html.erb:

 <%= field_wrapper f.object.errors[:to] do %>
      <%= error_label f, f.object.errors[:to] %>
      <%= f.email_field :to, class: "form-control" %>
      <%= f.label :to %>
   <% end %>


Finding the length of the longest common substring from two given strings in Java The following code returns the length of longest common substring from given two strings in Java. For cases of around \$10^6\$ characters, it is still not fast enough to get the answer in a few seconds.

Any suggestions for efficiency will be greatly appreciated!

int longestCommonSubstring(String s, String t){
    int maxlength = 0;
    for(int i=0; i<s.length(); i++){
        for(int j=0; j<t.length(); j++){
            int count = 0;
            int g = 0;
            while(i+g<s.length() && j+g<t.length()){
                if(t.charAt(j+g)==s.charAt(i+g)){
                    count++;
                    maxlength = Integer.max(count, maxlength);
                }
                else{
                    count = 0;
                }
                g++;
            }
        }
    }
    return maxlength;
}

Stylistic review

You really need some whitespace here and there. Please put a space between operators and between keywords / brackets, etc. For example:

    for(int i=0; i<s.length(); i++){
        for(int j=0; j<t.length(); j++){

This would be more readable:

    for (int i = 0; i < s.length(); i++) {
        for (int j = 0; j < t.length(); j++) {

You also could use better variable names. s and t aren't very obvious, and neither is g.

Overall, the code could potentially be more readable if it could be restructured into using some of the Stream algorithms, but that's not an easy task.

Performance review

The easiest solutions to this problem take \$O(nm)\$ time, where \$n\$ and \$m\$ are the lengths of the 2 input strings. Your algorithm is slower than this. Here are some places that slow it down:

You keep incrementing g even once the strings become dissimilar. You want to break out of the while loop once t.charAt(j + g) != s.charAt(i + g). Because we keep incrementing i and j, you don't have to worry about missing later common substrings.
If s = "abcdefg" and t = "abcdefg", one iteration of the inner for loop will find the correct maxLength. However, you then increment j and have to iterate over bcdefg again with the inner while loop even though we know that bcdefg is a shorter substring than abcdefg. You can keep your count variable from the inner while and use that to skip forward in j, at the very least.
This isn't an algorithmic concern, but you compute Integer.max on every single char, but you only need to do it once you find a complete substring. This can be accomplished by moving that line into the else block (before the count = 0, which can be removed if you follow my first point).

Those are the easy performance improvements I can see. If you still need to speed up your code, the easiest thing to do would probably be to redesign your algorithm.

One of the more powerful techniques in algorithm design is known as dynamic programming. It is basically just breaking down the problem into smaller pieces, then combining it together. For example, if it was possible to find the largest common substring (LCS) of the left and right halves of s and t, then use that information to find the LCS of the whole thing, that would be solving the problem through dynamic programming.

Wikipedia helpfully has a page on this problem. For a dynamic programming solution, it shows that you can solve this by taking all the possible prefixes of the strings, and finding the longest common suffixes for each pair of possible prefixes. If that algorithm isn't efficient enough, it helpfully provides another one which is faster.


Disable execution of parts of the code based on user's choice C++ I am working on the small program for data acquisition where the data is written to log file and the clients can listen to data broadcasted over socket.

What I would like to achieve is to give the user some control over the program, to decide, if the data logging or the socket server should be started or not, for now I am using the console arguments for those user-program interaction. The problem is that if the user decides not to use the socket server, then multiple lines of the code in different parts of the program should not be executed (the socket object should not be constructed and non of it's methods should be executed). This easily leads to the problems, as all executions of the methods need to have the constraints. If I forget any, it leads to problems

Here you can see my actual implementation of main, which checks the user's inputs and executes code accordingly:

int main(int argc, char *argv[])
{
    std::string port = "27013"; // default port for socket, used if not specified different in console argument
    bool socketsActivated = true; // default value
    bool logActivated = true; // default value
    std::unique_ptr<ServerCommunication> server; // socket broadcaster object
    std::unique_ptr<ServerEcho> echo; // socket echoing object
    std::unique_ptr<FileOperations> traceFile; // Trace logging object

    for (int i = 1; i < argc; ++i) {
        if (std::string(argv[i]) == "--port") {
            if (i + 1 < argc) { // Make sure we aren't at the end of argv!
                port = std::string(argv[i++]); // Increment 'i' so we don't get the argument as the next argv[i].
            }
            else { // There was no port argument
                std::cerr << "--port option requires one argument." << std::endl;
                return 1;
            }
        }
    }

    for (int i = 1; i < argc; ++i) {
        if (std::string(argv[i]) == "--nosocket")
            socketsActivated = false;
    }

    for (int i = 1; i < argc; ++i) {
        if (std::string(argv[i]) == "--nolog")
            logActivated = false;
    }

    if (socketsActivated){
        server= std::make_unique<ServerEcho>(port);
        echo = std::make_unique<ServerEcho>("27014");
    }

    if (logActivated)
        traceFile.reset(new FileOperations());

    while (!_kbhit()) // Till the key was pressed
    {
        Sleep(25); // lower the load on CPU

        if (socketsActivated){
            server->update();
            echo->update();
        }
    }
    return 0;
}

It is error-prone as if the code will get more complex additional parts of the program will need to be nested if the case requires, what will easily leads to problems...

Is there a better, easier approach for it? The preprocessor directives will not help here.

I see two things:

option acquisition (a lot of control flow and ad-hoc checks to detect the user input)
operational program flow (the decisions based on it). Here I'd suggest Null Object pattern

Acquisition

At first, I'd suggest using standard library algorithms and types instead of ad-hoc program flow and casting:

std::vector<std::string> const args(argv+1, argv+argc);

Which you can use much easier, like:

auto find_arg    = [&](auto option) { return std::find(args.begin(), args.end(), option); };

So now you can just locate an argument using find_arg("--port"). And you can build on it:

auto find_arg    = [&](auto option) { return std::find(args.begin(), args.end(), option); };
auto has_flag    = [&](auto option) { return args.end() != find_arg(option); };

Now you can simply do:

bool const socketsActivated = !has_flag("--nosocket");
bool const logActivated     = !has_flag("--nolog");

Note that these are immutable, so program logic is strictly simpler and more verifiable. Getting the port takes a little more effort, but can be supported generically too:

auto get_option  = [&](auto option, auto default_value) -> std::string {
    auto it = find_arg(option);
    if (it != args.end()) {
        it = ++it;
        if (it == args.end()) { // There was no argument
            throw std::runtime_error(std::string(option) + " requires an argument");
        }
        return *it;
    }
    return default_value;
};

So now you can write:

int main(int argc_, char *argv_[])
{
    std::vector<std::string> const args(argv_+1, argv_+argc_);

    auto find_arg    = [&](auto option) { return std::find(args.begin(), args.end(), option); };
    auto has_flag    = [&](auto option) { return args.end() != find_arg(option); };
    auto get_option  = [&](auto option, auto default_value) -> std::string {
        auto it = find_arg(option);
        if (it != args.end()) {
            it = ++it;
            if (it == args.end()) { // There was no argument
                throw std::runtime_error(std::string(option) + " requires an argument");
            }
            return *it;
        }
        return default_value;
    };

    std::string const port      = get_option("--port", "27013");
    bool const socketsActivated = !has_flag("--nosocket");
    bool const logActivated     = !has_flag("--nolog");

It's more legible, less error prone and very extensible.

The extra mile

When you find yourself collecting operations like has_flag, get_option you can move them to a suitable abstraction.

Here's exactly the same data + operations grouped into a CommandLineParser:

struct CommandLineParser {
    CommandLineParser(int argc, char** argv) : _args(argv+1, argv+argc) {}

    auto find_arg  (std::string const& option) const {
        return std::find(_args.begin(), _args.end(), option);
    };
    bool has_flag  (std::string const& option) {
        return _args.end() != find_arg(option);
    };
    std::string get_option(std::string const& option, std::string const& default_value) {
        auto it = find_arg(option);
        if (it != _args.end()) {
            it = ++it;
            if (it == _args.end()) { // There was no port argument
                throw std::runtime_error(option + " requires an argument");
            }
            return *it;
        }
        return default_value;
    };

    std::vector<std::string> const _args;
};

And now main starts like this:

int main(int argc, char *argv[])
{
    CommandLineParser clp(argc, argv);
    std::string const port      = clp.get_option("--port", "27013");
    bool const socketsActivated = !clp.has_flag("--nosocket");
    bool const logActivated     = !clp.has_flag("--nolog");

Clean as a whistle.

Null Object Pattern

Here you prevent having null objects or repeated switches by having different implementations of the same interface. You just instantiate a "do-nothing" implementation if the behaviour is disabled:

/////////////////////////////////////
// abstract interface and null-implementation
struct IServer {
    virtual void update() = 0;
};

struct NullServer : IServer {
    virtual void update() override { }; // do nothing
};

struct CommunicationServer : IServer {
    CommunicationServer(std::string) {}
    virtual void update() override {
        std::clog << "Communication update\n";
    }
};

struct EchoServer : IServer {
  EchoServer(std::string) {}
    virtual void update() override {
        std::clog << "Echo update\n";
    }
};

Now you can simplify the call - site:

int main(int argc, char *argv[])
{
    std::unique_ptr<IServer>          server(new NullServer);
    std::unique_ptr<IServer>          echo(new NullServer);
    std::unique_ptr<IFileOperations>  traceFile(new NullFileOperations);

    // parse user config
    CommandLineParser clp(argc, argv);

    if (!clp.has_flag("--nosocket")){
        server = std::make_unique<CommunicationServer>(clp.get_option("--port", "27013"));
        echo   = std::make_unique<EchoServer>("27014");
    }

    if (!clp.has_flag("--nolog")) {
        traceFile = std::make_unique<FileOperations>();
    }

    while (!_kbhit()) // Till the key was pressed
    {
        Sleep(25); // lower the load on CPU

        server->update();
        echo->update();
    }
}

Full Demo

Live On Coliru

#include <string>
#include <iostream>
#include <algorithm>
#include <vector>
#include <memory>
#include <thread>

// mocks
static void Sleep(int ms) {
    std::this_thread::sleep_for(std::chrono::milliseconds(ms));
}

static bool _kbhit() {
    static int remain = 10;
    return remain-- == 0;
}

/////////////////////////////////////
// abstract interface and null-implementation
struct IServer {
    virtual void update() = 0;
};

struct NullServer : IServer {
    virtual void update() override { }; // do nothing
};

struct CommunicationServer : IServer {
    CommunicationServer(std::string) {}
    virtual void update() override {
        std::clog << "Communication update\n";
    }
};

struct EchoServer : IServer {
  EchoServer(std::string) {}
    virtual void update() override {
        std::clog << "Echo update\n";
    }
};

/////////////////////////////////////
// similar for other interfaces
struct IFileOperations { };
struct NullFileOperations : IFileOperations { };
struct FileOperations : IFileOperations {};

struct CommandLineParser {
    CommandLineParser(int argc, char** argv) : _args(argv+1, argv+argc) {}

    auto find_arg  (std::string const& option) const {
        return std::find(_args.begin(), _args.end(), option);
    };
    bool has_flag  (std::string const& option) {
        return _args.end() != find_arg(option);
    };
    std::string get_option(std::string const& option, std::string const& default_value) {
        auto it = find_arg(option);
        if (it != _args.end()) {
            it = ++it;
            if (it == _args.end()) { // There was no port argument
                throw std::runtime_error(option + " requires an argument");
            }
            return *it;
        }
        return default_value;
    };

    std::vector<std::string> const _args;
};

int main(int argc, char *argv[])
{
    std::unique_ptr<IServer>          server(new NullServer);
    std::unique_ptr<IServer>          echo(new NullServer);
    std::unique_ptr<IFileOperations>  traceFile(new NullFileOperations);

    // parse user config
    CommandLineParser clp(argc, argv);

    if (!clp.has_flag("--nosocket")){
        server = std::make_unique<CommunicationServer>(clp.get_option("--port", "27013"));
        echo   = std::make_unique<EchoServer>("27014");
    }

    if (!clp.has_flag("--nolog")) {
        traceFile = std::make_unique<FileOperations>();
    }

    while (!_kbhit()) // Till the key was pressed
    {
        Sleep(25); // lower the load on CPU

        server->update();
        echo->update();
    }
}

Which, without options, prints e.g.

Communication update
Echo update
Communication update
Echo update
Communication update
Echo update
Communication update
Echo update
Communication update
Echo update
Communication update
Echo update
Communication update
Echo update
Communication update
Echo update
Communication update
Echo update
Communication update
Echo update

But with --nosocket prints nothing, and with missing port prints:

terminate called after throwing an instance of 'std::runtime_error'
  what():  --port requires an argument


Simple shell script to backup to Amazon over iSCSI The script works but I know that it can be improved. Please note that my scripting skills are very basic :) (as it can be seen in my Frankenstein script – I took bits and pieces of the code from different tutorials).

I would appreciated if someone could review this script and identify some major flaws.

Some other questions that I have:

After the backup is complete is it necessary/better to logout from the amazon and unmount the volume?
Is it better to check if the volume is mounted based on the UUID?

#!/bin/bash
##
## VARIABLES
##
BACKUP_FROM="/folder/"
BACKUP_TO="/folder/"
LOG_FILE="/var/log/backup.log"
SCSI="/sbin/iscsiadm"
VOL="ghrryhfd-56655565-456456-3453-dfhthjtrrgg"
##
## SCRIPT
##
# Check that the log file exists
if [ ! -e "$LOG_FILE" ]; then
    touch "$LOG_FILE"
fi

# Check if the volume is mounted if not login to amazon and mount
if mountpoint -q /mount_folder; then
echo "$(date "+%Y-%m-%d %k:%M:%S") - Volume mounted" >> "$LOG_FILE"
else
echo "$(date "+%Y-%m-%d %k:%M:%S") - Mounting!" >> "$LOG_FILE"
$SCSI --mode node --targetname abc.222-99.com.amazon:volume_name --portal 1.1.1.1:9999 --login
sleep 10
mount -U "$VOL" /mount_folder
sleep 8
fi

# Start entry in the log
echo "$(date "+%Y-%m-%d %k:%M:%S") - Sync started." >> "$LOG_FILE"

# Start sync
if rsync -a -v "$BACKUP_FROM" "$BACKUP_TO" &>> "$LOG_FILE"; then
    echo "$(date "+%Y-%m-%d %k:%M:%S") - Sync completed succesfully." >> "$LOG_FILE"
else
    echo "$(date "+%Y-%m-%d %k:%M:%S") - ERROR: rsync-command failed." >> "$LOG_FILE"
    echo "$(date "+%Y-%m-%d %k:%M:%S") - ERROR: Unable to sync." >> "$LOG_FILE"
    echo "" >> "$LOG_FILE"
    exit 1
fi

# End entry in the log
echo "" >> "$LOG_FILE"
exit 0

Extract common logic to a helper function

Notice the similarity in these statements:

echo "$(date "+%Y-%m-%d %k:%M:%S") - Volume mounted" >> "$LOG_FILE"
...
echo "$(date "+%Y-%m-%d %k:%M:%S") - Mounting!" >> "$LOG_FILE"
...
echo "$(date "+%Y-%m-%d %k:%M:%S") - Sync started." >> "$LOG_FILE"

They all have the same pattern:

echo "$(date "+%Y-%m-%d %k:%M:%S") - MESSAGE" >> "$LOG_FILE"

So it's a good candidate to turn into a function:

log() {
    echo "$(date "+%Y-%m-%d %k:%M:%S") - $@" >> "$LOG_FILE"
}

So that you can simplify the uses like this:

log "Volume mounted"
...
log "Mounting!"
...
log "Sync started."

Result of mount

The mount command might fail. When that happens, you might want to abort:

if ! mount -U "$VOL" /mount_folder; then
    log "Unable to Mount"
    exit 1
fi

Pointless touch

This code is completely unnecessary.
Whether the file existed or not, whether you touch it or not,
it won't affect the rest of the script.
You can safely remove it.

if [ ! -e "$LOG_FILE" ]; then
    touch "$LOG_FILE"
fi

Simplify

This is kind of tedious:

echo "" >> "$LOG_FILE"

You can just drop the "" to simplify:

echo >> "$LOG_FILE"

And, you don't need exit 0 at the end of the script.


Create a complete binary tree Write a program to create a complete binary tree, which includes an addItem method such that an individual element can be inserted into the tree to maintain the property that tree is complete.

I'm looking for code review, optimizations and best practices. This code is attributed to GeeksForGeeks.

I am also not sure how to unit test this code. More specifically, how to generate the input to equals function.  The tree I would compare would be using the same addItem method which is destroying the whole purpose of unit testing.

public class CreateCompleteBinaryTree<T> {

    private TreeNode<T> root;

    public CreateCompleteBinaryTree(List<T> items) {
        for (T item : items) {
            addItem(item);
        }
    }

    private static class TreeNode<T> {
        private TreeNode<T> left;
        private T item;
        private TreeNode<T> right;

        TreeNode(T item) {
            this.item = item;
        }
    }

    /**
     * Adds item, such the tree is a complete binary tree.
     *
     * @param item the item to be added.
     */
    public void addItem(T item) {

        System.out.println(item);

        if (root == null) {
            root = new TreeNode<T>(item);
            return;
        }

        final Queue<TreeNode<T>> queue = new LinkedList<>();
        queue.add(root);

        while (!queue.isEmpty()) {
            final TreeNode<T> currNode = queue.poll();

            if (currNode.left == null) {
                currNode.left = new TreeNode<T>(item);
                return;
            }

            if (currNode.right == null) {
                currNode.right = new TreeNode<T>(item);
                return;
            }

            queue.add(currNode.left);
            queue.add(currNode.right);
        }
    }

    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result + ((root == null) ? 0 : root.hashCode());
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (getClass() != obj.getClass())
            return false;
        CreateCompleteBinaryTree other = (CreateCompleteBinaryTree) obj;
        return equal(root, other.root);
    }

    private static boolean equal(TreeNode node1, TreeNode node2) {

        if (node1 == null && node2 == null) return true;
        if (node1 == null || node2 == null) return false;
        if (node1.item != node2.item) {
            System.out.println("---: " + node1.item + " : " + node2.item);
            return false;
        }

        return equal(node1.left, node2.left) && equal(node1.right, node2.right);
    }
}

There are two major issues I see with your implementation, one functional, and one performance.

You have implemented the equals() method and, additionally, you have implemented the hashCode() method too. It is good that you are implementing them both, but, they are broken.

The hashCode() method only calculates the hashCode of the root node (if any), and it does not calculate it off the value in the node, but off the node itself.

@Override
public int hashCode() {
    final int prime = 31;
    int result = 1;
    result = prime * result + ((root == null) ? 0 : root.hashCode());
    return result;
}

You do not override the hashCode of TreeNode, so the result is just a somewhat unique number, and thus, even if you have two trees with the same value at the root, they will have different hashCode values. If you override equals, you have to override hashCode, but, if you override hashCode, do it properly!

The Performance issue relates to the core addItem method. Your method is an \$O(n)\$ operation, scanning almost the entire tree, to get to the last item added.

A better algorithm would be to maintain a stack of the path to the most recently added node's parent. This would be maintained at the same time as the root node.... consider:

Deque<TreeNode<T>> insertPoint = new LinkedList<>();
TreeNode root = null;

Then, when you add an item:

/**
 * Adds item, such the tree is a complete binary tree.
 *
 * @param item the item to be added.
 */
public void addItem(T item) {

    if (root == null) {
        root = new TreeNode<T>(item);
        return;
    }

    if (!insertedAt.isEmpty() && insertedAt.peek().right != null) {
        TreeNode<T> kid = insertedAt.pop();
        while (!insertedAt.isEmpty() && kid == insertedAt.peek().right) {
            kid = insertedAt.pop();
        }
        if (!insertedAt.isEmpty()) {
            kid = insertedAt.peek();
            kid = kid.right;
            insertedAt.push(kid);
            while (kid.left != null) {
                kid = kid.left;
                insertedAt.push(kid);
            }
        }
    }

    if (insertedAt.isEmpty()) {
        // filled the leaf row, go down the left edge again.
        TreeNode<T> node = root;
        while (node != null) {
            insertedAt.push(node);
            node = node.left;
        }
    }

    TreeNode<T> node = insertedAt.peek();
    TreeNode<T> toadd = new TreeNode<>(item);
    if (node.left == null) {
        node.left = toadd;
    } else {
        node.right = toadd;
    }

}

What the above code does, is keep the most recently added-to node at the top of the stack. Then, if the node can still be added to, it fills it.

If the node is now full, it goes back up the tree, looking for the next insertion point, which may nto exist on this level. If it does not then it goes all the way down the left edge and starts a new level....

This means that, for many inserts (half of them?), the performance is \$O(1)\$ and for the worst case, when you have just completed a level of the tree, and you have to start over again at the left, it requires one walk up the right edge, and back down the left edge, in an \$O(\log{n})\$ operation.

So, this brings the process from a \$O(n)\$ operation with \$O(\log{n})\$ space complexity, to a worst-case \$O(\log{n})\$ operation with \$O(\log{n})\$ space as well.


Comparing two columns in two different rows I want to go through each line of the a .csv file and compare to see if the first field of line 1 is the same as first field of next line and so on. If it finds a match then I would like to ignore those two lines that contains the same fields and keep the lines where there is no match.

Here is an example dataset (no_dup.txt):

Ac_Gene_ID  M_Gene_ID
ENSGMOG00000015632  ENSORLG00000010573
ENSGMOG00000015632  ENSORLG00000010585
ENSGMOG00000003747  ENSORLG00000006947
ENSGMOG00000003748  ENSORLG00000004636

Here is the output that I wanted:

Ac_Gene_ID  M_Gene_ID
ENSGMOG00000003747  ENSORLG00000006947
ENSGMOG00000003748  ENSORLG00000004636

Here is my code that works, but I want to see how it can be improved:

import sys

in_file = sys.argv[1]
out_file = sys.argv[2]

entries = {}
entries1 = {}

with open(in_file, 'r') as fh_in:
    for line in fh_in:
        if line.startswith('E'):
            line = line.strip()
            line = line.split()
            entry = line[0]
            if entry in entries:
                entries[entry].append(line)
            else:
                entries[entry] = [line]

with open('no_dup_out.txt', 'w') as fh_out:
    for kee, val in entries.iteritems():
        if len(val) == 1:
            fh_out.write("{} \n".format(val))

with open('no_dup_out.txt', 'r') as fh_in2:
    for line in fh_in2:
        line = line.strip()
        line = line.split()
        entry = line[1]
        if entry in entries1:
            entries1[entry].append(line)
        else:
            entries1[entry] = [line]

with open(out_file, 'w') as fh_out2:
     for kee, val in entries1.iteritems():
        if len(val) == 1:
            fh_out2.write("{} \n".format(val))

The output that I am getting:

[["[['ENSGMOG00000003747',", "'ENSORLG00000006947']]"]]
[["[['ENSGMOG00000003748',", "'ENSORLG00000004636']]"]]

This part

        if entry in entries:
            entries[entry].append(line)
        else:
            entries[entry] = [line]

definitly smells like it could/should be written with setdefault or defaultdict.

This would be for instance entries.setdefault(entry, []).append(line).

Avoid to re-assign the same variable again and again as it makes it harder to hard to understand what the variable is supposed to represent.

        line = line.strip()
        line = line.split()

could be written : splitted_list = line.strip().split()

You are iterating over key ("kee"?)/values of a dictionnary but ignoring the actual key.

The convention is to use _ as the variable name for throw-away values so you could write : for _, val in entries.iteritems():. However, it would probably be better to just iterate over the values using itervalues, values or viewvalues.


How to differentiate web fonts font-face font-weights? What do you guys think:

OPTION #1 (same font-face name and change font-weight)

@font-face {
  font-family: ProximaNova;
  src: url('/fonts/ProximaNova-Regular.ttf');
  font-weight: normal;  // THIS IS NORMAL
  font-style: normal;
}
@font-face {
  font-family: ProximaNova;
  src: url('/fonts/ProximaNova-Semibold.ttf');
  font-weight: bold;   // THIS IS SEMI-BOLD
  font-style: normal;
}
@font-face {
  font-family: ProximaNova;
  src: url('/fonts/ProximaNova-Bold.ttf');
  font-weight: 900;   // THIS IS BOLD
  font-style: normal;
}

Then all my elements would use the font-family: ProximaNova and I would control the weight with the font-weight property, using bold and 900 values.

OPTION #2 (change the font-face name itself to control weight):

@font-face {
  font-family: ProximaNova;
  src: url('/fonts/ProximaNova.ttf');
  font-weight: normal;  // ALWAYS NORMAL
  font-style: normal;
}
@font-face {
  font-family: ProximaNovaSemiBold;   // THIS IS SEMIBOLD
  src: url('/fonts/ProximaNova-Semibold.ttf');
  font-weight: normal;  // ALWAYS NORMAL
  font-style: normal;
}
@font-face {
  font-family: ProximaNovaBold;       // THIS IS BOLD
  src: url('/fonts/ProximaNova-Bold.ttf');
  font-weight: normal;  // ALWAYS NORMAL
  font-style: normal;
}

Then I would control the weight by selecting different font-family names, like ProximaNova, ProximaNovaSemiBold and ProximaNovaBold and the font-weight would always be normal.

What would you go for?

Always option 1. That way it will correctly use bold in cases when the browser doesn't support or can't download the fonts.

EDIT: One more point I've noticed just now: Don't use bold (700) for "semi-bold". Most font creators suggest a font-weight value for each variant. Usually it's 600 for "semi-bold" (and 500 for "medium").


Adding pins onto a Google map I have the following block of code which adds pins including descriptions and links onto a Google map. How can I refactor it so that I can add pins easier and without so much redundant code?

var map;

function initializeMap() {
var myOptions = {
    zoom: 9,
    center: new google.maps.LatLng(42.2340464046899, -71.0956621170044),
    mapTypeId: google.maps.MapTypeId.TERRAIN,
    scrollwheel: false
};
map = new google.maps.Map(document.getElementById('map_canvas'), myOptions);

var marker1 = new google.maps.Marker({
    position: new google.maps.LatLng(42.2340464046899, -71.0956621170044),
    map: map,
    title: "Pin 1",
    url: "#tabs-pin1"
});
google.maps.event.addListener(marker1, 'click', function() {
    $("#tabs").tabs('select', marker1.url);
});
var marker2 = new google.maps.Marker({
    position: new google.maps.LatLng(41.9584457, -70.6672621),
    map: map,
    title: "Pin 2",
    url: "#tabs-pin2"
});
google.maps.event.addListener(marker2, 'click', function() {
    $("#tabs").tabs('select', marker2.url);

var marker9 = new google.maps.Marker({
    position: new google.maps.LatLng(42.445921, -71.2690294),
    map: map,
    title: "Pin 9",
    url: "#tabs-pin9"
});
google.maps.event.addListener(marker9, 'click', function() {
    $("#tabs").tabs('select', marker9.url);

}
google.maps.event.addDomListener(window, 'load', initializeMap);

The simplest solution is extract one function that will form the marker from the parameters and them call it as many times as markers count is:

var markers = [];    // probably you don't need this array
function CreateMarker(lat, lng, markerTitle, markerUrl)
{
var marker = new google.maps.Marker({
    position: new google.maps.LatLng(lat, lng),
    map: map,
    title: markerTitle,
    url: markerUrl
});
google.maps.event.addListener(marker, 'click', function() {
    $("#tabs").tabs('select', marker.url)
});

markers.push(marker); // you can omit this step if you are not going to work with markers after
}

function initializeMap() {
var myOptions = {
    zoom: 9,
    center: new google.maps.LatLng(42.2340464046899, -71.0956621170044),
    mapTypeId: google.maps.MapTypeId.TERRAIN,
    scrollwheel: false
};

CreateMarker(42.2340464046899, -71.0956621170044, "Pin 1", "#tabs-pin1");
// repeat this call for every marker
}


Using Gibbs sampling to segment an image I have been reading some NumPy guides but can't seem to figure it out. My TA told me I should be able to speed up my code by using a NumPy array instead of a for loop in the following segment of code.

for neighbor in get_neighbors(estimates,i,j):
            pXgivenX_ *= edge_model(True,neighbor)*observation_model(obs,True)
            pX_givenX_  *= edge_model(False,neighbor)*observation_model(obs,False)

Here is the entire code of the method it is in:

def gibbs_segmentation(image, burnin, collect_frequency, n_samples):
"""
Uses Gibbs sampling to segment an image into foreground and background.

Inputs
------
image : a numpy array with the image. Should be Nx x Ny x 3
burnin : Number of iterations to run as 'burn-in' before collecting data
collect_frequency : How many samples in between collected samples
n_samples : how many samples to collect in total

Returns
-------
A distribution of the collected samples: a numpy array with a value between
0 and 1 (inclusive) at every pixel.
"""
(Nx, Ny, _) = image.shape
total_iterations = burnin + (collect_frequency * (n_samples - 1))
pixel_indices = list(itertools.product(xrange(Nx),xrange(Ny)))
# The distribution that you will return
distribution = np.zeros( (Nx, Ny) )

# Initialize binary estimates at every pixel randomly. Your code should
# update this array pixel by pixel at each iteration.
estimates = np.random.random( (Nx, Ny) ) > .5

# PreProcessing
preProObs = {}
for (i,j) in pixel_indices:
        preProObs[(i,j)] = []
        preProObs[(i,j)].append(observation_model(image[i][j],False))
        preProObs[(i,j)].append(observation_model(image[i][j],True))

for iteration in xrange(total_iterations):

    # Loop over entire grid, using a random order for faster convergence
    random.shuffle(pixel_indices)

    for (i,j) in pixel_indices:

        pXgivenX_ = 1
        pX_givenX_ = 1

        for neighbor in get_neighbors(estimates,i,j):
            pXgivenX_ *= edge_model(True,neighbor)*preProObs[(i,j)][1]
            pX_givenX_  *= edge_model(False,neighbor)*preProObs[(i,j)][0]
        estimates[i][j] = np.random.random() > pXgivenX_/(pXgivenX_+pX_givenX_)

    if iteration > burnin and (iteration-burnin)%collect_frequency == 0:
        distribution += estimates

return distribution / n_samples

def edge_model(label1, label2):
    """
    Given the values at two pixels, returns the edge potential between
    those two pixels.

    Hint: there might be a more efficient way to compute this for an array
    of values using numpy!
    """
    if label1 == label2:
        return ALPHA
    else:
        return 1-ALPHA

Since this is homework, I won't give you the exact answer, but here are some hints.

== is overloaded in numpy to return an array when you pass in an array. So you can do things like this:

>>> numpy.arange(5) == 3
array([False, False, False,  True, False], dtype=bool)
>>> (numpy.arange(5) == 3) == False
array([ True,  True,  True, False,  True], dtype=bool)

You can use boolean arrays to assign to specific locations in an array. For example:

>>> mostly_true = (numpy.arange(5) == 3) == False
>>> empty = numpy.zeros(5)
>>> empty[mostly_true] = 5
>>> empty
array([ 5.,  5.,  5.,  0.,  5.])

You can also negate boolean arrays; together, these facts allow you to conditionally assign values to an array. (numpy.where can be used to do something similar.):

>>> empty[~mostly_true] = 1
>>> empty
array([ 5.,  5.,  5.,  1.,  5.])

You can then multiply those values by other values:

>>> empty * numpy.arange(5)
array([  0.,   5.,  10.,   3.,  20.])

And many different numpy functions (really ufuncs) provide a reduce method that applies the function along the entire array:

>>> results = empty * numpy.arange(5)
>>> numpy.multiply.reduce(results)
0.0

You should be able to completely eliminate that for loop using only the above techniques.


Simple memory pool using no extra memory I've borrowed the idea from the internet and I would like to know if my implementation is all right and what could be improved.

It uses the free memory to store links to each node, so there's no extra memory being used.

memory_pool.h

#ifndef MEMORY_POOL_H
#define MEMORY_POOL_H

#include <stdlib.h>

#define MEMORY_POOL_SUCCESS 1
#define MEMORY_POOL_ERROR 0
#define MEMORY_POOL_MINIMUM_SIZE sizeof(void *)

typedef struct {
    void **head;
    void *memory;
} Memory_Pool;

//size must be greater than or equal to MEMORY_POOL_MINIMUM_SIZE
int mp_init(Memory_Pool *mp, size_t size, size_t slots);
void mp_destroy(Memory_Pool *mp);

void *mp_get(Memory_Pool *mp);
void mp_release(Memory_Pool *mp, void *mem);

#endif

memory_pool.c

#include "memory_pool.h"

int mp_init(Memory_Pool *mp, size_t size, size_t slots)
{
    //allocate memory
    if((mp->memory = malloc(size * slots)) == NULL)
        return MEMORY_POOL_ERROR;

    //initialize
    mp->head = NULL;

    //add every slot to the list
    char *end = (char *)mp->memory + size * slots;
    for(char *ite = mp->memory; ite < end; ite += size)
        mp_release(mp, ite);

    return MEMORY_POOL_SUCCESS;
}

void mp_destroy(Memory_Pool *mp)
{
    free(mp->memory);
}

void *mp_get(Memory_Pool *mp)
{
    if(mp->head == NULL)
        return NULL;

    //store first address
    void *temp = mp->head;

    //link one past it
    mp->head = *mp->head;

    //return the first address
    return temp;
}

void mp_release(Memory_Pool *mp, void *mem)
{
    //store first address
    void *temp = mp->head;

    //link new node
    mp->head = mem;

    //link to the list from new node
    *mp->head = temp;
}

I see a few things that might be changed.

mp_init

First, consider checking the passed *mp variable to see if it's NULL at least within the mp_init call.  Alternatively, you could also allocate that structure within the mp_init routine and return a pointer to it or NULL on error.

The initialization is more complex than it needs to be. Rather than make repeated calls to mp_release and do all of that pointer manipulation, you could use this:

char *ptr;
for (ptr = mp->memory; --slots; ptr+=size)
    *(void **)ptr = ptr+size;
*(void **)ptr = NULL;
mp->head = mp->memory;

mp_release

Within mp_release, there is no error checking.  This might be OK if we're looking for extreme performance, but it might be nice to have at least a debug version that checks that mem actually points to a slot.  For that to work, of course, you'll have to add at least one more variable to the structure to contain the size parameter.

mp_destroy

In mp_destroy it might be prudent to set mp->head = NULL so that any subsequent mp_get attempts will fail.


Random range with no repeat and efficient for small subset Random range with no repeat and efficient for small subset.
But still survives if return most or all.

This comes up poker simulation as you typically only deal 1/3 the deck.

The idea is to use Random and HashSet to determine what has gone out.
When the first or last goes out then trim the range to reduce the number of Random.Next that have already gone out.

Could remove from HashSet that go outside of range but see little or no value in that.  It would reduce size but at a compute cost.  I thought Remove was O(n) but just looked it up and it is O(1) so it might be a good practice.

Ran a time and with 1 million return all (where it is weak) it was 4.7 seconds.  Just return 1/4 of the million then 0.2 seconds. Tested versus a full Fisher Yates shuffle and the break even point is about 1/4.  On the full then shuffle is about 5x faster.

//test
HashSet<int> testRandomSmallSubsetHash = new HashSet<int>();
RandomSmallSubsetHash randomSmallSubsetHash = new RandomSmallSubsetHash();
int randomSmall;
while ((randomSmall = randomSmallSubsetHash.Next()) != -1)
{
    Debug.WriteLine(randomSmall);
    if (!testRandomSmallSubsetHash.Add(randomSmall))
        Debug.WriteLine("ERROR ");
}
Debug.WriteLine("count " + testRandomSmallSubsetHash.Count);
//end test

public class RandomSmallSubsetHash
{
    private Random rand = new Random();
    private int rangeCount;
    private int range;
    private int start;
    private HashSet<int> used = new HashSet<int>();
    public int CountLeft { get { return rangeCount - used.Count; } }
    public bool HasNext { get { return (CountLeft > 0); } }
    public int Next()
    {
        if (!HasNext)
            return -1;

        int next = rand.Next(start, range + start);  //in .NET end is not inclusive
        while (used.Contains(next))
        {
            next = rand.Next(start, range + start);
        }
        //Debug.WriteLine(next);
        if (next == start + range - 1)
        {
            //can trim down the top to eliminte bad guesses
            range--;
            while (used.Contains(start + range))
            {
                range--;
            }
        }
        else if (next == start)
        {
            //can trim down bottom to eliminate bad guesses
            range--;
            start++;
            while (used.Contains(start))
            {
                range--;
                start++;
            }
        }
        used.Add(next);
        return next;
    }
    public void Reset (int RangeCount = 100, int Start = 1)
    {
        if (RangeCount< 1)
            throw new IndexOutOfRangeException();
        rangeCount = RangeCount;
        range = RangeCount;
        start = Start;
        used.Clear();
    }
    public void Reset()
    {
        Reset(rangeCount, start);
    }
    public RandomSmallSubsetHash(int RangeCount = 100, int Start = 1)
    {
        Reset(RangeCount, Start);
    }
}

Before I start, let me say that this code looks pretty good. You seem to have tested your algorithm pretty well and I've never read about shuffling algorithms other than the Fisher-Yates, so I won't comment on it.

First, this code seems a little squished together. I would put newlines between my methods to make it a little clearer where one unit of responsibility starts and the previous one stops.

Second, please use your braces. And if you aren't going to use braces, then consistently don't use them:

if (!HasNext)
    return -1;

while (used.Contains(next))
{
    next = rand.Next(start, range + start);
}

Third, parameters are named with camelCase by convention in C#:

public void Reset (int RangeCount = 100, int Start = 1)

Fourth, please put spaces around your operators:

if (RangeCount< 1)

In the if/else if block in Next(), you do range-- at the top of each block. You should move this to just above the conditional if it is going to happen either way. However, I'm not quite sure if it is supposed to use the -- operator in each given that the comment for the first says you are trimming the top of the range and the comment for the other says you are trimming the bottom.

//in .NET end is not inclusive

This comment is not really needed, unless you are reminding yourself about it. Robert C. Martin states in Clean Code that every time you use a comment to explain your code, what the comment is really saying is that you failed to write your code so it explains what it does itself. After reading and writing many comments myself, I have to agree with him.

C# 6 would allow you to use the => expression for your one-line calculated properties:

public bool HasNext => CountLeft > 0;

Instead of:

public bool HasNext { get { return (CountLeft > 0); } }