Compressing a vector into a specific range I want to solve the following task --- I am given a vector of integers. I want to 'compress' this list, where the elements will be replaced with numbers 0 through n - 1, where n is the number of unique elements in the vector, such that the relative ordering among the elements is preserved, i.e., if previously vector[index1] < vector[index2], then that is still true after the compression. In other words, each element is replaced with its ranking among the source vector.
For example, given the vector {1, 3, 10, 6, 3, 12}, the elements will be replaced with 0 through 4 since there are 5 unique value. To preserve ordering, it will be transformed into {0, 1, 3, 2, 1, 4}.
Right now, to complete this, I use the following algorithm:
#include <iostream>
#include <map>
#include <set>
#include <vector>

using namespace std;

int compress_vector(vector<int>& vec) {
  // function compresses the vector passed in by reference
  // and returns the total number of unique elements
  map<int, int> m;
  set<int>      s;
  for (auto i : vec) s.insert(i);
  int counter = 0;
  for (auto i : s) {
    m[i] = counter;
    counter++;
  }
  for (auto& i : vec) i = m[i];
  return s.size();
}

int main() {
  vector<int> vec = { 1, 3, 10, 6, 3, 12 };  //

  int size = compress_vector(vec);

  // Should output "0 1 3 2 1 4", then a newline, then a "5"
  for (auto i : vec) cout << i << " ";
  cout << endl;
  cout << size;

  return 0;
}

However, I feel this function is quite messy --- it uses maps, sets, and counters. While this is functional, is there a faster or cleaner way to do this?
Thanks

Algorithm improvements
I really don’t understand what this algorithm is supposed to be doing. Your description of it is incomplete and vague (for example, you say vector[index1] < vector[index2]… but what is index1 and index2?), and the single example is not illuminating. It looks like you’re just trying to get the sorted position of each element in the vector. So it’s very possible that there is a much better algorithm that can solve this; there’s no way for me to know when what you’re trying to do makes no sense to me.
However, I can look at what your existing implementation is doing and at least improve on that a bit.
You use a set to get all the unique elements in the vector, and then you use a map to get… I dunno, something something count (the sorted index?). I can’t help you with the second part. But I can help you with the first part, because you don’t need the set. You can use the map to get the unique values, like so:
auto compress_vector(std::vector<int>& vec)
{
    auto m = std::map<int, int>{};

    // You don't need the set. You can use the map's keys to get the unique
    // values.
    for (auto i : vec)
        m[i] = 0;

    // And then you can recover the "set" of unique values by just iterating
    // on the map's keys.
    auto counter = 0;
    for (auto [i, _] : m)
        m[i] = counter++;

    // The above loop may be optimized by avoiding the duplicate lookup:
    //  for (auto& p : m)
    //      p->second = counter++;

    for (auto& i : vec)
        i = m[i];

    return m.size();
}

You could also go the other way, and keep the set but ditch the map:
auto compress_vector(std::vector<int>& vec)
{
    auto s = std::set<int>{};

    for (auto i : vec)
        s.insert(i);

    // The values of the map are just the indices of the set.
    for (auto& i : vec)
        i = std::distance(s.begin(), s.find(i));

    return s.size();
}

Depending on a lot of factors, it might be more efficient to ditch the set and use a sorted vector instead:
auto compress_vector(std::vector<int>& vec) {
    // Take care of the degenerate case of an empty input vector first.
    //
    // Not strictly necessary, but will save a lot of work.
    if (vec.empty())
        return 0;

    // You could also do this:
    //  if (vec.size() == 1)
    //  {
    //      vec[0] = 0;
    //      return 1;
    //  }

    // A second container, either a map, set, or second vector, is probably
    // unavoidable, because we need to keep track of the original values while
    // also changing the vector's contents, in order to know the sorted
    // indices.
    //
    // Note that this is the lazy way of building the sorted vector. If there
    // are a lot of duplicate values, it *might* be faster to:
    //  1)  reserve vec.size()
    //  2)  for each element in vec, do a lower_bound() search, to find if
    //      it's already in sorted, and if not, then you have the insert
    //      position
    auto sorted = vec;
    std::sort(sorted.begin(), sorted.end());
    sorted.erase(std::unique(sorted.begin(), sorted.end()), sorted.end());

    // Transform each element in the input vector into the index of the
    // element in the sorted vector.
    std::transform(vec.begin(), vec.end(), vec.begin(),
        [&sorted](auto i)
        {
            // Instead of std::find(), you could also use a binary search,
            // like std::lower_bound().
            return static_cast<int>(
                std::distance(
                    sorted.begin(),
                    std::find(sorted.begin(), sorted.end(), i)
                )
            );
        }
    );

    return sorted.size();
}

Code review
using namespace std;

This is always a bad idea. You can probably get away with it in simple, toy programs, but you should never do it in real code.
int compress_vector(vector<int>& vec)

“Out” parameters (function parameters taken by non-const reference, and then modified with the “return” value) are generally not a great idea. They usually make functions harder to use, because they put the onus on the user to set up space for the return. What if I want the original vector and the “compressed” vector? Now I have to deal with the pain of setting up the result manually, rather than just writing auto result = compress_vector(input);. And if the vector I want to “compress” is already const (which is often the case), it’s on me to make a copy again.
I know there is an argument that out parameters can be more efficient, but that doesn’t really apply here, because you’re creating whole maps, sets, and/or copies of the vector in the function anyway.
An even better design would be to take an output iterator argument.
for (auto i : vec) s.insert(i);

Don’t do this. Saving a single line in the function just isn’t worth the risk of missing the hard-to-spot loop body and introducing bugs. If your function is so long that you actually need to save a line or two in order to fit it on screen, your function is too long in any case, and should be broken up.
for (auto i : vec) s.insert(i);
int counter = 0;
for (auto i : s) {
  m[i] = counter;
  counter++;
}
for (auto& i : vec) i = m[i];

Space out the code. There are THREE loops here, jammed all together. That is three entirely separate logical sections of the function. Each section should be separated from the others by a blank line, to make it clear where the function’s “paragraphs” are. (And, of course, both the first and last loops should not be single lines.)
You should also consider using algorithms instead of naked loops, for two reasons. First, they make your code clearer: a naked loop could be doing ANYTHING… but an algorithm spells out exactly what is happening. Also, algorithms are much easier to optimize.
So the big glob of code above could be:
std::for_each(vec.begin(), vec.end(), [&s](auto i) { s.insert(i); });

auto counter = 0;
std::for_each(s.begin(), s.end(), [&m, &counter](auto i) { m[i] = counter++; });

std::for_each(vec.begin(), vec.end(), [&m](auto& i) { i = m[i]; });

Of course, all three algorithms are for_each() here, because I don’t really understand what the loops are supposed to be doing. for_each() is what you use when nothing else makes more sense. You’ll note that in the modified algorithm I wrote above, I used more specific algorithms, like transform(), unique(), and find() because I understood what was going on.
return s.size();

You have a bug here. s.size() gives an unsigned type, which is problematic enough, but the real issue is that the type may be (and often is) larger than an int. But you are forcing it to be crammed into an int, which may cause truncation, or other weirdness. If you’re absolutely sure you want compress_vector() to return int, then you should at least assert that the size of the vector is smaller than the maximum value of int. Or, on the other hand, maybe you don’t really want compress_vector() to return int? I don’t understand what you want from this function, so I can’t guess which is the right answer.
In main():
for (auto i : vec) cout << i << " ";

Once again, this should not be on a single line. Also, you probably don’t want a whole string constant for just a space; you probably mean ' ', not " ".
cout << endl;

std::endl really makes no sense here. If you want a newline, use a newline: std::cout << '\n';.
return 0;

You don’t need this in main().
Summary
Because your description of the problem is so vague and incomplete, and your examples of the intended usage are so limited and unrevealing, it’s impossible to give good recommendations. There is just so much about this function that is unexplained, and so many unanswered questions: Does it really need to modify the input vector; could it not simply return a new vector instead? Why does it return the number of unique elements; is that really important information? Why does it return that value as an int, rather than std::size_t or std::vector::size_type? And so on and so forth.
The best I can do is offer base-level suggestions going by the literal operations in the given code; in other words, I can only give suggestions for tuning the existing algorithm… I can’t suggest better algorithms, if there are any.
That said, there are certainly some corners you could cut, and some inefficiencies you could remove. You don’t need a set and a map… that’s just overkill in just about any situation. You might even get away with a sorted vector, which should be way more efficient than either a map or a set… especially if you do a binary search.


Project Euler #7 in Java Project Euler #7:

By listing the first six prime numbers: 2, 3, 5, 7, 11, and 13, we can see that the 6th prime is 13.
What is the 10 001st prime number?

Here is my solution:
public class PrimeFinder {

    public static void main(String[] args) {
        long time = System.nanoTime();
        int result = getNthPrime(10001);
        time = System.nanoTime() - time;
        System.out.println("Result: " + result
                + "\nTime used to calculate in nanoseconds: " + time);
    }

    private static int getNthPrime(int n) {
        int max = (int) (1.4 * n * Math.log(n));
        boolean[] isPrimeArray = new boolean[max + 1];
        for (int i = 2; i <= max; i++) {
            isPrimeArray[i] = true;
        }
        for (int i = 2; i * i <= max; i++) {
            if (isPrimeArray[i]) {
                for (int j = i; i * j <= max; j++) {
                    isPrimeArray[i * j] = false;
                }
            }
        }
        // Find the nth prime
        int nthPrime = 0;
        int index = 0;
        for(boolean isPrime : isPrimeArray) {
            if(isPrime) {
                nthPrime++;
            }
            if(nthPrime == n) {
                return index;
            }
            index++;
        }
        throw new IllegalArgumentException("n is not valid");
    }
}

It simply performs a sieve, and then find the 10001st true.
Output:

 Result: 104743
 Time used to calculate in nanoseconds: 13812289

Questions:

This obviously is not efficient. How can I improve it?
Are there any bad practices?

Instead of looping one more time through your isPrime array, consider expanding the iterations on your first for loop and then exiting from there once you reach the desired result:

    ...
    int counter = 0;
    for (int i = 2; i <= max; i++) {
        if (isPrimeArray[i]) {
            if (++counter == n) {
                return i;
            }
            for (int j = i; i * j <= max; j++) {
                isPrimeArray[i * j] = false;
            }
        }
    }
    throw new IllegalArgumentException("n is not valid");


Multiple database connections class This is my database connection class in order to handle multiple connections with the use of context managers. Brief explanation:

The __init__ method reads database configurations from an INI file in order to call the connection and set two dictionaries, one for connections and one for cursors.
The __enter__ method takes the specified database connection and cursor returning at the end the cursor to execute queries.
The __exit__ method commits the query and close the cursor if no exception has been identified.

I'm using PyMySQL as MySQL module but I think that the code can be generalized for all MySQL modules, the functions are always those.

Any advice is welcome, I'm new with Python!

import os.path
import configparser
import pymysql.cursors

class Database:
    def __init__(self):
        config = configparser.ConfigParser()

        dbConfigFile = os.path.abspath(os.path.join(os.path.dirname(__file__), os.pardir, 'dbconfig.ini'))
        config.read(dbConfigFile)

        # Connections (database:connection method)
        self.connections = {
            'db1': self.connection(dict(config.items('db1info'))),
            'db2': self.connection(dict(config.items('db2info')))
        }

        # Cursors list (database:cursor method)
        self.cursors = {
            'db1': self.cursor(self.connections['db1']),
            'db2': self.cursor(self.connections['db2'])
        }

    def connection(self, config):
        return pymysql.connect(**config)

    def cursor(self, connection):
        return connection.cursor

    def __call__(self, database):
        self.database = database
        return self

    def __enter__(self):
        self.db = self.connections[self.database]
        self.cursor = self.cursors[self.database](pymysql.cursors.SSDictCursor)
        return self.cursor

    def __exit__(self, exc_type, exc_value, tb):
        if not exc_type:
            self.db.commit()
            self.cursor.close()

Usage:

db = Database()
with db('db1') as cursor:
    cursor.execute("SELECT 1 FROM table_name")

Neat idea! Some suggestions:

__exit__ should try to commit or roll back based on whether there is an unhandled exception, should then close the cursor, and should finally unconditionally close the connection. This is one of the main reasons for using a context manager - it is expected to always clean up after itself, leaving the relevant parts of the system in the same state as before using it.
I would reuse the configuration section name as the keys in the dict. That way you don't need to maintain a mapping in your head or the code - what's in the configuration is what you get when you use the context manager.
Rather than opening all connections and then using only one of them, it should open the connection and the cursor with the name passed in. Otherwise you're wasting resources.


Turn-based game setting properties for playcards I am programming with Java (server side) and JavaScript a card game and I do not know how to give properties (on server side) to every card that I create. For a match I need 20 different cards from a complete set of cards (52). Now I want that these 20 cards get a value (Ace, 2, 3, 4, 5, 6, 7, 8, 9, 10, Jack, Queen, King) and a symbol (like Spades, Hearts, Diamonds and Clubs).

How should I approach this? I thought using if clauses like I used on client side would be ok (JavaScript):

p.initialize = function (id) {

this.card= new createjs.Bitmap(images[id]);

// HEARTS
if (id > 0 && id < 14)
{
this.symbol = Card.HEARTS;
}

// DIAMONDS
else if (id > 13 && id < 28)
{
this.symbol = Card.DIAMONDS;
}

// CLUBS
else if(id > 27 && id <42)
{
this.symbol = Card.CLUBS;
}

// SPADES
else if(id > 41 && id <53)
{
this.symbol = Card.SPADES;
}

this.VALUE = id%13;
};

This is the way I did it on client side. Should I do the same with Java or are there better possibilities?

Slight improvements of your current code

Get rid of the comments and read three lines below which symbol you are currently declaring the conditions for. Your code is very self-documenting, no need to add comments.
You should know that there is a more than or equal to operator, which can be used to make your code a lot more readable (here I have also removed the braces since your statement is only one line)

if (id >= 1 && id <= 13) this.symbol = Card.HEARTS;
else if (id >= 14 && id <= 28) this.symbol = Card.KARO;
else if (id >= 28 && id <= 41) this.symbol = Card.CLUBS;
else if (id >= 42 && id <= 52) this.symbol = Card.SPADES;

This can in turn also be simplified to:

    if (id <= 0) { /* mega-super-error-should-probably-not-happen-and-if-it-does-then-it's-no-symbol */ }
    else if (id <= 13) this.symbol = Card.HEARTS;
    else if (id <= 28) this.symbol = Card.KARO;
    else if (id <= 41) this.symbol = Card.CLUBS;
    else if (id <= 52) this.symbol = Card.SPADES;

Weird King

Are you aware that in your current code, you will have the values 1 2 3 4 5 6 7 8 9 10 11 12 0? This feels weird. If Ace is 1 then 0 is king I assume? But using 0 as king does not make sense.

Adding a parameter to the function

Even though @rolfl provides a short way to do this, I just want to object to one thing: The use of the id parameter.

It seems like you are creating a card from an ID. Now what if you some day totally forget about how to calculate these IDs from a specified suite and rank? (or vice versa). I would instead use a function with two parameters: Suite and rank. I'm not sure how you are calling this method today, but to me it feels cleaner to create cards by using a nested for-loop to loop first over suite and then over rank, rather than looping from 1 to 52.

Also, I wouldn't use an uppercase property VALUE when the others, symbol and card is lowercase. It's better to use lowercase value also. Or, you could call it rank instead. And you might want to rename the card property to picture.

As for how to do it in Java, I would say that you should try to make it the same in both Java and JavaScript. Tidy up the JavaScript and then make it the same in Java.


Scala: Josephus problem with Actors Another stab at the Josephus problem, this time using Actors. N people stand in a circle and every stepth person is eliminated until one is remaining.

The way I solve it here is to give each actor a reference to their successor, and the actors send their successor a count of places since the last elimination. If an actor is eliminated, it replies with a reference to its successor so that the previous actor now links the one after.

I've hardly used Actors before. I'd like general comments, but in particular I'm wondering:

is using Actors appropriate for this problem?
are the messages I used sensible?
is the wait for reply necessary?
how would you do it differently?

btw I'm going to try this with Akka as well - if that will make any major difference to the solution that you know of, please mention!

object Josephus_Actors extends App {
  import actors._

  val N = 40
  val step = 3

  case class Person(id: Int) extends Actor { Self =>
    // would like to make `next` private[this], but it's then very tricky to set up
    var next: Person = _
    private[this] var alive = true
    def act {
      while (alive) {
        // Messages consist of (count, sender) where count is distance from last man out
        receive {
          // last man standing
          case (_, Self) => {
            println("Winner: "+id)
            sys.exit(0)
          }
          // kill self
          case (x: Int, _) if x == step => {
            alive = false
            reply {next}
          }
          // kill next
          case (x: Int, _) if x == step - 1 => {
            // wait until we have received our next value
            next !? ((step, Self)) match {
              case a: Person => next = a
              case _ =>
            }
            next ! (1, Self)
          }
          /* pass on message */
          case (x: Int, _) => {
            next ! (x + 1, Self)
          }
          case other => println(id + " unrecognized message: " + other)
        }
      }
    }
  }

  val acts = 1 to N map Person
  acts foreach { p =>
    // add reference to successor
    p.next = p.id match {
      case N => acts(0)
      case x => acts(x)
    }
    p.start()
  }
  acts(0) ! (1, null)
}

would like to make next private[this], but it's then very tricky to set up

To achieve that, you can define the initial next person as a lazy val field, and then define a var locally in the act method:

case class Person(id: Int) extends Actor { Self =>
  private[this] lazy val initialNext: Person = acts((id + 1) % N)
  ...
  def act {
    var next = initialNext

The initialization then simplifies to

val acts = 1 to N map Person
acts foreach { _.start }
acts(0) ! (1, null)


Extract a range of rows from one file into another Wondering if there is anything I can improve upon my code?

This method accepts a filename (usually will be the path to the file in txt format), the starting row count of that file and the total amount of rows of the file. The method is suppose to iterate through the starting row count passed in and then write into a new file with the remaining rows.

public void blah(String fileName, String startRowCount, String totalRowCount) {
        BufferedReader br = null;
        File newFile = null;
        BufferedWriter output = null;
        StringBuilder sb = null;
        int startRowCountInt = Integer.parseInt(startRowCount);
        int totalRowCountInt = Integer.parseInt(totalRowCount);

        try {
            br = new BufferedReader(new FileReader(fileName));
            sb = new StringBuilder();
            newFile = new File("name_Rerun.txt");
            output = new BufferedWriter(new FileWriter(newFile));
            String line = "";
            int counter = 0;

            while ((line = br.readLine()) != null) {
                if (startRowCountInt <= counter && counter <= totalRowCountInt) {
                    output.write(line);
                    output.newLine();
                }
                counter++;
            }
        } catch (IOException e) {
            // TODO Auto-generated catch block
            LOGGER.info("File was not found.");
            e.printStackTrace();
        } finally {
            // Should update to Java 7 in order to use try with resources and then this whole finally block can be removed.
            try {
                if ( br != null ) {
                    br.close();
                }
                if ( output != null ) {
                    output.close();
                }
            } catch (IOException e) {
                // TODO Auto-generated catch block
                LOGGER.info("Couldn't close BufferReader.");
                e.printStackTrace();
            }
        }
    }

public void blah(String fileName, String startRowCount, String totalRowCount)

A general purpose function should not have integer parameters passed as string, it's not its responsibility to parse them. Change them to int and move parsing outside. Side note: from given code it's not clear but parsing may fail if input is user provided (otherwise I see no reason you may have a string). In that case Integer.parseInt() will throw NumberFormatException.

br, sb but also newFile and counter aren't best variable names you can think of. Not to mention that you do not even use sb. Let's say you have these:

reader = new BufferedReader(new FileReader(inputFileName));
writer = new BufferedWriter(new FileWriter(outputFileName));

Note that I also changed parameter name and I introduced a constant for output file name. I'm generally against paths relative to current working directory but this may be a requirement in this case (I don't know.) If you're using Java 8 you also have an handy Files.lines(Paths.get(inputFileName)). You may also consider to use PrintWriter (which has a println() method) instead of BufferedWriter.

Your if condition inside while checks for two different things: skipping lines until first required one is read and ignore lines after last required one is read. If you do not require to consume all input lines then you're wasting time because you will read entire file even when you're done with your task. Few changes about names, you're counting line indices, not count.

String line = "";
int currentLineNumber = 0;

while ((line = br.readLine()) != null) {
    if (counter++ < startRowIndex)
        continue;

    if (counter > lastRowIndex)
        break;

    output.write(line);
    output.newLine();
}

Your pattern to catch IOException is little bit redundant, you do not need to log the inner error for close(), any relevant error might come from a previous call to flush():

try {
    // Do your stuff here...
    writer.flush();
} finally {
    if (writer != null) {
        try {
            writer.close();
        } catch (IOException e) {
            // We may ignore errors for close(), if anything went
            // wrong with our data then we already reported the issue
            // with the call to flush()
        }
    }
}

Note, however, that this pattern is prolix and it may be simplified (as you already marked in a comment) using try-with-resources:

try (BufferedReader reader = new BufferedReader(new FileReader(inputFilePath))) {
    // Do your stuff here...
} catch (IOException e) {
    // Log?
}


Getting a compass point from 140° This JavaScript converts degrees to a compass point, e.g. convert.toCompass(140) gets 140° -> SE.

I can't help but think: is there a more concise way to do this?

var convert = {
    toCompass: function(degrees)
    {
        if(degrees >= 0 && degrees <= 11.25)
        {
            return 'N';
        }

        else if(degrees > 11.25 && degrees <= 33.75)
        {
            return 'NNE';
        }

        else if(degrees > 33.75 && degrees <= 56.25)
        {
            return 'NE';
        }

        else if(degrees > 56.25 && degrees <= 78.75)
        {
            return 'ENE';
        }

        else if(degrees > 78.75 && degrees <= 101.25)
        {
            return 'E';
        }

        else if(degrees > 101.25 && degrees <= 123.75)
        {
            return 'ESE';
        }

        else if(degrees > 123.75 && degrees <= 146.25)
        {
            return 'SE';
        }

        else if(degrees > 146.25 && degrees <= 168.75)
        {
            return 'SSE';
        }

        else if(degrees > 168.75 && degrees <= 191.25)
        {
            return 'S';
        }

        else if(degrees > 191.25 && degrees <= 213.75)
        {
            return 'SSW';
        }

        else if(degrees > 213.75 && degrees <= 236.25)
        {
            return 'SW';
        }

        else if(degrees > 236.25 && degrees <= 258.75)
        {
            return 'WSW';
        }

        else if(degrees > 258.75 && degrees <= 281.25)
        {
            return 'W';
        }

        else if(degrees > 281.25 && degrees <= 303.75)
        {
            return 'WNW';
        }

        else if(degrees > 303.75 && degrees <= 326.25)
        {
            return 'NW';
        }

        else if(degrees > 326.25 && degrees <= 348.75)
        {
            return 'NNW';
        }

        else if(degrees > 348.75 && degrees <= 360)
        {
            return 'N';
        }
    }
}

By looking at values in your series of if's, you can make an array ([]), then calculate which index to pick:

var convert = {
    toCompass: function(degrees)
    {
        return ['N', 'NNE', 'NE', 'ENE', 'E', 'ESE', 'SE', 'SSE', 'S', 'SSW', 'SW', 'WSW', 'W', 'WNW', 'NW', 'NNW', 'N'][Math.round(degrees / 11.25 / 2)];
    }
}

alert(convert.toCompass(140));      // SE

JSFiddle demo


Printing out all the subsets of a set I am concerned about creating too many objects in my code.
Am I using duplicate code? Can I cut down on the number of lines of code?

Note: I only want to use recursion to solve this. Any suggestions on the design of the program and naming convention used here are appreciated.

import java.util.ArrayList;

class Subsets {
    public static void main(String[] args) {
        ArrayList<Integer> superSet = new ArrayList<Integer>();
        superSet.add(1);
        superSet.add(2);
        superSet.add(3);
        superSet.add(4);
        ArrayList<ArrayList<Integer>> lists = Subsets.getSubsets(superSet);
        System.out.println("final set ==> " + lists);
    }

    static ArrayList<ArrayList<Integer>> getSubsets(ArrayList<Integer> SubList) {
        if (SubList.size() > 0) {
            ArrayList<ArrayList<Integer>> list = addToList(SubList.remove(0),
                    SubList);
            return list;
        } else {
            ArrayList<ArrayList<Integer>> list = new ArrayList<ArrayList<Integer>>();
            list.add(SubList);
            return list;
        }
    }

    private static ArrayList<ArrayList<Integer>> addToList(
            Integer firstElement, ArrayList<Integer> SubList) {
        ArrayList<ArrayList<Integer>> listOfLists = getSubsets(SubList);
        ArrayList<ArrayList<Integer>> superList = new ArrayList<ArrayList<Integer>>();
        for (ArrayList<Integer> iList : listOfLists) {
            superList.add(new ArrayList<Integer>(iList));
            iList.add(firstElement);
            superList.add(new ArrayList<Integer>(iList));
        }
        return superList;
    }
}

You can combine your two methods in just one method, making the whole process much easier to understand due to less "jumping around".

static <T> List<List<T>> getSublists(List<T> list) {
    if (list.isEmpty()) {
        // if empty, return just that empty list
        return Collections.singletonList(list);
    } else {
        List<List<T>> sublists = new ArrayList<List<T>>();
        T first = list.get(0);
        // for each sublist starting at second element...
        for (List<T> sublist : getSublists(list.subList(1, list.size()))) {
            //... add that sublist with and without the first element
            // (two lines more, but this preserves the original order)
            List<T> sublistWithFirst= new ArrayList<T>();
            sublistWithFirst.add(first);
            sublistWithFirst.addAll(sublist);
            sublists.add(sublist);
            sublists.add(sublistWithFirst);
        }
        return sublists;
    }
}

Some more points:

I'd recommend using the more generic List whenever possible.
Also, no need to restrict the method to lists of integers.
That code definitely deserves some comments.
You are actually returning sub-lists; name your methods accordingly.
Note that by calling remove on the input list, you are modifying the original list!


Primality Test Refactoring I devised a new primality test isPrimeNumber() below which determines if 1000000007 is prime.
It works fine but the code looks unprofessional in my opinion. Is there any way to refactor it to make it more readable? Maybe inline the recursive gcd() function so everything is self-contained in the isPrimeNumber() function etc.,...? Any tips? Thanks!
// Primality Test
// Every n is prime if all lattice points on x+y=n are visible from the origin.

#include <stdio.h>
#include <stdint.h>
#include <math.h>

#define NOT_PRIME_NUMBER 0
#define PRIME_NUMBER 1

uint64_t gcd(uint64_t a, uint64_t b)
{
    return (b != 0) ? gcd(b, a % b) : a;
}

int isPrimeNumber(uint64_t n)
{
    if (n == 2 || n == 3 || n == 5)
    {
        return PRIME_NUMBER;
    }

    uint64_t m = n % 10;

    if (m == 1 || m == 3 || m == 7 || m == 9)
    {
        uint64_t x, y;
        uint64_t step = 0, maxStep = sqrt(n) / 2;

        // Start near line x=y.
        x = (n / 2) + 2;
        y = n - x;

        do {

            // Check lattice point visibility...
            if (gcd(x, y) != 1)
            {
                return NOT_PRIME_NUMBER;
            }

            x++; y--; step++;

        } while (step < maxStep);
    }
    else
    {
        return NOT_PRIME_NUMBER;
    }

    return PRIME_NUMBER;
}

int main(int argc, char* argv)
{
    uint64_t n = 1000000007;

    if (isPrimeNumber(n) == PRIME_NUMBER)
    {
        printf("%llu prime.", n);
    }
    else
    {
        printf("%llu not prime.", n);
    }

    return 0;
}

Your m digit checking can be simplified and your do while loop can be replaced with the simpler for loop. The constants also seem excessive given that non-zero truthiness is baked into C.
int isPrimeNumber(uint64_t n)
{
    if (n==2 || n==3) return 1;
    if (n%2==0) return 0;

    // Start near line x=y.
    uint64_t x = (n / 2) + 2;
    uint64_t y = n - x;

    uint64_t count = sqrt(n) / 2;
    for (uint64_t i=0;i<count;++i) {
        // Check lattice point visibility...
        if (gcd(x, y) != 1) return 0;

        x++; y--;
    }

    return 1;
}


Pure JS countdown clock not converting millisecond time correctly I have a semisimple pure js countdown clock that I am having trouble converting the millisecond time correctly.

Here is a working Plunker (besides the date counter)

It Should only be 38 days instead of 184.

//// Should Countdown till feb, 4th 2019 /////

let cd = new Countdown({
  cont: document.querySelector(".container"),
  endDate: 1549263600000,
  outputTranslation: {
    year: "Years",
    week: "Weeks",
    day: "Days",
    hour: "Hours",
    minute: "Minutes",
    second: "Seconds"
  },
  endCallback: null,
  outputFormat: "day|hour|minute|second"
});

Thank's I appreciate the help!

The plunker has this code at the top of the CountDown function:

let options = {
    // ...
    },
    lastTick = null,
    intervalsBySize = ["year", "week", "day", "hour", "minute", "second"],
    TIMESTAMP_SECOND = 200,
    TIMESTAMP_MINUTE = 60 * TIMESTAMP_SECOND,
    TIMESTAMP_HOUR = 60 * TIMESTAMP_MINUTE,
    TIMESTAMP_DAY = 24 * TIMESTAMP_HOUR,
    TIMESTAMP_WEEK = 7 * TIMESTAMP_DAY,
    TIMESTAMP_YEAR = 365 * TIMESTAMP_DAY,
    elementClassPrefix = "countDown_",
    interval = null,
    digitConts = {};

... but that states that there are 200 milliseconds in a second. Obviously there are 1000 of them. So the line in question should be:

TIMESTAMP_SECOND = 1000,

This solves the issue. However, your setInterval also uses this variable for the length of the interval, and it might make sense to have more than one tick per second. So for that interval you should probably use a new variable that would indeed have the value 200.

Nice work though on the CSS side!


Scroll to specific sections I have a jQuery code that helps me to scroll to specific sections on mouse scroll. To handle mouse scroll events I am using the jQuery mousewheel plugin.

    function goTo(location) {
        $('html,body').animate({
            scrollTop: $(location).offset().top
        }, 'slow');
         };
    //Mouse Wheel
// My code is repeating consistently below these line
    $('#slides').on('mousewheel', function(event) {
        event.preventDefault();
        if (event.originalEvent.wheelDelta >= 0) {} else {
            //console.log('Scroll down');
            goTo("#rev_slider_2_1");
        }
    });
    $('#rev_slider_2_1').on('mousewheel', function(event) {
        event.preventDefault();
        if (event.originalEvent.wheelDelta >= 0) {
            goTo("#slides");
        } else {
            //console.log('Scroll down');
            goTo("#clients");
        }
    });

    $('#clients').on('mousewheel', function(event) {
        event.preventDefault();
        if (event.originalEvent.wheelDelta >= 0) {
            goTo("#rev_slider_2_1");
        } else {
            //console.log('Scroll down');
            goTo("#slider");
        }
    });

I want to simplify and make this code shorter so that I no longer need to use the same code again and again. I just want to know how I can create a function that helps me to just pass the parameters and then do my required task.

A simple each loop could make your code smaller, something like:

var elems = [
  $('#slides'),
  $('#rev_slider_2_1'),
  $('#clients')
];

$.each(elems, function (index, $elem) {
  var previous = index > 0 : elems[index-1].id : null;
  var next = index < elems.length-1 ? elems[index+1].id : null;
  $elem.on('mousewheel', function(event) {
    event.preventDefault();
    if (event.originalEvent.wheelDelta >= 0 && previous) {
        //console.log('Scroll up');
        goTo(previous);
    } else if (next) {
        //console.log('Scroll down');
        goTo(next);
    }
  });
});


Modeling Cash-Case differences - should WebAPI DTO's be flat? I have an n-layer application with domain models and DTOs models.

My DTOs are going through entire application(every layer) and also I return them from my WebAPI. It is good approach?

Basically, if my DTOs go out app by API they should be flat or not?

Flat:

public class CashCase
{
    public string Id;
    public string Title;
    public DateTime Opened;

    public decimal DifferenceAmount;
    public decimal DifferenceOriginalAmount;
    public decimal DifferenceOriginalFcyAmount;
    public CashCaseDifferenceCurrency DifferenceCurrency;
    public CashCaseDifferenceSource DifferenceSource;
    public CashCaseDifferenceType DifferenceType;
}
public enum CashCaseDifferenceCurrency
{
    PLN
}
public enum CashCaseDifferenceSource
{
    ATM,
    [EnumMember(Value = "ATM Offsite")]
    ATMOffsite,
    Cash
}
public enum CashCaseDifferenceType
{
    Surplus, Deficiency
}

Not flat:

public class CashCase
{
    public string Id;
    public string Title;
    public DateTime Opened;

    public CashCaseDifference Difference;
}
public class CashCaseDifference
{
    public decimal Amount { get; set; }
    public decimal OriginalAmount;
    public decimal OriginalFcyAmount;
    public CashCaseDifferenceCurrency Currency { get; set; }
    public CashCaseDifferenceSource Source { get; set; }
    public CashCaseDifferenceType Type { get; set; }
}
public enum CashCaseDifferenceCurrency
{
    PLN
}
public enum CashCaseDifferenceSource
{
    ATM,
    [EnumMember(Value = "ATM Offsite")]
    ATMOffsite,
    Cash
}
public enum CashCaseDifferenceType
{
    Surplus,
    Deficiency
}

What would be the best style?

Make them flat if they will be exposed outside applications? I think I don't want expose my inner domain structure to world.

My vote is for not-flat, for several reasons:

The names of the members are less redundant, both in code and over the web
Code is more easily testable. If ever there is a method that deals only with the "difference" fields, there's less surface area to deal with than if you had to manipulate an entire CashCase.
Cardinality. If ever there comes a time that you need to remodel the application to support more than one "difference", having a CashCaseDifference class makes that effort trivial.


.ASP controller code I've tried to create application with MVC N-layer pattern. I asked my buddy for a shot review, and he said, "You should separate logic from controller".
But my controller only work as a ViewModel creator. I'm sure I must not create ViewModel from BusinessLayer.  I need to pass data in DTO like I did here. But what about logic that takes data from DTO and puts it in ViewModel with some rules? I completely misunderstand and need assistance.

public class HomeController : Controller
{
    public ActionResult Index()
    {
        var uploadedImages = UploadedImageHandler.GetAllUploadedImages();
        var model = new IndexViewModel(uploadedImages);
        return View(model);
    }

    [HttpPost]
    public void SetDescription(string description, int id)
    {
        UploadedImageHandler.SetDescription(description, id);
    }

    [HttpPost]
    [ValidateAntiForgeryToken]
    public JsonResult UploadImage(HttpPostedFileBase file)
    {
        var result = UploadedImageHandler.UploadFile(file);
        JsonResult json;

        if (result != null)
        {
            var uploadImageViewModel = new UploadedImagesViewModel
            {
                Compression = result.Compression,
                DateTime = result.DateTime.HasValue ? result.DateTime.Value.ToString("f") : null,
                Description = result.Description ?? "",
                ExifVersion = result.ExifVersion,
                ExposureTime = result.ExposureTime,
                GpsLatitude = result.GpsLatitude,
                GpsLongitude = result.GpsLongitude,
                ImagePath = result.ImagePath,
                Manufacturer = result.Manufacturer,
                Model = result.Model,
                UploadedImageId = result.UploadedImageId
            };
            var uploadResult = new UploadResultViewModel
            {
                Preview = new PreviewViewModel {Id = result.UploadedImageId, PreviewPath = result.PreviewPath},
                UploadedImage = uploadImageViewModel
            };
            json = new JsonResult
            {
                Data = uploadResult,
                ContentType = "text/html"
            };
        }
        else
        {
            json = new JsonResult
            {
                ContentType = "text/html"
            };
        }

        return json;
    }
}

"You should separate logic from controller"

Honestly if a colleague said that to me then I would immediately say.  Great thanks.  What do you think I could move out and why.

It's great to offer feedback but generic feedback like that for me was never useful and I personally always prefer specific help to follow.

Looking at that code it doesn't look like you any logic there to play with, so unless he is meaning the Model -> ViewModel mapping then yes, you could move that to a Mapping layer but I don't consider that a necessity.  If you were considering that the code would probably benefit from using a tool such as AutoMapper to achieve this.

Using AutoMapper

The trick here is you define your mapping rules in the AutoMapper configuration so they can be reused whenever you need to map X -> Y.

public JsonResult UploadImage(HttpPostedFileBase file)
{
    var result = UploadedImageHandler.UploadFile(file);

    if(result == null)
    {
        return new JsonResult { ContentType = "text/html" };
    }

        // Assuming ResultModel is the class type for result
    var uploadResult = Mapper.Map<UploadResultViewModel, ResultModel>(result);

    return new JsonResult
    {
        Data = uploadResult,
        ContentType = "text/html"
    };
}

Not using AutoMapper

No mapping framework with If statements inverted to reduce nesting as well as remove need for additional variables.

public JsonResult UploadImage(HttpPostedFileBase file)
{
    var result = UploadedImageHandler.UploadFile(file);

    if(result == null)
    {
        return new JsonResult { ContentType = "text/html" };
    }

    var uploadImageViewModel = new UploadedImagesViewModel
    {
        Compression = result.Compression,
        DateTime = result.DateTime.HasValue ? result.DateTime.Value.ToString("f") : null,
        Description = result.Description ?? "",
        ExifVersion = result.ExifVersion,
        ExposureTime = result.ExposureTime,
        GpsLatitude = result.GpsLatitude,
        GpsLongitude = result.GpsLongitude,
        ImagePath = result.ImagePath,
        Manufacturer = result.Manufacturer,
        Model = result.Model,
        UploadedImageId = result.UploadedImageId
    };

    var uploadResult = new UploadResultViewModel
    {
        Preview = new PreviewViewModel
        {
            Id = result.UploadedImageId,
            PreviewPath = result.PreviewPath
        },
        UploadedImage = uploadImageViewModel
    };

    return new JsonResult
    {
        Data = uploadResult,
        ContentType = "text/html"
    };
}


Counting the number of distinct words in the input Below is a working program I wrote. Please provide comments to help me improve my coding and problem solving skills. I am learning C++ myself by reading Accelerated C++ by Andrew Koenig.

#include <iostream>
#include <vector>

using namespace std;

int main()
{
    vector<string> words;
    cout<<"Please Enter words(Press Ctrl+Z in the end)"<<endl;

    string x; //Word Input
    cin>>x;
    words.push_back(x); //The first word
    int ndw=1; //Number of distinct words

    while(cin>>x) //Input new word
    {
        for(unsigned int counter = 0; counter!=words.size(); ++counter)
        {
            //Check if we already have this word in our list
            if(x!=words[counter])
            {
                if(counter==words.size()-1)//We have reached the end of list
                {
                    words.push_back(x);
                    ndw+=1;
                }
            }
            else
            {
                //If there is a match, leave this word
                break;
            }
        }
    }
    cout<<"number of distinct words are: "<<ndw;
    return 0;
}

Here is a sample output

Another Sample output

Here are some things that may help you improve your program.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Use all the required #include files

The code uses std::string but doesn't include the corresponding file.  The code should have this line added:

#include <string>

Decompose the program into smaller parts

Right now, all of the code is in main which isn't necessarily wrong, but it means that it's not only hard to reuse but also hard to troubleshoot.  Better is to separate the code into small chunks.  It makes it both easier to understand and easier to fix or improve.

Use an appropriate data structure

The current code uses a std::vector to hold the words and linearly searches for each new word.  Far better would be to use a std::unordered_map for this.  Here's a version which not only counts each unique word, but also counts the number of occurrences of each word:

#include <iostream>
#include <string>
#include <unordered_map>

int main() {
    std::unordered_map<std::string, unsigned> dict;
    for (std::string word; std::cin >> word; ) {
        ++dict[word];
    }
    std::cout << "number of distinct words are: " << dict.size() << "\n";
}

There is a significant performance benefit.  I used both the original and this version to count all the words in the Project Gutenberg eBook of Dracula, by Bram Stoker. Both correctly reported 19027 distinct words, but the original took 1.011 s and the version above took 0.057 s (17 times faster).

Don't use std::endl unless you really need to flush the stream

The difference between std::endl and '\n' is that std::endl actually flushes the stream. This can be a costly operation in terms of processing time, so it's best to get in the habit of only using it when flushing the stream is actually required. It's not for this code.

Use a newer book

The book you have was fine in its day, but it is now woefully out of date.  I'd recommend Stroustrup's book "A Tour of C++" instead, since the current edition covers C++11 which is a very much improved and much different language than previous versions of C++.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.


Evaluating an XPath with document.evaluate() to get an array of nodes The Problem Statement:

  Filter all nodes with an existing attribute that starts with a specific string (temp for example purposes). Print an array of node string representations as a result.

The Code:

var result = document.evaluate("//*[@*[starts-with(name(), 'temp')]]", document, null, XPathResult.ANY_TYPE, null);

var node = result.iterateNext();
var nodes = [];
while (node) {
  nodes.push(node);
  node = result.iterateNext();
}
console.log(nodes);

The code is a bit bulky for such a straightforward problem. Is there a simpler and a more concise way to get an array of nodes from a document.evaluate() result?

I don't see a method to directly turn it into an array -- Array.from doesn't work, eg.  The best I could achieve was a little cleanup:

var result = document.evaluate("//*[@*[starts-with(name(), 'temp')]]", document, null, XPathResult.ANY_TYPE, null);

var node, nodes = []
while (node = result.iterateNext())
  nodes.push(node);

console.log(nodes);


Reading and processing a file using Pandas I am trying to read a file using pandas and then process it. For opening the file I use the following function:

import os
import pandas as pd

def read_base_file(data_folder, base_file):
    files = map(lambda x: os.path.join(data_folder, x), os.listdir(data_folder))
    if base_file in files:
        try:
            df = pd.read_csv(base_file, na_values=["", " ", "-"])
        except Exception, e:
            print "Error in reading", base_file
            print e
            df = pd.DataFrame()

    else:
        print "File Not Found."
        df = pd.DataFrame()
    return df

My main concerns are the if statement and what I should return if there is an error.

Generator expression

I advice using a generator expression instead of map:

map(lambda x: os.path.join(data_folder, x), os.listdir(data_folder))

should become:

(os.path.join(data_folder, x) for x  in os.listdir(data_folder))

Also x should be renamed to something more expressive.

Separation of concerns

You both print and return values, if the printing is for debugging purposes, use logger.log

Specific Exception

If you write:

except Exception, e:

any Exception will be caught, I suggest IOException.


Smartly encrypting strings I am trying to encrypt the input string using the following rule:

1st Character of string will be increment by 4 ( a will become e )
2nd Character of string will be increment by 1 ( a will become b )
3rd Character of string will be increment by 7 ( a will become h )
Rest of the characters will be replaced using the above rules.

Examples:

(Before Enc)    (After Enc)
""              ""
"b"             "f"
"x"             "b"
"imagine"       "mnhkjui"
"abcdef"        "ecjhfm"
"xz"            "ba"

I have written the following code to achieve this:

public static string Puzzle(string str)
{
    string result = "";
    var w = s.a;
    string x = "";
    foreach (var item in str)
    {
        if (item + (int)w > 122)
        {
            x += (char)(96 + ((item + (int)w) % 122));
        }
        else
        {
            x += (char)((int)w + item);
        }
        w = Convert(w);
    }
    return x;
}

enum s { a = 4, b = 1, c = 7 };

private static s Convert(s w)
{
    if (w == s.a)
    {
        w = s.b;
    }
    else if (w == s.b)
    {
        w = s.c;
    }
    else
    {
        w = s.a;
    }
    return w;
}

What I want is a better and more efficient solution than the one I have written.

Your Convert function cycles through the values 4, 1, 7. You do this by comparing the given state with each possible state, and then return the next state. This could be made much simpler by using an index rather than values. Then instead of calling Convert, we can increment the index and if necessary wrap the index around by using a modulo operation. When we need the value, we can use the index to look it up in an array that contains these values.

This would look similar to:

int[] states = {4, 1, 7};
int i = 0;

// get the value:
states[i]

// increment the index:
i = (i + 1) % states.Length;

If we really want to go overboard with this, we could define a type that represents these cyclical states:

class Cycle<T>
{
    private readonly T[] values;
    private int i = 0;

    public Cycle(params T[] values)
    {
        this.values = values;
    }

    public T Value
    {
        get
        {
            return values[i];
        }

    }

    public static Cycle<T> operator++ (Cycle<T> cycle)
    {
        cycle.i = (cycle.i + 1) % cycle.values.Length;
        return cycle;
    }
}

Used as:

var states = new Cycle<int>(4, 1, 7);

// get the value:
states.Value

// increment the index:
++states;

While using such a class makes for a very nice interface, it's not exactly the most efficient thing to do. Using an array as shown at the top is probably a good compromise between good code and performance. In particular, both of my solutions allow you to set the values of the cycle at runtime, rather than hardcoding all values in an enum.


Basic shift cipher in Python I am a bit new to Python. This is my first code review, and I am excited to get a critique on how to make this (very simple) shift cipher more Pythonic, cleaner, and more organized.

alphabet = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j','k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']

def shift(offset):

    message = input("Input Message You Would Like Encrypted:\n")
    new_message = ''

    for letter in message:

        letter = letter.lower() #doesn't handle upper-case yet

        if letter.isalpha():
            shift_pos = alphabet.index(letter) + offset
            new_pos = alphabet[shift_pos]
            new_message += new_pos

        #these will not be shifted

        elif ' ' or '/t' or '/n' in letter:
            new_message += letter

        elif letter.isnumeric():
            new_message += letter

        else:
            print("An error took place in recording the message. Check input.\n")

    print(new_message)

shift(-1)

Bug

elif ' ' or '/t' or '/n' in letter:
    new_message += letter

Provided that execution reaches that point (i.e. letter.isalpha() is false), this condition always evaluates to True, because the space character is a non-empty string.  The rest of the expression doesn't matter due to short-circuit evaluation of or.  For the record, the string consisting of the two characters / and t is always True, and the two-character string '/n' can never appear within a one-character string.

Of course, that means that the elif letter.isnumeric() and the else branches are unreachable.

Did you mean to write this instead?

elif letter in ' \t\n':
    new_message += letter

Naming

letter might not be a letter of the alphabet.  A better name would be character, char, or just c.

Design

shift would be a better as a pure function: it should accept the message as one of its parameters (instead of prompting for it) and return the enciphered result (instead of printing it).  Then, you would call

print(shift(input("Input message you would like encrypted:\n"), -1))

Why do you want to disallow certain characters?  If digits and spaces are to be passed through unmodified, why shouldn't the same be true of punctuation?

Efficiency

Strings in Python are immutable.  Every time you call += on a string, it allocates a new string and fills it with the old content and the new ending.  Therefore, += is a relatively expensive operation and should be avoided.

A better alternative would be to build a list (by appending or using a list comprehension).  Then, you construct the result by calling ''.join(…).

Pythonic cleverness

Your alphabet could just be string.ascii_lowercase instead of that list.

The classic way to implement a Caesar Cipher in Python is using str.translate().

from string import ascii_lowercase as ALPHABET

def shift(message, offset):
    trans = str.maketrans(ALPHABET, ALPHABET[offset:] + ALPHABET[:offset])
    return message.lower().translate(trans)

print(shift(input("Input message you would like encrypted:\n"), -1))


BinarySearch Tree implementation & traversals I am practicing various tree algorithms and that require code review for it's efficiency, clarity and also if it can be made better etc.

Is there a better way to call parent.findMin(parent)

TreeNode.java

package binarytree;

public class TreeNode
{
    TreeNode left;
    TreeNode right;
    int data;
    TreeNode parent;
    int size=0;

    TreeNode(int data)
    {
        this.data = data;
        size = 1;
    }

    int size()
    {
        return size;
    }

    void setLeftChild(TreeNode left)
    {
        this.left = left;
        if(left == null)
        {
            left.parent = this;
        }
    }

    void setRightChild(TreeNode right)
    {
        this.right = right;
        if(right == null)
        {
            right.parent =  this;
        }
    }

    void insertInOrder(int d)
    {
        if(d <= data)
        {
            if(left == null)
            {
                setLeftChild(new TreeNode(d));
            }
            else
            {
                left.insertInOrder(d);
            }
        }
        else{
            if(right == null)
            {
                setRightChild(new TreeNode(d));
            }
            else{
                right.insertInOrder(d);
            }
        }
        size++;
    }

    void postOrderTraversal(TreeNode parent)
    {
        if(parent == null) return;

        postOrderTraversal(parent.left);
        postOrderTraversal(parent.right);
        printVal(parent);
    }

    void preOrderTraversal(TreeNode parent)
    {
        if(parent == null) return;

        printVal(parent);
        preOrderTraversal(parent.left);
        preOrderTraversal(parent.right);
    }

    void inOrderTraversal(TreeNode parent)
    {
        if(parent == null) return;

        inOrderTraversal(parent.left);
        printVal(parent);
        inOrderTraversal(parent.right);
    }

    void printVal(TreeNode parent)
    {
        System.out.print(parent.data + "\t");
    }

    /* binary tree find Minimum */
    int findMin(TreeNode parent)
    {
        if(parent == null) return 0;

        int min = parent.data;
        if(parent.left !=null)
        {
            min = Math.min(min, findMin(parent.left));
        }

        if(parent.right != null)
        {
            min = Math.min(min, findMin(parent.right));
        }

        return min;
    }

     /* binary tree find Maximum */
    int findMax(TreeNode parent)
    {
        if(parent == null) return 0;

        int max = parent.data;
        if(parent.left !=null)
        {
            max = Math.max(max, findMax(parent.left));
        }
        if(parent.right != null)
        {
            max = Math.max(max, findMax(parent.right));
        }
        return max;
    }
}

Traversal.java

package binarytree;

import binarytree.TreeNode;
public class Traversals {

    public static void main(String[] args)
    {
        TreeNode parent = new TreeNode(1);
        TreeNode left = new TreeNode(2);
        TreeNode right = new TreeNode(3);
        TreeNode next = new TreeNode(5);

        parent.insertInOrder(left.data);
        parent.insertInOrder(right.data);
        parent.insertInOrder(next.data);

        System.out.println("Pre-order traversal");
        parent.preOrderTraversal(parent);
        System.out.println();

        System.out.println("Post-order traversal");
        parent.postOrderTraversal(parent);
        System.out.println();

        System.out.println("In-order traversal");
        parent.inOrderTraversal(parent);
        System.out.println();

        System.out.println("min element:"+parent.findMin(parent));
        System.out.println("max element:"+parent.findMax(parent));
    }

}

Here are a few things you can do to improve this:

You don't specify visibility of your methods or variables.  Presumably you want your variables to be private, but I'm not sure which of your methods you want public or not. I imagine you don't want setLeftChild or setRightChild to be public since you can't enforce your ordering constraints if anyone can just change children when they feel like it.
Both setLeftChild and setRightChild will throw NullPointerExceptions when passed a null value. You want != instead of == in your check.
Do your nodes need references to their parents?
In your Traversals class you have nodes named parent, but you are actually referring to the root node of your tree. I would rename that.
You don't need to create the other three TreeNode classes since you only use them to get the value that you passed into them when you call insertInOrder.  Just call insertInOrder with the values you want.
You shouldn't pass in the parent parameter to your traversal methods. These are instance methods and can just refer to the member variables directly.
Initialize size once.  Currently you set it to 0 and then 1.
I'm assuming you have plans to use size later on. Otherwise you can just remove it entirely.


"Angry Professor" Python implementation Problem Statement

  The professor is conducting a course on Discrete Mathematics to a class of N students. He is angry at the lack of their discipline, and he decides to cancel the class if there are fewer than K students present after the class starts.

  Given the arrival time of each student, your task is to find out if the class gets cancelled or not.

  Input Format

  The first line of the input contains T, the number of test cases. Each test case contains two lines.
  The first line of each test case contains two space-separated integers, N and K.
  The next line contains N space-separated integers, \$a_1,a_2,…,a_N\$, representing the arrival time of each student.

  If the arrival time of a given student is a non-positive integer (\$a_i≤0\$), then the student enters before the class starts. If the arrival time of a given student is a positive integer (\$a_i>0\$), the student enters after the class has started.

  Output Format

  For each testcase, print "YES" (without quotes) if the class gets cancelled and "NO" (without quotes) otherwise.

  Constraints

  $$\begin{align}
1&≤T≤10,\\
1&≤N≤1000,\\
1&≤K≤N,\\
−100&≤a_i≤100, \text{where}\ i∈[1,N]
\end{align}$$

  Note

  If a student enters the class exactly when it starts (\$a_i=0\$), the student is considered to have entered before the class has started.

  Sample Input

2
4 3
-1 -3 4 2
4 2
0 -1 2 1

  Sample Output

YES
NO

Solution

def is_on_time(a):
    return a <= 0

def class_cancelled(expected, arrival_times):
    actual = map(lambda x: 1 if is_on_time(x) else 0, arrival_times).count(1)
    return actual >= expected

def read_int():
    return int(raw_input())

def read_ints():
    return map(int, raw_input().strip().split())

if __name__ == '__main__':
    T = read_int()
    for _ in xrange(T):
        N, K = read_ints()
        A = read_ints()
        print "NO" if class_cancelled(K, A) else "YES"

def class_cancelled(expected, arrival_times):
    ....
    return actual >= expected

print "NO" if class_cancelled(K, A) else "YES"

class_cancelled is a misnomer. It in fact computes a class_not_cancelled condition: class should be cancelled when actual < expected. Also the problem asks to print "Yes" when the class gets cancelled. Funny how two wrongs make it right.


Find the number of times that the difference of array values are equal to the number I wrote the following code to answer this, but I am wondering if there is any better approach.

For example

If the following array is given:

{10,21,34,45,56}

we should find the differentiate of each number with other values of the array and in case it is equals to 11 should increment the counter.

10-21=-11
10-34=-24
10-45=-35
10-56=-46
21-10=11     <
21-34=-13
21-45=-24
21-56=-35
34-10=24
34-21=13
34-45=-11
34-56=-22
45-10=35
45-21=24
45-34=11   <<
45-56=-11
56-10=46
56-21=35
56-34=22
56-45=11   <<<

Code

List<Integer> a = new ArrayList<Integer>();
       a.add(10);
       a.add(21);
       a.add(34);
       a.add(45);
       a.add(56);
      int number = 11;
      int counter = 0;
       for(int i=0;i<a.size();i++){
           for(int j=0;j<a.size();j++){
               if(j!=i){
                   int t = a.get(i) - a.get(j);
                   if(t == number){
                       System.err.println(a.get(i) +"-"+ a.get(j) + "=" + t);
                       counter++;
                   }
               }
           }
       }
       System.err.println(counter);
    }

Output

The application is correctly showing the results but I am wondering if there is any other approach to the above solution.

21-10=11
45-34=11
56-45=11
3

I think you should look at the problem in a different way and come up with a simpler solution.

Try thinking at having a set of elements. What you want to know is if there is any pair of elements such that a - b = x.

You're given x, so for a given value of a in your set you should check whether it also contains x + a.

Your implementation should first move the content of the list to a Set. Then you have to iterate through the set and for each element a check if the set contains also x + a. If it is true then you should add (a,b) to your solution.

In your example, x = 11. If you consider a = 10 you'll check if it contains 11 + 10 = 21. It does so you can the pair (21,10) is a valid solution. Conversely, when you consider a = 21, you obtain b = 32, which is not part of the set and therefore you have to discard that pair.

int checkDifferences(Set<Integer> numbers, int difference) {
    int occurrences = 0;
    for (Integer number : numbers) {
        if (numbers.contains(number + difference)) {
            occurrences++;
        }
    }
    return occurrences;
}


Little log engine in C I programmed a little log engine in C I plan to use in my project and maybe some others in future. I am very novice C programmer and would like to have feedback of some experienced ones on this. It's spawned over few files but I've joined them so it's easier to compile. Thank you.

#include <stdarg.h>
#include <syslog.h>
#include <assert.h>
#include <time.h>
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <errno.h>

#include "config.h"

/* this is log.h */
#define TPL_IDENT PACKAGE_NAME

enum tp_log_level {
    TPL_DEBUG,
    TPL_INFO,
    TPL_ERR,
    TPL_EMERG,
};

enum tp_log_mode {
    TPLM_SYSLOG,
    TPLM_FILE,
};

void tp_log_init(int mode, int level, int fd);
void tp_log_write(int level, const char *fmt, ...);
void tp_log_close(void);
/* end of log.h */

/* function from pio.c I use */
ssize_t tp_write(int fd, const void *buf, size_t len)
{
    ssize_t ret, wlen = 0;
    const char *ptr;

    ptr = buf;
    while (wlen != len) {
        ret = write(fd, ptr, len-wlen);

        if (ret == -1)
            if (errno == EINTR)
                continue;
            else
                return -1;

        wlen += ret;
        ptr += ret;

    }
    return wlen;
}

/* start of log.c */

static int log_fd = -1;
static int log_level;
static int log_mode;

static const char *level_txt[] = {
    "DEBUG", "INFO", "ERROR", "EMERG"
};

static const int level_syslog[] = {
    LOG_DEBUG, LOG_INFO, LOG_ERR, LOG_EMERG
};

void tp_log_init(int mode, int level, int fd)
{
    assert(mode == TPLM_SYSLOG || mode == TPLM_FILE);
    assert(mode == TPLM_FILE ? fd >= 0 : 1);
    assert(level >= TPL_DEBUG && level <= TPL_EMERG);

    switch (mode) {
        case TPLM_FILE:
            log_fd = fd;
            break;
        case TPLM_SYSLOG:
            openlog(TPL_IDENT, LOG_PID|LOG_CONS, LOG_DAEMON);
            break;
    }
    log_level = level;
    log_mode = mode;
}

#define MAX_TIME_LEN 512
#define MAX_PREFIX_LEN 10
#define MAX_MSG_LEN 1024
#define MAX_POSTFIX_LEN 2

static void tp_vlog_write(int level, const char *fmt, va_list alist)
{
    int ret, len;
    time_t t;
    struct tm *tm;
    char msg[MAX_TIME_LEN+MAX_PREFIX_LEN+MAX_MSG_LEN+MAX_POSTFIX_LEN];

    assert(log_fd);
    assert(fmt);
    assert(level >= TPL_DEBUG && level <= TPL_EMERG);

    if (level < log_level)
        return;

    switch (log_mode) {
        case TPLM_FILE:

            t = time(NULL);
            tm = localtime(&t);
            if (tm == NULL)
                abort();

            len = strftime(msg, MAX_TIME_LEN, "%a, %d %b %Y %T %z", tm);
            len += snprintf(msg+len, MAX_PREFIX_LEN, " [%s] ",
                    level_txt[level]);

            ret = vsnprintf(msg+len, MAX_MSG_LEN, fmt, alist);
            if (ret >= MAX_MSG_LEN)
                len += MAX_MSG_LEN-1;
            else
                len += ret;

            snprintf(msg+len, MAX_POSTFIX_LEN, "\n");
            tp_write(log_fd, msg, len+1);
            break;

        case TPLM_SYSLOG:
#ifdef HAVE_VSYSLOG
        vsyslog(level_syslog[level], fmt, alist);
#else
        vsnprintf(msg, MAX_MSG_LEN, fmt, vl);
        syslog(level_syslog[level], "%s", msg);
#endif
            break;
    }
}

void tp_log_write(int level, const char *fmt, ...)
{
   va_list vl;

   va_start(vl, fmt);
   tp_vlog_write(level, fmt, vl);
   va_end(vl);
}

void tp_log_close(void)
{
    assert(log_fd);

    switch (log_mode) {
        case TPLM_SYSLOG:
            closelog();
            break;
        case TPLM_FILE:
            close(log_fd);
            break;
    }
}

As promised, here are some comments on the actual code you posted.

General:

don't define multiple variables on the same line.
is there a good reason to redefine log levels in tp_log_level rather than
use the levels Syslog defines?
you have two arrays, level_txt and level_syslog that are dependent upon
the values of tp_log_level but nothing ties these together.  In such a
small program, this is unlikely to be a problem, but this sort of loose
dependency tends to break with time - someone appends (or even inserts!) a
value to one part without adjusting the other.  Do you really need to log
the level text, "DEBUG" etc?  Syslog doesn't.  Why complicate the job?  If
you just use syslog constants, both these arrays could be discarded.  If you
really must use them, make a structure holding a level and a text and make
an array of these; then at least you only have one dangling dependency...
I personally don't have any dogmatic objection to multiple returns.  Some
coding standards object to them on the grounds that they make functions
unclear.  I would argue the contrary: they can, if used well, make functions
clearer.

In tp_write

why using raw write (as opposed to stdio)?  The fact that you test for
interrupted writes makes me think that you intend to log to a socket or a
serial port etc...  Is that so?
you should get compiler warnings about comparisons and conversions between
size_t and ssize_t.  Since the function only has two return values, -1 and
len, and the caller knows the value of len anyway, it seems unnecessary
to return anything but an int: 0 for success, and -1 for failure.  That way
you can avoid using a ssize_t and the warnings it generates.
you need braces in the if/else statements: the compiler has to assume
that the else belongs to the inner if.  Using braces even when not
strictly necessary is generally considered good practice.  But they can be
ugly...
might be better coded as:

ssize_t tp_write(int fd, const void *buf, size_t len)
{
    const char *p = buf;
    const char *end = p + len;

    while (p < end) {
        ssize_t ret = write(fd, p, (size_t) (end - p));
        if (ret > 0) {
            p += ret;
        }
        else if (errno != EINTR) {
            return -1;
        }
    }
    return 0;
}

In tp_log_init

mode would be more logically of type tp_log_mode
I don't use asserts much.  You are relying on the asserts to find problems
that the subsequent code does not handle (eg. the switch has no
default).
But another approach is to allow the code to fail if the parameters are
wrong by falling back on syslog.  This works even when NDEBUG is defined
(asserts disabled):

void tp_log_init(enum tp_log_mode mode, enum tp_log_level level, int fd)
{
    log_level = level;
    if (mode == TPLM_FILE) {
        if (fd >= 0) {
            log_fd = fd;
            log_mode = TPLM_FILE;
            return;
        }
        /* print an error or abort() if you like */
    }
    openlog(TPL_IDENT, LOG_PID|LOG_CONS, LOG_DAEMON);
    log_mode = TPLM_SYSLOG;
}

 In tp_vlog_write

MAX_TIME_LEN and MAX_MSG_LEN seem generous for a log message.  Remember that
msg is on the stack and if you are in a restricted environment, 1500 bytes
might be too much.  ctime uses 26 bytes for the time/date, so I doubt your time
format needs 512.
level seems like it should be a enum tp_log_level
the switch could be done with if/else
I would extract the creation of a time string into a separate function.
Also, why abort if localtime fails?  This makes your code more fragile
than it needs to be - just don't call strftime() if tm == NULl.
your assert(log_fd) fails if log_fd == 0.  This should apply to the
TPLM_FILE mode only.
as I said before, printing the level text seems unnecessary.  If you need
it, use strcpy instead of snprintf.
printing "\n" could be done with strcpy(msg+len, "\n")
Personally, I have no objection to the use of varargs in this code.  @Lundin
is right that it is not type-safe, but sometimes it has its uses.  This
seems a legitimate use to me.

 In tp_log_close

your assert fails if log_fd == 0.
if/else would be clearer.


Reversing words in a string I have to reverse the string "He is the one" to "one the is He".  I have written some programs in Java but am looking for other best solutions.  Suggest any possible ways to minimize the current program.

First approach:

class StringRev{
    public static void main(String args[]){
    String str = "He is the one";
    String temp = "";
    String finalString = "";
        for(int i =str.length()-1;i>=0;i--){
            temp +=i!=0?str.charAt(i):str.charAt(i)+" ";
            if(str.charAt(i) == ' '||i==0){
                for(int j=temp.length()-1;j>=0;j--){
                    finalString += temp.charAt(j);
                }
                temp = "";
            }
        }
            System.out.println(finalString);
    }
}

Second approach:

class StringRev2{
    public static void main(String args[]){
    String str[] = "He is the one".split(" ");
    String finalStr="";
        for(int i = str.length-1; i>= 0 ;i--){
            finalStr += str[i]+" ";
        }
        System.out.println(finalStr);
    }
}

"Minimize" is a really bad word to use when trying to consider the quality of your code.  It's vague, and it can lead to undesirable outcomes, such as broken or unusable code.

For example, is your goal to minimize the number of bytes used storing the source code on a hard drive? That might have been important when paper tape was a popular storage medium, but it's irrelevant today.  It's so irrelevant that the difference between these two files is less than the size of a sector, so both actually occupy the same amount of storage space.

Is your goal to reduce the number of CPU cycles spent?  Given that cell phones have 100MHz processors these days, you need to do more work to figure out if the effort you expend thinking about the problem will ever be made up for in efficiency experienced by your users.  If this is run once or twice, efficiency simply is not important. On the other hand, if it could ever become part of a networking protocol, efficiency is extremely important.  Quite honestly, English word reversal seems to be too specialized to fit the test of practicality for efficiency.

In general, what most people should be striving for in their code is "correctness" and "clarity".  You want to know that it works correctly in all situations.  The answer to that is to write unit tests.  For clarity, you want the code to be readable, understandable, and usable.  Make sure you have chosen good names.  Modularize the functions.  For example, you should consider extracting the dependency on System.out.println, as outputting the string has nothing to do with reversing the string.


ADT stack with a dynamic array I'm starting to learn about data structures and coding in C. The stack is the simplest of all data structures so I'm trying to implement an ADT stack with dynamically allocated array in C. This implementation does have one obvious problem i.e. it hoards memory even after popping the data. How do I solve this problem?

stacklib.h

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>

#ifndef STACKLIB_H
#define STACKLIB_H

/* generic type pushed into the stack */
/* in this case it's just an int */
typedef int item_t;

/* our stack data type */
typedef struct {
    int size;
    int capacity;
    int *elements;
} stack;

/* interface to our new stack data type */
stack* create_stack(int capacity);
int push_stack(stack *mystack, item_t data);
void* pop_stack(stack *mystack);
void delete_stack(stack *mystack);

#endif

stacklib.c

#include "stacklib.h"

/* a helper function to handle exceptions */
void handle_error(const char* message)
{
    if(errno) {
        perror(message);
    } else {
        printf("Error: %s\n", message);
    }

    exit(1);
}

stack* create_stack(int capacity)
{
    stack *stack_ptr = (stack*) malloc(sizeof(stack));

    if(stack_ptr == NULL)
        handle_error("Error while allocating memory.");

    int *elements_ptr = (item_t *) malloc(capacity*sizeof(item_t));

    if(elements_ptr == NULL)
        handle_error("Error while allocating memory.");

    stack_ptr->size = -1;
    stack_ptr->capacity = capacity;
    stack_ptr->elements = elements_ptr;

    return stack_ptr;
}

void delete_stack(stack *mystack)
{
    free(mystack->elements);
    free(mystack);
    mystack = NULL;
}

/* Returns 0 on success, 1 on failure */
int push_stack(stack *mystack, item_t data)
{
    int res = 0;
    /* double the array size when stack is full */
    if(mystack->size == (mystack->capacity - 1)) {
        item_t *tmp = (item_t*) realloc(mystack->elements, sizeof(item_t) * mystack->capacity * 2);
        if(tmp == NULL) {
            res = 1;
        } else {
            mystack->elements = tmp;
            mystack->capacity = mystack->capacity * 2;
        }
    }
    mystack->size++;
    mystack->elements[mystack->size] = data;

    return res;
}

/* Returns a pointer to the popped data or NULL if the stack is empty */
void* pop_stack(stack *mystack)
{
    int *res;
    if(mystack->size == -1) {
        res = NULL;
    } else {
        res = &mystack->elements[mystack->size];
        mystack->size--;
    }
    return res;
}

main.c

#include "stacklib.h"

int main(void){
    stack *my_stack = create_stack(5);
    item_t i;
    for(i = 0; i < 50; i++)
        push_stack(my_stack, i);

    item_t res, *item ;
    while((item = (item_t*)pop_stack(my_stack)) != NULL) {
        res = *item;
        printf("data: %d\n", res);
    }
    delete_stack(my_stack);
    printf("last item popped was  %d\n", res);
    return 0;
}

Wrong type

You defined a type called item_t but you didn't use it everywhere:

/* our stack data type */
typedef struct {
    int size;
    int capacity;
    int *elements;
} stack;

Here, elements should be of type item_t *.  Also, your pop function should return a item_t * instead of a void *.

You could go a different direction and use void * as your generic type instead of item_t.  That way, you could store anything in your stack, but you'd have to allocate each item.

Size of -1?

I agree with janos that size should start at 0 instead of -1.  That way, size will represent the number of items on the stack.

Error handling in push

You correctly check the return value of realloc() in your push function.  However, if it returns NULL, your code currently keeps going and causes a buffer overflow.  You should return 1 immediately and not fall through to the rest of the code.

Useless line

In delete_stack(), the line:

mystack = NULL;

doesn't do anything because it won't affect the caller's pointer.

Code simplification

There are a few things that you could do to simplify your code.  One is to eliminate your res variable if you can just return a value immediately.  Another is to merge the postdecrement/postincrement operation into the array access.  For example, your pop function:

void* pop_stack(stack *mystack)
{
    int *res;
    if(mystack->size == -1) {
        res = NULL;
    } else {
        res = &mystack->elements[mystack->size];
        mystack->size--;
    }
    return res;
}

could be simplified to:

item_t *pop_stack(stack *mystack)
{
    if (mystack->size == -1)
        return NULL;
    return &mystack->elements[mystack->size--];
}

Reducing memory

In regards to your question about reducing capacity when popping, you could shrink your array when you pop but you need to be careful:

You should probably remember the initial capacity and not shrink the array below that level.
You don't want to do something where you resize too often.  For example, if you did the natural thing of shrinking when you pop below half capacity: if (size == capacity/2) { resize smaller }, then what could happen is that you could bounce between sizes if the user pushes and pops right at the threshold.  In other words, if the last push caused an expansion, the next pop would cause a contraction, and that would be bad because they could keep pushing and popping at that level and cause a resize every time.  Therefore, you need to either use a smaller ratio than 1/2 for contraction or use a different criterion.


App for painting images I am a newcomer to jQuery and am working on an app that allows the user to paint images. To do that, I've created a color palette, and if a user clicks on a color, the paint brush successfully changes to that color. However, my code looks really ugly. I really want to clean it up.

$(document).ready(function() {
        $('#color-one').click(function() {
          brush.setColor('#990000');
        });
        $('#color-two').click(function() {
        brush.setColor('#CC0000');
    });
        $('#color-three').click(function() {
        brush.setColor('#CC3300');
    });
        $('#color-four').click(function() {
        brush.setColor('#FFCC00');
    });
        $('#color-five').click(function() {
        brush.setColor('#FFFF00');
    });
        $('#color-six').click(function() {
        brush.setColor('#CCFF00');
    });
        $('#color-seven').click(function() {
          brush.setColor('#66FF00');
        });
        $('#color-eight').click(function() {
        brush.setColor('#003300');
    });
        $('#color-nine').click(function() {
        brush.setColor('#33FFCC');
    });
        $('#color-ten').click(function() {
        brush.setColor('#3300CC');
    });
        $('#color-eleven').click(function() {
        brush.setColor('#660033');
    });
        $('#color-twelve').click(function() {
        brush.setColor('#660066');
    });
        $('#color-thirteen').click(function() {
        brush.setColor('#000000');
    });
        $('#color-fourteen').click(function() {
        brush.setColor('#ffffff');
    });
    });
</script>

HTML

I was thinking that the refactored version might look something along the lines of this:

$('#color-one, #color-two, ... color-fourteen).click(function() {
  brush.setColor(this.val);
});

However, I would like to see what this community has to think.

You are right.  You have way too much duplication there.  And you are on the right track with your refactored version but I don't think it goes far enough.

Right now if you want to add a new color you have to do the following.

Add a new div to the color buttons with a unique id.
Add style rules (background) to that div with the color.
Add a button click rule that selects the button by id.
Change the brush color making sure it matches the background done in #2.

With your refactoring you would change this to:

Add a new div to the color buttons with a unique id.
Add style rules (background) to that div with the color.
Add a value to the div button.
Add the id to the button click function.

So you might have made the code shorter but didn't really make it much easier to maintain.

So, how can we get this shorter.  Well, we can select elements by class name.  So instead of using the id we just use class names.

    ...

And your script becomes:

$('#colors .brush-selector').click(function() {
    brush.setColor(this.data('val'));
});

Now to add a new color you have the following steps:

Add a new div to the color buttons with a unique id.
Add style rules (background) to that div with the color.
Add a value to the div button that matches the color.

Better, but we are still duplicating info.  The background color and data-val fields are the same.  And if we accidentally type one wrong then we have incorrect behavior.  So why don't we just use the background-color of the div.

    ...

$('#colors .brush-selector').click(function() {
    brush.setColor(this.css('background-color'));
});

That takes our steps down to:

Add a new div to the color buttons.
Add style rules (background) to that div with the color.

Which is pretty good.  Basically one step to adjust colors.  Just adjust the divs.

There are further steps you could take that I won't go into detail but leave to you.

If you knew the only divs in the #colors elements were brush selectors you could remove the class altogether and just select the divs within #colors $(#colors div).

You could also create a javascript array of allowed colors.  Then create the buttons using the array.  Then the only step you would need would be to adjust the colors in that array.  This would also allow you to select colors dynamically since that array could come from anywhere.


Accessing a certain I/O controller using memory-mapped I/O I have a program that accesses a certain I/O controller using memory-mapped I/O. I currently store the base address of this controller in a const variable.

This leads to a code structure something like this:

gpio.h:

int gpioFunc (unsigned int pin, unsigned int func);

int setGpio (unsigned int pin, unsigned int val);

gpio.c:

#include "gpio.h"
unsigned volatile int* const gpioAddr = (unsigned int*) 0x20200000;

int gpioFunc (unsigned int pin, unsigned int func)
{
    //code that uses the variable
}

int setGpio (unsigned int pin, unsigned int val)
{
    //code that uses the variable
}

main.c:

#include "gpio.h"
void main ()
{
    gpioFunc (16,1);
    setGpio(16,0);
    hlt();
}

I have always been uncertain of the best practices in C as to where to put constants.  Does this way make sense, or would it be better practice to put it somewhere else?  If so, where?

You should make a small driver layer, so that the main doesn't know about the low level details of what is going on - and here in that .c file that you have implemented the driver in, the constants goes as well.

Then you call your driver and all is good :)


Shift one value of array values to the beginning Given an integer $value in the range of 1, 2, 3, I want to get an array that starts with $value and proceeds with the remaining two integers from above in arbitrary order. The best I could think of was doing something like this:

switch ($value) {
      case '1':
        $array = [1,2,3];
        break;

      case '2':
        $array = [2,1,3];
        break;

      case '3':
        $array = [3,1,2];
        break;
    }

Is there a shorter and more beautiful way of doing that? Something like that:

$array = push_to_top_of_array($value,[1,2,3]);

Is there a shorter and more beautiful way of doing that?

shorter: yes
more beautiful: well, that sounds subjective... you can be the judge of the approaches below.

One approach would be to take off the value using $index with array_splice() and then put it (the first element from that spliced array) at the beginning using array_unshift():

$array = [1, 2, 3];
array_unshift($array, array_splice($array, $value - 1, 1)[0]);

See it demonstrated in this playground example.

Another approach would be to merge the spliced array and the original array using array_merge():

$array = [1, 2, 3];
$array = array_merge(array_splice($array, $value - 1, 1), $array);

See it demonstrated in this playground example.


Sort group item by attribute I have a repository method which retrieve a list of items, for example:

StaffID: 250026965
Name: Abbasov Islam
FreeTextName: NULL

StaffID: NULL
Name: NULL
FreeTextName: Antonio Puccio

StaffID: 358
Name: Prosinecki Robert
FreeTextName: NULL

I have to sort this list in this manner:

First the items with StaffID > 0 by Name, then the items with StaffID == NULL by FreeTextName.

In the example case:

StaffID: 250026965
Name: Abbasov Islam
FreeTextName: NULL

StaffID: 358
Name: Prosinecki Robert
FreeTextName: NULL

StaffID: NULL
Name: NULL
FreeTextName: Antonio Puccio

At the moment I am doing:

var resList = conn.Query<TeamSheetStaffPoco>("[dbo].[TList]", p, commandType: CommandType.StoredProcedure).AsList();
var result = resList.Where(x => x.StaffID> 0).OrderBy(y => y.Name).ToList();
result.AddRange(resList.Where(x => x.StaffID== null).OrderBy(y => y.FreeTextName).ToList());

It works, but I am not sure if exist a more concise and elegant mode to to do the same thing.

Use a custom IComparer<T> to sort the the items.

class TeamSheetStaffPocoComparer : IComparer<TeamSheetStaffPoco> {

    public int Compare(TeamSheetStaffPoco a, TeamSheetStaffPoco b) {

        if (a.StaffID == null && b.StaffId == null)
        {
            return a.FreeTextName.CompareTo(b.FreeTextName);
        }

        if (a.StaffID > 0 && b.StaffID > 0)
        {
            return a.Name.CompareTo(b.Name);
        }

        return a.StaffID > 0 ? 1 : -1;
    }

 }

Then you can simplify the last two lines to

var result = resList.OrderBy(x => x, new TeamSheetStaffPocoComparer()).ToList();

Also, you can now reuse the comparison class elsewhere too if you wish.


TicTacToe classic in C I'm learning myself C and to make sure that I learn to code C in a proper way I created tic tac toe and want to hear your opinion on what I should do and what I shouldn't do.

One thing is sure is to divide the code up in several files maybe but outside of that.

The code:

#include <stdio.h>
#include <stdlib.h>

#define ROWS 3
#define BOARD_SIZE ROWS * ROWS

typedef enum { false, true } bool;

bool enter_entry(char *board, char character, int x, int y);
void print_board(const char *board);
void *init_board();
bool is_solved(char *board, char item);
bool is_draw(char *board);
bool is_equal(char *board, int indexA, int indexB, int indexC, char item);
void player_selection(char *board, char item);
void game_loop(char *board);

int main()
{
    char *board = init_board();
    game_loop(board);
    return 0;
}

void *init_board()
{
    char *board = malloc(sizeof(char) * BOARD_SIZE);
    for(int i = 0; i < BOARD_SIZE; i++)
    {
        board[i] = ' ';
    }
    return board;
}

void print_board(const char *board)
{
    for(int i = 1; i < BOARD_SIZE+1; i++)
    {
        printf("| %c ", board[i-1]);
        if(i % 3 == 0)
        {
            printf("|\n");
        }
    }
    printf("\n");
}

bool is_solved(char *board, char item)
{
    if(is_equal(board, 0, 1, 2, item)) { return true; }
    if(is_equal(board, 3, 4, 5, item)) { return true; }
    if(is_equal(board, 6, 7, 8, item)) { return true; }

    if(is_equal(board, 0, 3, 6, item)) { return true; }
    if(is_equal(board, 1, 4, 7, item)) { return true; }
    if(is_equal(board, 2, 5, 8, item)) { return true; }

    if(is_equal(board, 0, 4, 8, item)) { return true; }
    if(is_equal(board, 2, 4, 6, item)) { return true; }
    return false;
}

bool is_draw(char *board)
{
    for(int i = 0; i < BOARD_SIZE; i++)
    {
        if(board[i] == ' ')
        {
            return false;
        }
    }
    return true;
}

bool is_equal(char *board, int indexA, int indexB, int indexC, char item)
{
    if(board[indexA] == item && board[indexB] == item && board[indexC] == item)
    {
        return true;
    }
    return false;
}

bool enter_entry(char *board, char character, int x, int y)
{
    int index = x + ROWS * y;

    if(board[index] != ' ')
    {
        return false;
    }

    board[index] = character;
    return true;
}

void player_selection(char *board, char item)
{
    int x, y;
    printf("enter coords (x, y): \n");

    while(1)
    {
        scanf(" %d %d", &x, &y);
        bool succes = enter_entry(board, item, x-1, y-1);
        if(succes)
        {
            break;
        }
        printf("This coord is already used or not valid enter new ones:\n");
    }
    print_board(board);
}

void game_loop(char *board)
{
    char playerOneChar = 'o';
    char playerTwoChar = 'x';

    printf("Welcome to tic tac toe!\n");
    printf("Press enter to continue\n");
    char enter = 0;
    while (enter != '\r' && enter != '\n') { enter = getchar(); }

    printf("Let's start the game!\n");
    print_board(board);

    while(1)
    {
        printf("Player one: \n");
        player_selection(board, playerOneChar);
        if(is_solved(board, playerOneChar))
        {
            printf("Player one won!\n");
            break;
        }

        if(is_draw(board))
        {
            printf("No winners!\n");
            break;
        }

        printf("Player two: \n");
        player_selection(board, playerTwoChar);
        if(is_solved(board, playerTwoChar))
        {
            printf("Player two won!");
            break;
        }
        if(is_draw(board))
        {
            printf("No winners!\n");
            break;
        }
    }
}

Thank you in advance!

malloc

I start with one of the most common ones. Instead of char *board = malloc(sizeof(char) * BOARD_SIZE) write char *board = malloc(sizeof(*board) * BOARD_SIZE). If you decide to change the type in the future, you don't have to change at more than one place. And besides, sizeof(char) is ALWAYS 1.

But the biggest problem is that you're not checking the return value. It should look like this:

void *init_board()
{
    char *board = malloc(sizeof(char) * BOARD_SIZE);
    if(!board) {
        /* Handle error */
    } else {
        for(int i = 0; i < BOARD_SIZE; i++) {
            board[i] = ' ';
        }
    }
    return board;
}

Or like this:

void *init_board()
{
    char *board = malloc(sizeof(char) * BOARD_SIZE);
    if(board) {
        for(int i = 0; i < BOARD_SIZE; i++) {
            board[i] = ' ';
        }
    }
    return board;
}

But if you choose the latter one, then you need to check the return value of init_board().

Another thing about the board variable. Why not make it into a 3x3 array instead? It's overkill to call malloc for a 9 byte array. I would do like this instead:

const int dim=3;

void init_board(char board[dim][dim])
{
    for(int i=0; i<dim; i++)
        for(int j=0; j<dim, j++)
            board[i][j]=' ';
}

And then in main()

char board[3][3];
init_board(board);

scanf

You're also not checking the return value of scanf. That should also always be done.

scanf(" %d %d", &x, &y);

should be

if(scanf(" %d %d", &x, &y) != 2) {
    /* Handle error */
} else {

But if you ask me, the best method, even though it takes a few more lines, is this:

const size_t buffer_size = 100;
char buffer[buffer_size];
if(!fgets(buffer, buffer_size, stdin)) {
    /* Handle error */
} else {
    if(sscanf(buffer, "%d %d", &x, &y) != 2) {
        /* Handle error */
    }
}

bool

No reason to define bool, true and false on your own. Just include stdbool.h.

const

A minor thing is that you should declare playerOneChar and playerTwoChar as const.

style

This is my personal preference, but I think you waste a lot of space with unnecessary braces. I would at least move the opening brace to then end of previous line, except for functions. Like this:

bool is_draw(char *board)
{
    for(int i = 0; i < BOARD_SIZE; i++) {
        if(board[i] == ' ') {
            return false;
        }
    }
    return true;
}

or even

bool is_draw(char *board)
{
    for(int i = 0; i < BOARD_SIZE; i++)
        if(board[i] == ' ')
            return false;
    return true;
}

Remember that readability also includes not having to scroll more than necessary. Putting the opening brace on the line before or sometimes even removing them completely barely makes it harder to read at all. If you ask me it's even easier. But it can save you a ton of lines, making more code visible at the same time. In the above example I would probably remove the braces for the if statement, but keep the braces for the for loop.

I usually go by the coding style guide for the Linux kernel, except that I prefer a tab size of 4 instead of 8.

Apart from these things, I think it looks pretty good. Nice work.