Arduino based ultrasonic sensor program, output HIGH to control electric motor I am brand new to C++ and am working with the Arduino micro controller. The project I've been working on for my university requires me to write code to do the following:

  If the ultrasonic sensor detects a distance within a certain range, output HIGH to activate an electric motor
  If the sensor detects distance outside this range, outputs LOW to deactivate the motor

I hope to create an array that will average the readings from the ultrasonic sensor over a .5 sec - 1 second interval and then output that average.
I started with a rangefinder script that went along with a tutorial provided on Arduino's website, so my goal is to modify that script to fit into my goals to output HIGH or LOW based on the code that I've tested and already works well to test for distance.

I am looking for any tips to create an array to average the distance from the sensor, then using a for loop to test whether it is in the interval and output HIGH or LOW.

Any help and direction would be appreciated!

const int TriggerPin = 8;  //sensor trigger pin
const int EchoPin = 9;    //sensor echo pin
const int MotorPin = 7;  //motor out pin
long Duration = 0;

void setup(){
  pinMode(TriggerPin,OUTPUT);  //sets trigger as output
  pinMode(EchoPin,INPUT);      //sets echo as input
  pinMode(MotorPin,OUTPUT);    //sets motor as output
  Serial.begin(9600);          //displays to serial monitor
}

void loop(){
 digitalWrite(TriggerPin,LOW);  //trigger pin to 0V
 delayMicroseconds(2);          //waits 2 us
 digitalWrite(TriggerPin,HIGH); //trigger pin to 5V
 delayMicroseconds(10);         //10 us delay to recieve ping
 digitalWrite(TriggerPin,LOW);

Duration = pulseIn(EchoPin,HIGH);  //waits for echo pin to get high
//pulseIn returns the Duration in microseconds
long Distance_mm = Distance(Duration);  //uses function to calc. distance

delay(100);  //delay half second, do next measurement
}

long Distance(long time);
{

long DistanceCalc;  //calculation variable
DistanceCalc = ((time /2.9) / 2);

//want to average this DistanceCalc reading over 10 readings, then use this value to    compare to the desired range

void loop(){
  if  (dist_avg < 600 && dist_avg > 400);
  digitalWrite(MotorPin,HIGH);
  else
  digitalWrite(MotorPin,LOW);
}
//if average distance is within .4-.6 meter range, output HIGH; turns motor on
//if avergae distance is outside .4-.6 meter range, outut LOW; turns motor off

As per C++ naming convention, only user-defined types and macros in C++ are capitalized.  Your constants, global variable, and functions should start with a lowercase letter.
Since your constants are similar, you can group them into an enum instead:

enum Pin { Motor=7, Trigger, Echo };

With the first one set to 7, the following ones will be set to 8 and 9 respectively.

This could also help with code maintenance.  If you ever need to add another Pin, you just need to insert it into the enum appropriately, as opposed to creating another constant.
Duration is only used in loop(), so just initialize and use it there.  Having it as a global variable could invite bugs, and is otherwise bad practice.  This applies to global variables in general because they're accessible throughout the entire program, meaning they can be changed anywhere.
You're using "magic numbers" (hard-coded numbers) in places.  You should either make them variables or constants, or provide comments to specify their meaning.
Your indentation is quite inconsistent.  You indent a different number of spaces in different functions, but you should stick to one (most common is four spaces).  You especially should have indentation within functions, otherwise it may appear that the code isn't contained within that function.
This isn't syntactically correct:

if  (dist_avg < 600 && dist_avg > 400);
digitalWrite(MotorPin,HIGH);
else
digitalWrite(MotorPin,LOW);

There shouldn't be a semicolon after the if condition, otherwise the following statements won't work with it.  The second and fourth lines should also be indented.


Ruby hashes of status counts I have a method that takes a model object, reads through its children and returns a hash of status counts. It should only return a count on a status if one exists.

This method works, but I was wondering if there are a cleaner, more Rubyish way to write it.

The batch_details table is getting very large, so if possible I'd like not to access it twice.

def batch_status
   batch_status = {
      'ready' => 0,
      'processing' => 0,
      'error' =>  0,
      'purchase' => 0,
      'completed' => 0
   }

   self.batch_details.each do |bd|
      batch_status[bd.status] = batch_status[bd.status] + 1
   end
   batch_status['count'] = self.batch_details.count
   batch_status.delete_if {|k, v| v == 0 }
   return batch_status
end

2.1.2 :002 > b.batch_status
 => {"completed"=>4, "count"=>4}

You can use Array#group_by to produce a hash keyed by status. Then follow that by a map to collect the counts. You'll get an array of key/value pairs, so use Hash[] to make that into a proper hash:

def batch_status
  counts = self.batch_details.group_by(&:status).map { |k, v| [k, v.count] }
  Hash[counts]
end

Alternatively, you can use use your current approach, just shortened a bit

def batch_status
  self.batch_details.reduce({}) do |counts, details|
    counts[details.status] = (counts[details.status] || 0) + 1
  end
end

Edit: Completely overlooked the "count" key. I muddies the picture a little, but you could do something like this for code blocks 1 and 2 above

def batch_status
  counts = self.batch_details.group_by(&:status).map { |k, v| [k, v.count] }
  counts << ["count", self.batch_details.count]
  Hash[counts]
end

and

def batch_status
  counts = { "count" => self.self.batch_details.count }
  self.batch_details.reduce(counts) do |counts, details|
    counts[details.status] = (counts[details.status] || 0) + 1
    counts
  end
end

Or, as Cary Swoveland suggests:

def batch_status
  counts = { "count" => self.self.batch_details.count }
  counts.default = 0
  self.batch_details.reduce(counts) do |counts, details|
    counts[details.status] += 1
    counts
  end
end

Which cleans it up nicely. Further cleanup could be done by using each_with_object in place of reduce:

def batch_status
  counts = { "count" => self.self.batch_details.count }
  counts.default = 0
  self.batch_details.each_with_object(counts) do |counts, details|
    counts[details.status] += 1
  end
end


Setting output width with ostream_iterator I'd like to display a vector (implemented in my custom Vector class) and have the output width of the elements be configurable. I've done this by creating a simple wrapper for the element being displayed which stores the desired width in a static field. I'm not sure if this is the best approach.

/** A struct for formatting doubles with a fixed width. */
struct FixedWidthDouble {
  FixedWidthDouble(double x) : x_(x) {}
  double x_;
  static int width_;
};

/** Apply a fixed width manipulator to a double for display. */
std::ostream& operator<<(std::ostream& out,
    const FixedWidthDouble &fixedWidthDouble) {
  return out << std::setw(fixedWidthDouble.width_) << fixedWidthDouble.x_;
}

Here's an example of it being applied:

/** Convert a Vector into a stream suitable for display. */
std::ostream& Vector::Print(std::ostream& out, int width) const {
  out << "[" << std::setprecision(1) << std::fixed;
  FixedWidthDouble::width_ = width;
  std::copy(begin(), end(), std::ostream_iterator<FixedWidthDouble>(out, " "));
  return out << " ]";
}

Is there a cleaner way to do this?

Proposed Solution

Based on feedback from Begemoth this is the solution I decided to go with. Does this look reasonable?

template<int width>
struct FixedWidthDouble {
  FixedWidthDouble(double x) : x_(x) {}
  double x_;
};

/** Apply a fixed width manipulator to a double for display. */
template <int width>
std::ostream& operator<<(std::ostream& out,
    const FixedWidthDouble<width> &fixedWidthDouble) {
  return out << std::setw(width) << fixedWidthDouble.x_;
}

/** Convert a Vector into a stream suitable for display. */
template <int width>
std::ostream& Vector::Print(std::ostream& out) const {
  out << "[" << std::setprecision(1) << std::fixed;
  std::copy(begin(), end(),
      std::ostream_iterator<FixedWidthDouble<width> >(out, " "));
  return out << " ]";
}

There are one problem with your approach you make local information global as consequence your FixedWidthDouble class is not thread-safe. There is a simple way to solve the problem by turning width_ into a integral template argument of the class template template<int width> FixedWidthDouble.

In C++11 I'd use range-based for:

std::ostream& Vector::Print(std::ostream& out, int width) const
{
  out << "[" << std::setprecision(1) << std::fixed;
  for (auto& x : *this)
    out << std::setw(width) << x;
  return out << " ]";
}

In the old C++98 it is cleaner to use old for with iterators.  Also you can create a new iterator type that will set the width:

template<typename T, typename CharT = char, typename Traits = std::char_traits<CharT> >
class width_ostream_iterator
  : public std::iterator<std::output_iterator_tag, void, void, void, void>
{
public:
  typedef CharT char_type;
  typedef Traits traits_type;
  typedef std::basic_ostream<CharT, Traits> ostream_type;

private:
  ostream_type* stream;
  const CharT* separator;
  int width;

public:
  explicit width_ostream_iterator(ostream_type& stm, int width, const CharT* separator = 0)
    : stream(&stm), separator(separator), width(width)
  { }

  width_ostream_iterator& operator=(const T& value)
  {
    *stream << std::setw(width) << value;
    if (separator) *stream << separator;
    return *this;
  }

  width_ostream_iterator& operator*()     { return *this; }
  width_ostream_iterator& operator++()    { return *this; }
  width_ostream_iterator& operator++(int) { return *this; }
};

Update: Your proposed solution is valid if the width can only be changed at compile time, if you want to change the width at the runtime (e.g. from configuration file or command line arguments) you have to use plain for loop or the width_ostream_iterator I've provided.  One thing I do not like about the width_ostream_iterator — it is definitely a library entity intended to be used in multiple places but it allows you only to change the width of the field and nothing else.  It is too specific for the library (it does not allow an arbitrary manipulator to a stream before outputting a value) it is too complicated for a single use.

And one more note: the Print function uses only public interface of the vector (I assume that begin and end functions are public), so it does not required to be a member function.  Prefer to make such functions free not member.  Why not use member function for all class operations? You can extend the interface of a class without modifying it, e.g. you can have a Vector class and stream I/O operations defined in separate headers, you can later add another I/O operations (e.g. for QDataStream) in a third header and you will not have to recompile the parts of the program that depend only on the first and second headers.  Your Print function can potentially work with any class which looks and behaves like a standard sequence, just like any algorithm from the standard library.  The class (just a function) must do one thing well, the Vector class stores a number of values of some type and provides constant-time random-access to the elements, it doesn't sort elements (that what std::sort algorithm is for), it does not search for elements (that what std::find and std::find_if algorithms are for), it does nit provide elementwise arithmetic operations (oops, there is no std::zip to combine several sequences into one), etc.


SpaceSaving frequent item counter in Python In the code below, I implemented the SpaceSaving frequency estimation algorithm described in this paper. Given a parameter eps, the algorithm finds all elements of a data stream of length n that occur more than n/eps times (with high probability). Here's a screenshot from the paper with the pseudocode:

I would appreciate any feedback on my implementation: style, performance, etc.

import math, heapq

class SpaceSavingCounter:
    def __init__(self, eps):
        self.k = math.ceil(1/eps)
        self.n = 0
        self.counts = dict()
        self.queue = []

    def inc(self, x):
        # increment total elements seen
        self.n += 1

        # x is being watched
        if x in self.counts:
            self.counts[x] += 1

        # x is not being watched
        else:
            # make room for x
            if self.n > self.k:
                while True:
                    count, tstamp, key = self.pop()
                    assert self.counts[key] >= count
                    if self.counts[key] == count:
                        del self.counts[key]
                        break
                    else:
                        self.push(self.counts[key], tstamp, key)
            else:
                count = 0

            # watch x
            self.counts[x] = count + 1
            self.push(count, self.n, x)

    def push(self, count, tstamp, key):
        heapq.heappush(
            self.queue,
            (count, tstamp, key)
        )

    def pop(self):
        return heapq.heappop(self.queue)

def test_SpaceSavingCounter():
    seq = [1,5,3,4,2,7,7,1,3,1,3,1,3,1,3]
    counter = SpaceSavingCounter(1 / 1.9)
    for x in seq:
        counter.inc(x)
    assert counter.counts.keys() == {1,3}

Thanks for putting this code online! Two updates for people who may want to use it:

Correctness: The "if self.n > self.k:" is probably mistaken: for instance, when the sequence starts with k times the same element, we are later stuck with a data structure that contains a single counter. Instead, we can use self.k as representing the number of remaining counters with value 0: specifically, replace this line by "if self.k==0:" and just after the "else:" insert "self.k=self.k-1".
Small optimization: above "def push", replace by "self.push(self.counts[x], self.n, x)", i.e. use the latest value of x since we just updated it


Allow users to add custom queries to Mustache variables I'm writing an application that will be managed by my team, it uses Mustache.php as template engine.

I have a PHP file that makes an array of variables and functions that is passed to the Mustache parser to populate the template.

I even provide a system to override partials of my template to edit the aspect of the application for specific users.

Now I'd like to add a way to allow my team to add custom variables to the array passed to Mustache.php without have to hack the core of my application.

I thought about a little function that checks if a .php file with the name of the user exists and if so, include it after I've generated the array and before I pass it to mustache.

It should looks like:

contents.php:

// my queries
$variables = Array(1,2,3,bla,foo,bar);

if (file_exists($_SERVER['BASE_DIR'] . '/customcontents/'. $username . '.php')) {
   include($_SERVER['BASE_DIR'] . '/customcontents/'. $username . '.php');
}

// Call mustache and pass $variables to it

<username>.php:

// custom queries
$variables = array_merge($variables, $customvariables)

I'm not sure if this can be a nice solution or if you guys have some better idea.

I don't know anything about Mustache, so just a note about the code: $_SERVER['BASE_DIR'] . '/customcontents/'. $username . '.php' is duplicated, it could be extracted out to a local variable:

$userFile = $_SERVER['BASE_DIR'] . '/customcontents/' . $username . '.php'
if (file_exists($userFile)) {
   include($userFile);
}

(You might have ideas for a better name.)

Reference: Chapter 6. Composing Methods, Introduce Explaining Variable in Refactoring: Improving the Design of Existing Code by Martin Fowler:

  Put the result of the expression, or parts of the expression,
  in a temporary variable with a name that explains the purpose.

And Clean Code by Robert C. Martin, G19: Use Explanatory Variables.


Executing multiple async methods synchronously without throwing I have a following task: execute multiple asynchronous methods synchronously and always execute them all, even when one of them throws. Rethrow thrown exceptions if any occured only after all tasks have finished. Here is my code:

public static async Task ContinueWithoutThrowing(this IEnumerable<Func<Task>> tasks)
    {
        var thrownExceptions = new List<Exception>();

        foreach (var task in tasks)
        {
            try
            {
                await task().ConfigureAwait(false);
            }
            catch (Exception ex)
            {
                thrownExceptions.Add(ex);
            }
        }

        if (thrownExceptions.Any())
        {
            throw new AggregateException(thrownExceptions);
        }
    }

Usage of this code is following:

Func<Task>[] tasks = { First, Second };
await tasks.ContinueWithoutThrowing().ConfigureAwait(false);

I am not sure whether what I am doing is accomplishable using already implemented methods on Task class like WhenAll. But using WhenAll with await throws on first exception. Also couldn't figure out some better name than ContinueWithoutThrowing when it actually throws if used with await (but its wanted behavior). I know this code doesn't handle cancellation but its not my main concern right now.

After my initial answer and based on your subsequent comments, I would say that your implementation is OK.

As for naming I would keep it simple like WhenAllCompleted

/// <summary>
/// Creates a task that will complete when all of the supplied tasks have completed synchronously.
/// </summary>
/// <param name="tasks">The tasks to wait on for completion.</param>
/// <returns>A task that represents the completion of all of the supplied tasks.</returns>
public static async Task WhenAllCompleted(this IEnumerable<Func<Task>> tasks) {
    if (tasks == null) throw new ArgumentNullException("tasks");
    var exceptions = new List<Exception>();
    using (var e = tasks.GetEnumerator()) {
        while (e.MoveNext()) {
            var task = e.Current;
            try {
                await task();
            } catch (Exception ex) {
                exceptions.Add(ex);
            }
        }
    }
    if (exceptions.Any()) {
        throw new AggregateException(exceptions);
    }
}

I've basically only added the null check on the provided argument. I considered doing null checks on the function and task returned like

var function = e.Current;
if (function == null) {
    exceptions.Add(new ArgumentException("The tasks collection contained a null task.", "tasks"));
    continue;
}
//...

but figured that would all be caught by your simpler implementation, so decided against it.

Though you have not added cancellation as yet the following should be of some help with an optional cancellation token

/// <summary>
/// Creates a task that will complete when all of the supplied tasks have completed synchronously.
/// </summary>
/// <param name="tasks">The tasks to wait on for completion.</param>
/// <returns>A task that represents the completion of all of the supplied tasks.</returns>
public static async Task WhenAllCompleted(this IEnumerable<Func<Task>> tasks, CancellationToken token = default(CancellationToken)) {
    if (tasks == null) throw new ArgumentNullException("tasks");
    var exceptions = new List<Exception>();
    using (var e = tasks.GetEnumerator()) {
        while (e.MoveNext()) {
            if (token.IsCancellationRequested) {
                exceptions.Add(new OperationCanceledException("tasks collection was cancelled", token));
                break;
            }
            try {
                await e.Current();
            } catch (Exception ex) {
                exceptions.Add(ex);
            }
        }
    }
    if (exceptions.Any()) {
        throw new AggregateException(exceptions);
    }
}


Private method to validate Excel header I am new to unit testing and I am unsure how to design classes that use third party libraries, so that I can easily test them. The example I'll use is with EPPlus-OfficeOpenXml (an Excel document manipulator). I have a class to import excel documents and validate them.

Here is a sample of my code:

Imports OfficeOpenXml
Public Class TestImporter

    Private errorList As New List(Of String)

    Public Function import(fileName As String) As List(Of String)
        Dim openFile As New FileInfo(fileName)
        Dim document As New ExcelPackage(openFile)
        Dim sheet As ExcelWorksheet = document.Workbook.Worksheets(1)

        validateAll(sheet)

        Return errorList
    End Function

    Private Sub validateAll(sheet As ExcelWorksheet)
        validateHeaders(sheet)
    End Sub

    Private headers As String() = {"Header 1", "Header 2", "Header 3", "Header 4", "Header 5"}

    Private Sub validateHeaders(sheet As ExcelWorksheet)
        For i As Integer = 1 To headers.Length
            If Not (sheet.Cells(1, i).Value = headers(i - 1)) Then
                errorList.Add(String.Format("The header '{0}' does not match '{1}'", sheet.Cells(1, i).Value, headers(i - 1)))
            End If
        Next
    End Sub
End Class

The validation subroutines are private, because the only action I want to expose for this class is the import functionality.

How should I design this class in order to test my validation of Excel inputs? My first thought is have a test file for each test. Then pass in the name of the test file into the import function in each test. This doesn't seem to be the correct solution, and seems kind of hackish.

It's basically like RubberDuck said.

  [...] the only action I want to expose for this class is the import functionality.

This means the validation logic is an implementation detail of the TestImporter class. When you test a unit, you only care about the public interface, and treat private members (which are called by the public ones) as implementation details; not testing them directly is fine, unless you do want to test that logic separately...

  How should I design this class in order to test my validation of Excel inputs?

So you want to test the validation logic? Great! Treat it as a unit then (as opposed to an implementation detail), and extract it into its own class.

You will want to decouple the validation logic from the TestImporter class though, and it's easier to do that if you code against an abstraction. Make a TestImporter constructor inject the dependency:

Private ReadOnly HeaderValidator As IHeaderValidationLogic

Public Sub New(validator As IHeaderValidationLogic)
    HeaderValidator = validator
End Sub

Now, instead of calling validateAll, you call into the IHeaderValidationLogic interface:

validationErrors.AddRange(HeaderValidator.Validate(sheet))

But this creates another problem: does the validation logic really needs to be coupled with an ExcelWorksheet object? What if you switched to the VSTO interop assemblies and worked off a Microsoft.Office.Interop.Excel.Worksheet object instead, would the logic be any different?

Best would be to abstract away that "worksheet", and make the validation logic work off what it actually validates: the contents of the worksheet.

Extract the values of range $A$1:$E$1 into an array, and pass that array (of strings?) to your header validation logic. Assuming we're returning a list of error messages, the IHeaderValidationLogic interface could look like this:

Public Interface IHeaderValidationLogic
    Function Validate(headerContents As IEnumerable(Of String)) As IReadOnlyList(Of String)
End Interface

Now, returning a list of error messages isn't very .NET-like. Why not do this instead?

Public Interface IHeaderValidationLogic
    Sub Validate(headerContents As IEnumerable(Of String))
End Interface

And then the implementation can throw some ValidationException, with whatever string content you want it to have.

And that exception would bubble up the stack to Import(fileName As String), which could now be a Sub - because again, returning errors isn't very .NET-like.

Note that your method names should be PascalCase, to follow the established VB.NET naming conventions.

If you're going to want to write a unit test for the actual Import procedure, you'll need to reduce coupling - otherwise your tests will incur I/O, which slows them down and makes them dependent on the file system, which isn't ideal.

Extract another interface.

Public Interface IExcelPackageProvider
    Function Open(fileName As String) As ExcelPackage
End Interface

Now you can mock this interface and make the Open method return a stub ExcelPackage with a fake ExcelWorksheet object that you've setup for your tests, and as a side-effect of doing this, you've extracted a highly specialised and reusable component from your Import procedure, making the class more cohesive and a little less coupled.

Private ReadOnly WorkbookProvider As IExcelPackageProvider
Private ReadOnly HeaderValidator As IHeaderValidationLogic

Public Sub New(provider As IExcelPackageProvider, validator As IHeaderValidationLogic)
    WorkbookProvider = provider
    HeaderValidator = validator
End Sub

It's now the job of WorkbookProvider to deal with file I/O and return a workbook object for the Import method to work with.


Deque Container with an instant consumer for high speed object dumping I currently have a container that has two features it allows adding objects to it and at the same time an independent thread continuously consumes items from it. It is a FIFO type deque. Which receives object at an extremely high rate. Following is the code for the container

std::deque<Some_object*> my_container;

void Container::Add_item(Some_object* obj)
        {
            try
            {
                {//Begin Lock
                    boost::lock_guard<boost::mutex> lock(mutex_lk);
                    my_container.push_back(obj);
                }
                condition_consumer_silo.notify_one();
            }
            catch (std::exception& e)
            {
                __debugbreak();
            }
        }

        //This method continuously consumes object in FIFO order
        void Container::Consume_Object()
        {
            while(true)
                {
                    Some_object* obj;
                    {//Lock
                        try
                        {
                            boost::unique_lock<boost::mutex> lock(mutex_lk);
                            while(my_container.empty()) { condition_consumer_silo.wait(lock); }//Block if empty - for spurious wakeup

                            if(!my_container.empty())
                            {
                                obj = my_container.front();
                                my_container.pop_front();
                            }
                        }
                        catch (std::exception& e)
                        {
                            __debugbreak();
                        }

                    }//Unlock
                    try
                    {
                            Consume_obj(obj);

                     } catch (std::exception& e)
                     {
                        __debugbreak();
                     }
                }//end while
        }

Could this container be improved ? Is there a library out there that would do a better job.

Ownership symantics:
void Container::Add_item(Some_object* obj)

Who owns obj?
This is a C-Style interface. Don't do it.
It would be better to pass in as unique_ptr. This shows transfer of ownership. Also with good compiler optimization is no more expensive than a normal pointer. This also results in slight modification in your code to make sure it handles ownership correctly.
Note: I would still use: std::deque<Some_object*> my_container;
Lockless queues
Lock are very expensive (relatively to queue management). There is a concept of thread safe lock-less queues.
If you have a very high frequency modification to the queue then that is something I would look into. see How do I build a lockless queue?


Speeding up evaluation of power series, Python I want to plot (on the complex plane, with mpmath) the power series whose general term is the number of partitions of an integer.

The more points I want (and the larger the degree is), the longer the plot takes.

I first wrote a naive version, added memoization of the sequence, changed the naive evaluation of the polynomial with the Horner algorithm. All these modifications led to speed ups, and I want to know if there are more optimizations I could do.

import mpmath

MAX_DEGREE = 250
NB_POINTS = 40000

def memoize(function):
    memo = {}

    def wrapper(*args):
        if args in memo:
            return memo[args]
        else:
            rv = function(*args)
            memo[args] = rv
            return rv

    return wrapper

@memoize
def p_k(k, n):
    if k == 1 or k == n:
        return 1
    if k > n:
        return 0

    return p_k(k - 1, n - 1) + p_k(k, n - k)

@memoize
def p(n):
    res = 1
    for k in range(1, n):
        res = res + p_k(k, n)
    return res

def mgf(x):
    if abs(x) >= 1:
        return 0
    res = 0
    x_i = x
    for i in range(1, MAX_DEGREE):
        res = res + p(i) * x_i
        x_i = x_i * x
    return res

def mgf_horner(x):
    if abs(x) >= 1:
        return 0
    res = p(MAX_DEGREE)
    for i in reversed(range(1, MAX_DEGREE)):
        res = res * x + p(i)
    return res * x

if __name__ == "__main__":
    assert(p(1) == 1)
    assert(p(2) == 2)
    assert(p(5) == 7)
    assert(p(7) == 15)
    assert(round(mgf_horner(0.3), 8) == round(mgf(0.3), 8))
    assert(round(mgf_horner(0.5), 8) == round(mgf(0.5), 8))

    print("Tests passed!")

    mpmath.cplot(mgf_horner, points=NB_POINTS, re=[-1, 1], im=[-1, 1])

I think most savings are already achieved by the memoization. If you deconstruct the algorithms, your mgf and mgf_horner don't seem to do a lot different. The main difference is the direction of the summation, which affects the float rounding errors.

some small changes:

instead of reversed(range(1, MAX_DEGREE)) you can use range(MAX_DEGREE - 1, 0, -1)
you can use the built-in sum or math.fsum if you rewrite the formula's a bit

something like this:

@lru_cache(maxsize=MAX_DEGREE)
def p2(n):
    return sum((p_k(k, n) for k in range(1, n)), 1)

def mgf_fsum(x):
    if abs(x) >= 1:
        return 0
    return math.fsum(p2(i) * x ** i for i in range(1, MAX_DEGREE))

def mgf_horner_fsum(x):
    if abs(x) >= 1:
        return 0
    return math.fsum(p2(i) * x ** i for i in range(MAX_DEGREE, 0, -1))

def mgf_sum(x):
    if abs(x) >= 1:
        return 0
    return sum(p2(i) * x ** i for i in range(1, MAX_DEGREE))

def mgf_horner_sum(x):
    if abs(x) >= 1:
        return 0
    return sum(p2(i) * x ** i for i in range(MAX_DEGREE, 0, -1))

but this rounding has an effect on the results:

steps = 15
for x in range(1, steps, 1):
    print(x/steps, mgf(x/steps), mgf(x/steps)/mgf(x/steps), mgf_sum(x/steps)/mgf(x/steps), mgf_fsum(x/steps)/mgf(x/steps), mgf_horner(x/steps)/mgf(x/steps), mgf_horner_sum(x/steps)/mgf(x/steps), mgf_horner_fsum(x/steps)/mgf(x/steps))

x mgf_value mgf mgf_sum mgf_fsum mgf_horner mgf_horner_sum mgf_horner_fsum
0.06666666666666667 0.07655349092586666 1.0 1.0 1.0 1.0 1.0 1.0
0.13333333333333333 0.17795095671821595 1.0 1.0 1.0 1.0 1.0 1.0
0.2 0.3152135557353453 1.0 1.0 0.9999999999999998 0.9999999999999998 0.9999999999999998 0.9999999999999998
0.26666666666666666 0.5067809825345801 1.0 1.0 1.0000000000000007 1.0000000000000007 1.0000000000000007 1.0000000000000007
0.3333333333333333 0.7853123419985341 1.0 0.9999999999999997 1.0 0.9999999999999999 1.0000000000000002 1.0
0.4 1.2130721483139313 1.0 1.0 1.0 0.9999999999999998 1.0 1.0
0.4666666666666667 1.9206394911829312 1.0 1.0 1.0000000000000004 1.0000000000000004 1.0000000000000004 1.0000000000000004
0.5333333333333333 3.2187202179248864 1.0 1.0 1.0000000000000004 1.000000000000099 1.000000000000099 1.000000000000099
0.6 5.9870712939827015 1.0 1.0 0.9999999999999999 1.000000000201781 1.0000000002017808 1.000000000201781
0.6666666666666666 13.435823754250293 1.0 0.9999999999999999 1.0000000000000002 1.0000001435010422 1.0000001435010424 1.0000001435010424
0.7333333333333333 43.087235122138615 1.0 1.0000000000000002 1.0 1.0000353393606511 1.0000353393606507 1.000035339360651
0.8 288.2244586544328 1.0 1.0000000000000004 1.0 1.0023338650736258 1.002333865073626 1.002333865073626
0.8666666666666667 6672.576615237916 1.0 0.9999999999999998 0.9999999999999994 1.0273438993019854 1.0273438993019859 1.0273438993019859
0.9333333333333333 379389.61410647386 1.0 0.9999999999999997 0.9999999999999999 1.0860986641621089 1.086098664162109 1.086098664162109

for these tests I limited MAX_DEGREE to 70.

As you can see, the 3 methods with increasing iteration give a different result than the 3 methods with decreasing iterations, especially with values closer to 1

Once the p(x) are calculated and in memory, this goes pretty fast, and no noticeable difference between the algorithms


Converting a range in excel to a single column with excel vba I am very new to coding and know only the basics.  The first part of this code is running fine.  It converts a range of values to a single column.  However, with my data set the rows of data step down, as shown in the sample data set below, so that when they are converted to a single column there are large gaps of 0 values in the column.  I added a portion of code to the end to look at each cell in the column and delete any 0 values.  The problem is this code takes around 4-5 hours to run.  I am hoping there is a better way to write the code to speed up the processing time.

Any help is appreciated!

Sub CombineColumns()

Application.ScreenUpdating = False
Application.EnableEvents = False
Application.Calculation = xlCalculationManual

Dim rng As Range
Dim iCol As Long
Dim lastCell As Long
Dim k As Long

k = 484
'set K equal to the number of data points that created the range

Set rng = ActiveCell.CurrentRegion
lastCell = rng.Columns(1).Rows.Count + 1

For iCol = 2 To rng.Columns.Count
    Range(Cells(1, iCol), Cells(rng.Columns(iCol).Rows.Count, iCol)).Cut
    ActiveSheet.Paste Destination:=Cells(lastCell, 1)
    lastCell = lastCell + rng.Columns(iCol).Rows.Count

Next iCol
Dim z As Long
Dim m As Long

z = k ^ 2

For row = z To 1 Step -1
    If Cells(row, 1) = 0 Then
    Range("A" & row).Delete Shift:=xlUp

   Application.StatusBar = "Progress: " & row & " of z: " & Format((z - row) / z, "Percent")
   DoEvents

    End If

Next

Application.StatusBar = False
Application.ScreenUpdating = True
Application.EnableEvents = True
Application.Calculation = xlCalculationAutomatic

End Sub

4-5 hours is ridiculous

4-5 hours with ScreenUpdating, Events, & Calculation disabled is even more so.

What you've discovered here is that Excel is very slow at inserting/deleting columns/rows when you have large amounts of data in a Worksheet.

and you're doing it up to 235,000 times.

Delete everything in a single operation

What we're going to do here is loop through Ranges and, as we go, add all the Ranges-to-be-deleted to one master Range using the Union() function.

Then, at the end, delete the entire master range in one go:

Dim rowsToBeDeleted As Range '/ our master delete range

For row = Z To 1 Step -1

    If Cells(row, 1) = 0 Then

        If rowsToBeDeleted is Nothing Then '/ check if any ranges have been added yet
            Set rowsToBeDeleted = Range("A" & row) '/ add the first range
        Else
            Set rowsToBeDeleted = Union(rowsToBeDeleted, Range("A" & row)) '/ add the new range to the existing ones
        End If

    End If

Next row

If Not rowsToBeDeleted Is Nothing Then '/ check that we found anything to delete
    rowsToBeDeleted.EntireRow.Delete
End If

I suspect that this change alone will take your runtime from hours to minutes.

(Just as an aside, worth noting that a Range object can only have up to 1,048,576 range areas. So if you ever get up to more than 1,024^2, you'll have to check against it.)


Emacs Etags Shortcut Functions A few functions to let me manage TAGS files more easily. Typically, my projects contain at least one sub-folder. I got sick of manually updating, so I wrote this to help me update a single TAGS file per project (always in the projects' root directory).

Please share your thoughts (aside from "Why are you using Emacs?", you're not likely to change my mind).

One thing I've noticed that could trip me up is that it only tags files of one language (and if I had a project that had both Haskell and JS code, for example, I could tag one or the other). I would particularly appreciate advice on better ways to write find-parent-tags than just building each path from available sub-strings.

(defun create-tag-table ()
  "This will recursively tag all files of a given type starting with the current buffers' directory.
   It overwrites the old TAGS file, if one exists.
   Haskell files assume you've installed `hasktags`."
  (interactive)
  (let ((file-type (get-taggable-extension))
        (cur-dir default-directory)
        (tags-file (find-parent-tags default-directory)))
    (if (equalp file-type "hs")
        (shell-command "hasktags --ignore-close-implementation --etags `find . -type f -name \"*.*hs\"`")
          (shell-command (concat "find -name \"*." file-type "\" -print | etags -")))))

(defun find-parent-tags (dir)
  "Traverses the directory tree up to /home/[user]/ or / whichever comes first.
   Returns either nil or the directory containing the first TAGS file it finds."
  (interactive (list default-directory))
  (find-parent-tags-rec (build-tag-paths dir)))

(defun find-parent-tags-rec (list-of-filepath)
  (cond ((null list-of-filepath) nil)
        ((file-exists-p (car list-of-filepath)) (car list-of-filepath))
        (t (find-parent-tags-rec (cdr list-of-filepath)))))

(defun build-tag-paths (dir-string)
  (build-tag-paths-rec (remove-if #'empty-string? (split-string dir-string "/")) (list "/")))

(defun build-tag-paths-rec (steps acc)
  (if (null steps)
      (mapcar (lambda (p) (concat p "TAGS")) acc)
    (build-tag-paths-rec (cdr steps)
             (cons (concat (car acc) (car steps) "/") acc))))

(defun get-taggable-extension ()
  "Either returns the current file's extension (if it's appropriate) or asks the user to pick one with completion"
  (let ((b-name (buffer-file-name (current-buffer)))
        (valid-exts (list "lisp" "py" "c" "hs" "rb" "ss" "scm" "js" "erl" "el")))
    (string-match "\\.\\(.*?\\)$" b-name)
    (let ((current-filetype (match-string 1 b-name)))
      (if (member current-filetype valid-exts)
          current-filetype
        (completing-read "File type: " valid-exts nil 'confirm)))))

(defun empty-string? (s) (equalp s ""))

(provide 'tagariffic)

General notes

Always prefix your functions with the name of your package or project, e.g. tagariffic-create-tag-table and so on. Emacs has no package scoping.
Whenever possible (i.e. in the absence of mutual recursion), define your functions before using them. It's a lot clearer for the reader. It's also necessary if you ever define macros.
You're using some functions from the cl package, so you should start with (require 'cl).
The usual style for predicates in Lisp is name-p, not name? (that's Scheme).
Emacs has functions to manipulate file and directory names. Use them rather than going around concatenating slashes; this way your code will work on exotic platforms such as Windows.

Finding the tags file

Emacs 23 has a function locate-dominating-file (in files.el).

  Look up the directory hierarchy from FILE for a file named NAME.
  Stop at the first parent directory containing a file NAME,
  and return the directory.  Return nil if not found.

Your (find-parent-tags default-directory) is almost equivalent to (locate-dominating-file (buffer-file-name) "TAGS"). If you want to use the current directory rather than the directory of the current buffer's file (they're almost always the same), use (locate-dominating-file (concat (default-directory) ".") "TAGS"). locate-dominating-file stops at your home directory, too.

The code you posted does nothing with the tags-file variable. Did you mean to run the tag generation command in that directory, or failing that in the current directory? If so, this should do it:

(let ((default-directory (if tags-file (file-name-directory tags-file) default-directory)))
  (shell-command …))

From file type to shell commands

Rather than build in an exception for Haskell, the usual style would be to define an alist from extensions to commands.

(defvar tagariffic-command-alist
  '(("hs" . "… hasktags …")
    (t . "… etags …")))

However, there's a better approach than separate commands for each type; see below.

The shell commands

Your hasktags command will choke on file or directory names that contain shell special characters: \[?* and whitespace. This isn't very common in source trees, but it could happen. Generally speaking, never use command substitution to generate a list of file names. Instead, let find call the command.

find . -type f -name '*.hs' -exec hasktags --ignore-close-implementation --etags {} +

A second problem is that if there are too many files, the command you wrote will die of a command line too long error; the one I just showed will run hasktags several times with command lines of a permitted size, causing the tag file to be overwritten. As a workaround, start by creating an empty tags file and tell hasktags to append to the tags file.

: >TAGS
find . -type f -name '*.hs' -exec hasktags --ignore-close-implementation -e -a {} +

An improvement to the find commands would be to ignore certain directories belonging to version control systems. Especially with svn, this could speed things up.

find . -type d \( -name .bzr -o -name .git -o -name .hg -o -name .svn -o -name CVS -o -name _darcs \) -prune -o \
       -type f …

Above I mentioned a different approach to choosing the tag generation command. Your approach assumes that a given project will be in a single language. It's easy enough to cater for mixed-language projects: build a single tags file containing the output generated by etags and hasktags.

: >TAGS
find . -type d \( -name .bzr -o … \) -prune -o \
       -type f -name '*.hs' -exec hasktags -e -a {} + -o \
       -type f \( -name '*.[CScls]' -o -name '*.el' -o … \) -exec etags {} +


ReactJS, Material UI component and code organization This is my first post here so I hope you will be patient if I'm doing something wrong. I made a small website with create-react-app and I would like to request feedback about. This website was a test and I had a limited amount of time to complete it, hence there are no tests nor license, which for the test purposes weren't necessary.

The main things I would like to have feedback on are:

1) Are my components organized properly?

2) Is the pattern I'm using to develop components appropriate?

Although I'm posting here some code, I would like to ask if you could take a quick look at my github repository and let me know what you think about it: https://github.com/sickdyd/foriio

For the components organization I used the following structure:

To avoid having multiple nested folders, I decided to put all of the sub components that belong to a specific component on the same level. The naming should give away the function, and the folder name has the same name as the main component.

One quick question: the component src/components/body/UserProfile/UserWorks.js is reused in src/components/body/UserWork/UserWork.js; is it ok to leave them with this structure?

The components generally have the following code structure (this is one of the simplest components):

import React from 'react';

import PropTypes from 'prop-types';

import Typography from '@material-ui/core/Typography';
import Skeleton from '@material-ui/lab/Skeleton';

import { makeStyles } from '@material-ui/styles';

const useStyles = makeStyles(theme => ({
  bio: {
    display: "flex",
    justifyContent: "center",
    marginTop: theme.spacing(2),
    color: theme.palette.text.primary,
  },
}));

export default function UserBio(props){

  const classes = useStyles();

  const {
    loading,
    profile,
  } = props;

  return (

    loading ?

        <Skeleton variant="rect" style={{ marginBottom: 12, width: 315 }} />
        <Skeleton variant="rect" style={{ marginBottom: 12, width: 315 }} />
        <Skeleton variant="rect" style={{ marginBottom: 12, width: 315 }} />

    :

        <Typography>
          { profile.bio }
        </Typography>

  )
}

UserBio.propTypes = {
  loading: PropTypes.bool.isRequired,
  profile: PropTypes.object,
}

Your file structure is okay, though it is not the "conventional" react folder structure. I personally like your approach better than having lots of nested folders with index.js but this is because my workflow relies on searching files by name, and searching for index.js is impossible — you always have to search by file.

  One quick question: the component src/components/body/UserProfile/UserWorks.js is reused in src/components/body/UserWork/UserWork.js; is it ok to leave them with this structure?

As I understand, UserWorks is some component that shows some sort of preview of works; I think that the structure should be the other way around, UserProfile should depend on UserWorks not the other way around. You might even structure it explicitly that UserProfile and UserWork pages (components) depend on a representation component UserWorks, though naming becomes a bit cumbersome.

Note on using export default

It seems to be standard "good" practice to use export default in Javascript, but I personally don't like it for various reasons and one of them shows in UserWork: you are importing UserWorks as UserRelatedWorks this suggests that UserWorks is used with different intention that is originally designed. Named imports would not eliminate this issue completely but would at least force explicit statement import {UserWorks as UserRelatedWorks} from …

Comments on the code:

lots of unnecessary empty lines, this is a bit unusual. In most cases empty lines are used to group statements together, here they appear to be random and make code harder to read.
you are using classes for div.bio but not for Skeleton though those styles are repeated but bio is not (or should not be)
no matter value of loading you are returning a container with <div className={classes.bio}>, you can return container and resolve content inside of it based on the loading to avoid duplication.

In this code filter method is used incorrectly, the callback should return boolean but in this case returns the object itself or null, this relies on implicit type casting to truthy value and is very confusing

          works.filter(w => {

            if ((!category) || (category === "all")) {
              return w;
            }
            else {
              if (w.category_list.includes(category)) return w;
            }

            return null;

          })

this code should be

works.filter(work => !category || category === 'all' || w.category_list.includes(category))


Generics for reducing redundancies I have a routine that can be potentially reduced.

internal ArrayBufferObject GetAccessorArray()
{
    ColladaDocument colladaDocument = GetColladaDocument();
    ColladaArray colladaArray = colladaDocument.GetSpecialElement<ColladaArray>(Source);
    ArrayBufferObject bufferObject;

    if (IsSimpleParamsConfiguration() == false)
        throw new NotSupportedException("complex accessor params not supported");

    string simpleParamsType = Params[0].Type;

    if (simpleParamsType == ArrayTypeFloat) {
        uint cursorOffset = Offset;

        ArrayBufferObject<float> arrayBufferObject = new ArrayBufferObject<float>(BufferObject.Hint.StaticCpuDraw);

        for (uint i = 0; i < Count; i++) {
            foreach (ColladaParam param in Params) {
                if (param.Name != null)
                    arrayBufferObject[cursorOffset - Offset] = (float) colladaArray[cursorOffset];
                cursorOffset++;
            }
            cursorOffset += Stride - (uint)Params.Count;
        }

        bufferObject = arrayBufferObject;

    } else if (simpleParamsType == ArrayTypeInt) {
        uint cursorOffset = Offset;

        ArrayBufferObject<int> arrayBufferObject = new ArrayBufferObject<int>(BufferObject.Hint.StaticCpuDraw);

        for (uint i = 0; i < Count; i++) {
            foreach (ColladaParam param in Params) {
                if (param.Name != null)
                    arrayBufferObject[cursorOffset - Offset] = (int) colladaArray[cursorOffset];
                cursorOffset++;
            }
            cursorOffset += Stride - (uint)Params.Count;
        }

        bufferObject = arrayBufferObject;
    } else
        throw new NotSupportedException(String.Format("simple accessor params type {0} not supported", simpleParamsType));

    return (bufferObject);
}

As you can see, there are two if branches that are the same. Essentially they create a generic class using float and int type.

The class ArrayBufferObject is the base of ArrayBufferObject<T>, but only the latter implements an indexer (strongly typed). It is implemented as following:

public T this[uint index]
{
    get
    {
        if ((MemoryBuffer == null) || (MemoryBuffer.AlignedBuffer == IntPtr.Zero))
            throw new InvalidOperationException("not defined");
        if (index >= ItemCount)
            throw new ArgumentException("index out of bounds", "index");

        return ((T) Marshal.PtrToStructure(new IntPtr(MemoryBuffer.AlignedBuffer.ToInt64() + (index * mItemSize)), typeof(T)));
    }
    set
    {
        if ((MemoryBuffer == null) || (MemoryBuffer.AlignedBuffer == IntPtr.Zero))
            throw new InvalidOperationException("not defined");
        if (index >= ItemCount)
            throw new ArgumentException("index out of bounds", "index");

        Marshal.StructureToPtr(value, new IntPtr(MemoryBuffer.AlignedBuffer.ToInt64() + (index * mItemSize)), false);
    }
}

The source of the assigned item comes from a ColladaArray class, that is an abstract class wrapping an array of values. To resolve the data accessor I've defined in it a generic numeric accessor like the following:

/// <summary>
/// Access this array.
/// </summary>
/// <param name="i">
/// The index of the element to be accessed.
/// </param>
/// <returns>
/// It returns an equivalent <see cref="System.Double"/> of the value accessed.
/// </returns>
public virtual double this[uint i] { get { throw new NotSupportedException("array accessor not implemented"); } }

Which is implemented in each derived class returning the real array value casted to double (all arrays contains numeric data... hopefully).

How would you suggest to remove the GetAccessorArray routine redundancies? Help! Soon it will grow up with all possible numeric types!

Presumably you could pull those sections out into a generic method.

private ArrayBufferObject CreateArrayBuffer<T>(ColladaArray colladaArray)
{
     uint cursorOffset = Offset;

     ArrayBufferObject<T> arrayBufferObject = new ArrayBufferObject<T>(BufferObject.Hint.StaticCpuDraw);

     for (uint i = 0; i < Count; i++)
     {
         foreach (ColladaParam param in Params)
         {
             if (param.Name != null)
                 arrayBufferObject[cursorOffset - Offset] = (T)Convert.ChangeType(colladaArray[cursorOffset], typeof(T));
             cursorOffset++;
         }
         cursorOffset += Stride - (uint)Params.Count;
     }

     return arrayBufferObject;
}

And then replace those sections with a call to that method.

if (simpleParamsType == ArrayTypeFloat)
    return CreateArrayBuffer<float>(colladaArray);
else if (simpleParamsType == ArrayTypeInt)
    return CreateArrayBuffer<int>(colladaArray);
else
    throw new NotSupportedException(String.Format("simple accessor params type {0} not supported", simpleParamsType));

Depending on the types that you're converting to and from, the call to Convert.ChangeType() may have to be changed to something else, but it should be sufficient for the standard numeric types. For more information on type conversion, see this.


Groupby, apply custom function to data, return results in new columns I want to group by id, apply a custom function to the data, and create a new column with the results. It seems there must be a faster/more efficient way to do this than to pass the data to the function, make the changes, and return the data. Here is an example.

Example

dat = pd.DataFrame({'id': ['a', 'a', 'a', 'b', 'b', 'b'], 'x': [4, 8, 12, 25, 30, 50]})

def my_func(data):
    data['diff'] = (data['x'] - data['x'].shift(1, fill_value=data['x'].iat[0]))
    return data

dat.groupby('id').apply(my_func)

Output

id  x   diff
0   a   4   0
1   a   8   4
2   a   12  4
3   b   25  0
4   b   30  5
5   b   50  20

Is there a more efficient way to do this?

I tried a few variations on your code. I was surprised at how performant the groupby approach really was!

I changed your test data to use more values -- this amortizes the overhead a bit more. Surprisingly, the overhead is a lot of the difference. When I pushed the array length too high, the differences got very small for the groupby-based alternatives.

That said, there are some things you can do to speed things up:

original:        18.022180362
org_prefill:     14.969489811999996
unique_keys:     23.526526202000007
groupby_return:  15.557421341999998
groupby_prefill: 15.846651952999991
shifty:           9.605120361000004

I tried moving away from groupby by iterating over the distinct key values, but that didn't pay off: the performance got worse (unique_keys). I tried playing games with the return value from groupby, hoping to eliminate some duplicated effort. I eventually got that in groupby_return. For small sizes, where the overhead is more of a factor, I got a tiny speed boost by pre-filling the result column before running the groupby. That's groupby_prefill and then org_prefill where I back-ported it. You can see that it pays off versus the original code, but not against the groupby_return code.

Finally, I eliminated the groupby entirely, by figuring out how to detect the start of a group using .shift(). Then I computed a shifted-by-one series and did the subtract operation as a single expression. That's shifty and it's the most performant by a bunch. W00t!

import sys
import timeit

import numpy as np
import pandas as pd

def make_df():
    n = 10_000
    df = pd.DataFrame({'id': ['a']*(n//2) + ['b']*(n//2),
                    'x': np.random.randn(n)})
    return df

def original(df):
    def my_func(group):
        group['diff'] = (group['x'] - group['x'].shift(
            1, fill_value=group['x'].iat[0]))
        return group

    df.groupby('id').apply(my_func)

def org_prefill(df):
    def my_func(group):
        group['diff'] = (group['x'] - group['x'].shift(
            1, fill_value=group['x'].iat[0]))
        return group

    df['diff'] = df['x']
    df.groupby('id').apply(my_func)

def unique_keys(df):
    #print("DF:\n", df)
    df['diff'] = 0
    for key in df.id.unique():
        matches = (df.id == key)
        #df.loc[matches, 'diff'] = df.loc[matches, 'x'] - df.loc[matches, 'x'].shift(1, fill_value=df.loc[matches, 'x'].iat[0])
        df_lmx = df.loc[matches, 'x']
        df.loc[matches, 'diff'] = df_lmx - df_lmx.shift(1, fill_value=df_lmx.iat[0])

def groupby_iter(df):
    for key, subset in df.groupby('id'):
        subset['diff'] = subset.x - subset.x.shift(1,
                fill_value=subset.x.iat[0])

def groupby_return(df):
    def my_func2(group):
        gx = group['x']
        result = gx - gx.shift(1, fill_value=gx.iat[0])
        return result

    res = df.groupby('id').apply(my_func2)
    df['diff'] = res.values

def groupby_prefill(df):
    def my_func2(group):
        gx = group['x']
        result = gx - gx.shift(1, fill_value=gx.iat[0])
        return result

    df['diff'] = df['x']
    res = df.groupby('id').apply(my_func2)
    df['diff'] = res.values

def shifty(df):
    shifted = df['x'].shift(fill_value=df['x'].iat[0])
    shifted.loc[(df.id != df.id.shift())] = df['x']
    df['diff'] = df['x'] - shifted

if __name__ == '__main__':
    kwargs = {
        'globals': globals(),
        'number': 1000,
        'setup': 'df = make_df()',
    }

    print("original:", timeit.timeit('original(df)', **kwargs))
    print("org_prefill:", timeit.timeit('org_prefill(df)', **kwargs))

    print("unique_keys:", timeit.timeit('unique_keys(df)', **kwargs))
    #print("groupby_iter:", timeit.timeit('groupby_iter(df)', **kwargs))
    print("groupby_return:", timeit.timeit('groupby_return(df)', **kwargs))
    print("groupby_prefill:", timeit.timeit('groupby_prefill(df)', **kwargs))
    print("shifty:", timeit.timeit('shifty(df)', **kwargs))


Assigning book ids to certain students on registration of a new student I have a function where I am trying to assign book ids to certain students on registration of a new student.

FOR book_id IN
   SELECT a.bookid
   FROM  books a
   WHERE  a.isdelete = false and published = true

LOOP
IF NOT EXISTS (select 1 from studentbooks where  bookid = book_id and  studentid = id) THEN
     INSERT INTO studentbooks(bookid, studentid) values (book_id::uuid, id );
END IF;

END LOOP;

I wrote something like this and it works fine for smaller set of data and under performs when we have more books to loop through. I just wanted a have a review here.

Instead of extracting all books and execute a separate sub-query on each FOR loop iteration - we can select only the books that don't have matches in studentbooks table for specified student (presented by id argument) by running PostgreSQL LEFT JOIN with specific WHERE clause.

Furthermore, to make it one-shot action - we can insert data from query using INSERT INTO SELECT statement.

The query for the whole task:

INSERT INTO
   studentbooks(bookid, studentid)
   SELECT
      b.bookid, your_function_name.id
   FROM
      books b
      LEFT JOIN
         studentbooks sb
         ON sb.bookid = b.bookid AND studentid = id
   WHERE
      b.isdelete = false AND b.published = true
      AND sb.bookid is NULL;

replace your_function_name with your actual function name to refer the function's argument name.


Updating a JavaFX ListView after service update I wrote a JavaFX program that contains a list of notes, each having a title and a content.

This is the controller code:

public class MainPresenter implements Initializable
{
    @FXML
    private ListView<Note> listView;

    @FXML
    private TextField titleField;
    @FXML
    public TextArea contentField;
    @FXML
    public Button saveButton;

    @Inject
    private NoteService noteService;

    @Override
    public void initialize( URL location, ResourceBundle resources )
    {
        listView.setCellFactory( param -> new NoteListCell() );
        listView.setItems( FXCollections.observableArrayList( noteService.getNotes() ) );
        listView.getSelectionModel().selectedItemProperty().addListener( new NoteListViewSelectionChangeListener() );

        selectFirstItemIfPossible();

        saveButton.setOnAction( event -> {

            // Save the updated note with the service
            Note note = new Note( listView.getSelectionModel().getSelectedItem().getId(),
                                  titleField.getText(),
                                  contentField.getText() );
            noteService.updateNode( note );

            // Update the view
            int selectedIndex = listView.getSelectionModel().getSelectedIndex();
            listView.setItems( FXCollections.observableArrayList( noteService.getNotes() ) );
            listView.getSelectionModel().select( selectedIndex );

        } );
    }

    private void selectFirstItemIfPossible()
    {
        if (listView.getItems().size() > 0)
        {
            listView.getSelectionModel().select( 0 );
        }
    }

    private static class NoteListCell extends ListCell<Note>
    {
        @Override
        protected void updateItem( Note item, boolean empty )
        {
            super.updateItem( item, empty );
            if (item != null)
            {
                setText( item.getTitle() );
            }
        }
    }

    private class NoteListViewSelectionChangeListener implements ChangeListener<Note>
    {
        @Override
        public void changed( ObservableValue<? extends Note> observable, Note oldValue, Note newValue )
        {
            if( newValue != null )
            {
                titleField.setText( newValue.getTitle() );
                contentField.setText( newValue.getContent() );
            }
        }
    }
}

I am mainly looking for feedback on the code that gets run when you click on the save button:

saveButton.setOnAction( event -> {

            // Save the updated note with the service
            Note note = new Note( listView.getSelectionModel().getSelectedItem().getId(),
                                  titleField.getText(),
                                  contentField.getText() );
            noteService.updateNode( note );

            // Update the view
            int selectedIndex = listView.getSelectionModel().getSelectedIndex();
            listView.setItems( FXCollections.observableArrayList( noteService.getNotes() ) );
            listView.getSelectionModel().select( selectedIndex );

        } );

I now need to re-fetch all notes from the noteService again to be able to update the list view. I guess there is a better way to do it?

Found a better way myself:

saveButton.setOnAction( event -> {
            // Save the updated note with the service
            Note selectedItem = listView.getSelectionModel().getSelectedItem();
            selectedItem.setTitle( titleField.getText() );
            selectedItem.setContent( contentField.getText() );
            noteService.updateNode( selectedItem );

            // First update the item in the model
            listView.getItems().set( listView.getSelectionModel().getSelectedIndex(), selectedItem );
            // Trigger a sort of the items. Could be avoided by checking if the title really did change
            listView.getItems().sort( new NoteComparator() );

This avoids getting the full lists of notes from the service again each time.


Displaying the total and average for an array input I'm currently taking my first class for c++ and our current lab is asking us to:

declare an array with a maximum of 100 integer
display total of all the scores
display the average of scores entered
have a negative as a sentinel since the scores all will be positive (the sentinel is -1)

afterwards We're then supposed to display the entered scores in descending order.

What I currently have is this:

#include <iomanip>
#include <iostream>
using namespace std;

void header();

int main()
{

    const int number = 100; // total number of allocated array inputs.
    int myArray[number], //array set
        counter = 0; // current array input number (1~100)
    double total = 0, // total for all scores added up
        average; // the average point for the scores.

    header(); // for calling title and description.

    cout << "Please enter the test scores you recieved." << endl;
    cout << "Once all scores are in, enter -1 to end input" << endl << endl;

    while (cin >> myArray[counter] && myArray[counter] != -1)
    {
         total += myArray[counter]; // adds all inputs for grand total.
         counter++;
        average = total / counter; // grand total / number of input
    }

    cout << "The total for all the scores is " << total << endl;
    cout << endl << "Average score is " << fixed << average << endl;

    cout << endl << "Program ending, shutting down." << endl;
    return 0;
}

void header() // simple header for lab description.
{
    cout << "***********************************" << endl;
    cout << "Lab 7:score array " << endl;
    cout << "Programmed by: ~~~ " << endl;
    cout << "This array will allow the user to enter up to 100 test scores," << endl;
    cout << "Show the total for the the inputted scores, and display the average score rounded." << endl;
    cout << "***********************************" << endl << endl;
}

It works for the most part, but think that the while loop is probably wrong.

Am I on the right track or is this a mess?

Avoid Using Namespace std

If you are coding professionally you probably should get out of the habit of using the using namespace std; statement. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The function cout you may override within your own classes. This stack overflow question discusses this in more detail.

Variable Naming

Use descriptive variable names so the code is more meaningful and comments are not as necessary, some examples

Current name: Suggested name
number      : maxTestScores
myArray     : testScores

Not currently in code
endTestScoresList : -1 (make -1 a symbolic constant like number is).

Variable Declarations

A good programming habit to get into is to put each variable declaration in a separate statement on a separate line. Instead of

    int myArray[number], //array set
            counter = 0; // current array input number (1~100)
    double total = 0, // total for all scores added up
            average; // the average point for the scores.

it would be better the following way because it is easier to add or delete a variable and it is easier to read and maintain the code.

    int myArray[number];
    int counter = 0;
    double total = 0;
    double average = 0;

In C++ it is also a good habit to initialize all the variables.

Move Loop Invariants out of the Loop

In the following code the variable average is a loop invariant and should not be calculated within the loop. Optimizing compilers may do this for you, but it is better to remove things that don't belong in the loop.

    while (std::cin >> myArray[counter] && myArray[counter] != -1)
    {
        total += myArray[counter]; // adds all inputs for grand total.
        counter++;
        average = total / counter; // grand total / number of input
    }

would be better as

    while (std::cin >> myArray[counter] && myArray[counter] != -1)
    {
        total += myArray[counter]; // adds all inputs for grand total.
        counter++;
    }

    average = total / counter; // grand total / number of input

Possible Bug

As was observed in another answer, if the user entered more than 100 test scores the previous code could throw an index out of range error and the program would crash. Rather than user a for loop as shown in the other answer, the code could be modified to test the index in the while loop:

    while ((counter < number) && std::cin >> myArray[counter] && myArray[counter] != -1)
    {
        total += myArray[counter++]; // adds all inputs for grand total.
    }

    average = total / counter; // grand total / number of input

By putting the test (counter < number) first the std::cin will not be called and the loop will exit, preventing the out of range problem.

Complexity

The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

  that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

There are at least 3 possible additional functions that can be called from main().
  - Get the user input
  - Calculate and print the total and average
  - List the scores in decsending order

By reducing the complexity the code becomes easier to read, write, modify and debug.

Prefer '\n' Over std::endl.

As mentioned in another review std::endl calls a system function to flush the output buffer. Sometimes this is needed, but generally to improve performance std::endl isn't called. It might be called after a loop of std::cout has been completed.


Listing ordered components, recursively I am trying to get ordered components of an interface properly without having a local variable orderedComponents outside, and here is my current code.

Reason is because orderedComponents will be in Interface.java and getChildren will be in componentDefinitions.java and I want a more controllable code.

I was also thinking of changing from List to single dimension array and ill soon remove the static ones I separate them.

public static List<Integer> orderedComponents = new ArrayList<Integer>();

/**
 * returns the components ordered
 *
 * @param interfaceId
 * @return The components of an interface sorted
 */
public static List<Integer> getOrderedComponents() {

    orderedComponents.clear();

    for (int i = 0; i < inter.getComponents().length; i++) {

        // if all comps are added, stop the loop
        if (orderedComponents.size() == inter.getComponents().length)
            break;

        if (orderedComponents.isEmpty() && inter.getComponent(i).hasParent()
                && !orderedComponents.contains(inter.getComponent(i).getParentId()))
            continue;

        // do the magic
        getAllChildren(i);

    }
    return orderedComponents;
}

public static void getAllChildren(int componentId) {

    // get the current component definitions
    ComponentDefinitions component = inter.getComponent(componentId);

    // TODO first parent of the parents
    // if component has parent and it is not added to the list, add it
    if (component.hasParent() && !orderedComponents.contains(component.getParentId())) {
        orderedComponents.add(component.getParentId());
    }

    // if component is not added to the list, add it
    if (!orderedComponents.contains(component.componentId)) {
        orderedComponents.add(component.componentId);
    }

    // loop through all childs of the current components and re-add their childs
    for (int j = 0; j < inter.getComponent(componentId).getChilds().size(); j++) {
        ComponentDefinitions child = inter.getComponent(componentId).getChilds().get(j);
        getAllChildren(child.componentId);
    }
}

Please let me know what improvements could be done here and what I am doing wrong or is advisable to be changed.

If I understood correctly, you have to traverse a list of components where each element has a hierarchy-like structure?

Try the visitor pattern https://www.tutorialspoint.com/design_pattern/visitor_pattern.htm it is a very clean solution of what you're trying to do without any static methods.

And for traversing the children of each component, I think you're trying to do a pseudo-BFS, but it's a bit confusing. BFS is very tricky using recursion. The iterative version kf BFS should be something similar to this https://stackoverflow.com/questions/16380026/implementing-bfs-in-java


Compressing a 'char' array using bit packing I have a large array (around 1 MB) of type unsigned char (i.e. uint8_t). I know that the bytes in it can have only one of 5 values (i.e. 0, 1, 2, 3, 4). Moreover, we do not need to preserve '3's from the input, they can be safely lost when we encode/decode.

So I guessed bit packing would be the simplest way to compress it, so every byte can be converted to 2 bits (00, 01..., 11).

As mentioned, all elements of value 3 can be removed (i.e. saved as 0), which gives me option to save '4' as '3'. While reconstructing (decompressing) I restore 3's to 4's.

I wrote a small function for the compression but I feel this has too many operations and is just not efficient enough. Any suggestions or hints on how to handle the operations more efficiently but maintaining the readability will be of much help.

/// Compress by packing ...
void compressByPacking (uint8_t* out, uint8_t* in, uint32_t length)
{
  for (int loop = 0; loop < length/4; loop ++, in += 4, out++)
  {
    uint8_t temp[4];

    for (int small_loop = 0; small_loop < 4; small_loop++)
    {
      temp[small_loop] = *in;           // Load into local variable

      if (temp[small_loop] == 3)        // 3's are discarded
        temp[small_loop] = 0;
      else if (temp[small_loop] == 4)   // and 4's are converted to 3
        temp[small_loop] = 3;

    } // end small loop

    // Pack the bits into write pointer
    *out = (uint8_t)((temp[0] & 0x03) << 6) |
                    ((temp[1] & 0x03) << 4) |
                    ((temp[2] & 0x03) << 2) |
                    ((temp[3] & 0x03));

  } // end loop
}

Cross-posted from SO

Conditional jumps are murder on throughput due to branch-misprediction. Consider simply using a lookup-table instead:
const static uint8_t map[] = { 0, 1, 2, 0, 3 };
*out = (uint8_t)
    ( (map[in[0]] << 0)
    | (map[in[1]] << 2)
    | (map[in[2]] << 4)
    | (map[in[3]] << 6));

There are many architectures where shifting is more expensive than masking, but I doubt there are any where the reverse holds. Thus, the following code, even though it probably requires one extra machine code instruction (3 shifts vs. 4 masks) is likely to be faster:
const static uint8_t map[] = { 0x00, 0x55, 0xaa, 0x00, 0xff };
*out = (uint8_t)
    ( (map[in[0]] & 0x03)
    | (map[in[1]] & 0x0c)
    | (map[in[2]] & 0x30)
    | (map[in[3]] & 0xc0));

By the way, how do you handle the tail of up to 3 bytes? Or is your data guaranteed to be a multiple of 4 bytes long?
Also, your inner loop currently duplicates the first byte of every 4-byte-chunk four times.
Your comments are blatantly superfluous; at best they repeat the code. Say why you do something, not what you just did.


Given an unordered list of numbers, find all pairs that add up to x I am coding an interview question from Yahoo Software Engineer Intern Interview.

  Given an unordered list of numbers, find all pairs that add up to x.

I have a working solution in Java:

    public static void main (String args[]){
        int[] numbers = new int[]{2, 5, 7, 3, 9, 4, 1, 8}; //input array
        int x = 6; //target sum

        int[] result = new int[numbers.length]; //stores the output pairs
        int count = 0;

        int[] sorted = numbers;
        Arrays.sort(sorted);
        int beg = 0;
        int end = numbers.length-1;
        while (beg < end){
            int sum = sorted[beg] + sorted[end];
            if (sum > x)
                end--;
            else if (sum == x){
                result[count++] = sorted[beg];
                result[count++] = sorted[end];
                end--; beg++;
            }
            else if (sum < x)
                beg++;
        }
    }

Looking for code review optimizations and best practices.

What is the time complexity of this solution? They will definitely ask that.

The description doesn't specify the ordering of the pairs. It would be good to ask to clarify. If the pairs can be unordered, and the numbers are unique, a more efficient algorithm exists. (Hint: would it help to use a hash set? What will be the time complexity then? If the numbers may contain duplicates, then a hash map of counts could be used, as @mleyfman pointed out.)

This looks suspicious:

int[] sorted = numbers;
Arrays.sort(sorted);

The variable sorted is redundant. You simply gave numbers another name. Which is fine, as long as you keep in mind that the sort operation destroys the original ordering of the input.

Speaking of input... You implemented the solution in a main method, using hardcoded values. It would be better to write a proper function that takes the array and target as parameters, and (ideally) return something. That way the solution becomes unit-testable.

In this if-else chain the last else if can be a simple else:

if (sum > x)
    // ...
else if (sum == x){
    // ...
}
else if (sum < x)
    // ...

And it's recommended to use braces with even single-line statements.

With a better name for x (like target), you wouldn't need the comment here:

    int x = 6; //target sum


Naive SCP module in Elixir I'm trying to do a simple implementation of a scp wrapper in Elixir.

The code is working as intended, but I got a feeling that the error conditions can be simplified. Besides error handling, I don't the best way to append the -r flag if the source_path is a directory with a cond keyword.

defmodule Scp do
  def scp(user, ip, source_path, destination_path, timeout \\ 30) do
    scp_flags = [
      "-oStrictHostKeyChecking=no",
      "-oConnectTimeout=#{timeout}",
      "-oPasswordAuthentication=no",
      source_path,
      "#{user}@#{ip}:#{destination_path}"
    ]

    scp_flags =
      case File.dir?(source_path) do
        true -> ["-r" | scp_flags]
        _ -> scp_flags
      end

    if File.exists?(source_path) do
      case System.cmd("scp", scp_flags, stderr_to_stdout: true) do
        {resp, 0} ->
          {:ok, resp, 0}

        {resp, exit_code} ->
          cond do
            String.contains?(resp, "Permission denied") ->
              {:error, :permdenied, exit_code}

            String.contains?(resp, "Connection timed out") ->
              {:error, :conntimeout, exit_code}

            true ->
              {:error, :errunkown, exit_code}
          end
      end
    else
      {:error, :enoent, -1}
    end
  end
end

I have never used Elixir in my life until seeing this question, so my feedback is very limited and I probably shouldn't be putting an answer on here. However, there are a few ways it could be simplified:

the module lstat(path, opts \\ []) reads information about a path, returns a FileStat struct which has a type: field which can be type: :device | :directory | :regular | :other | :symlink; this could be used to determine what the path is (and whether to use -r), and if there is an error it means that the path does not exist (thus adding in the error handling)
in the case {resp, exit_code} -> instead of checking the error message contents via String.contains?, do the switching based on exit code and then return the appropriate error messages.
in the case {resp, exit_code} ->, I'd add a few extra error-handlers for statuses (for example directory not found on destination, invalid credentials, etc.)
for the inputs (user and ip address), a regex could be used to ensure sanity (the ip address looks like an ip, the username is not blank, etc.)
the case true -> {:error, :errunkown, exit_code} could be replaced with _ -> {:error, :errunkown, exit_code} as _ appears to be a fall through (I'm unsure about this)


Deleting most recent files by parsing filename I have hundreds of .mp3 files in a single directory of the same naming format, title_YYYY-MM-DD.mp3, with maybe 30 different titles.
Here is an example two different titles:

vision_am_2015-08-04.mp3
vision_am_2015-08-03.mp3
vision_am_2015-07-31.mp3
vision_am_2015-07-30.mp3

lum_pro_2015-08-04.mp3
lum_pro_2015-08-03.mp3
lum_pro_2015-08-01.mp3
lum_pro_2015-07-31.mp3
lum_pro_2015-07-30.mp3
lum_pro_2015-07-29.mp3
lum_pro_2015-07-28.mp3
lum_pro_2015-07-27.mp3

I need to keep X number of most recent files for each title. I figured that since the date format is YYYY-MM-DD, after building a data structure for the files, I can make sure the files are sorted in descending order. Then iterate through them. Then safely delete with confidence each file after the Xth iteration.

Here is my idea:

my $num_to_keep = 2; # or get from @ARGV
$num_to_keep = $num_to_keep - 1;

my $dir = "/home/mp3files";
opendir my $DH, "$dir" or die "$! not open";
my $dateRE = qr/\d{4}-\d{2}-\d{2}/;
my $fileRE = qr/^.+_$dateRE\.mp3$/; # only mp3s
my @files = sort grep {/$fileRE/ && -f "$dir/$_"} readdir($DH);
close $DH;

my %hash = ();
for my $file (reverse @files) {
    my ($fname) = $file =~ m/(.*)?_$dateRE/;
    push(@{ $hash{$fname} }, $file);
}

for my $fname (sort keys %hash) {
    my @files = @{$hash{$fname}};
    print "\n\n\nFILE: $fname<<\n";
    for my $i (0..$#files) {
        if ($i > $num_to_keep) {
            unlink "$dir/$files[$i]";
        }else{
            print "\t\t\t\tI will keep this file $files[$i]\n";
        }
    }
}

This is working as I expected, but since I am using this to delete large numbers of files regularly, I would like an expert take on this. I do not want to accidentally delete wrong files. Plus, I am interested in any general improvements or more elegant solutions.

You can reduce number of loops, sorts, and matches, so this should perform faster,

my $num_to_keep = 2;

my $dir = "/home/mp3files";
opendir my $DH, $dir or die "$! $dir";

# only mp3s
my $fileRE = qr/(.+) _ (\d{4}-\d{2}-\d{2}) \.mp3$/x;

my %count;
# files to delete
my @files = map {
  my $basename = $_->[1];
  (++$count{$basename} > $num_to_keep) ? $_->[0] : ();
}
sort {
  $b->[2] cmp $a->[2] # sort descending by date
}
map {
  my @match = /$fileRE/;
  (@match && -f "$dir/$_") ? ["$dir/$_", @match] : ();
}
readdir($DH);

close $DH;
unlink(@files);


Hash function for a key of type structure I need to implement a hash function for a key of type structure. This structure is as follows:

typedef struct hash_key_s {
    void *k1;
    char *k2;
    char *k3;
} hash_key_t;

My proposed hash function is as follows:

unsigned long hash_func (void *key)
{
    hash_key_t  *k = (hash_key_t *) key;
    unsigned long h = 0;
    char c = 0;
    char *p = k->k2;

    if (p != NULL) {
        for (c=0; c=*p; p++) {
            h = h*31 + c;
        }
    }
    p = k->k3;
    if (p != NULL) {
        for (c=0; c=*p; p++) {
            h = h*31 + c;
        }
    }

    h = h*31 + (((unsigned long) k->k1) >> 2);
    return h*110351524UL;
}

I just want to know if it is a good hash or if I can improve it further.

"Is this a good hash function?" is not a question that can be answered in isolation.  It depends greatly on the expected distribution of the inputs and on the desired distribution of its output.  Do you want each bit of the output to be equally likely to be 0 or 1, for example?

Here, I'm going to assume that you want the minimal requirements for a hash function - maximum likelihood of different outputs for different inputs, and no requirements for cryptographic-strength collision resistance.

It's interesting that you're combining the content pointed to by k2 and k3 but that value of the pointer k1 - a comment is needed to explain why this is what you want.

I assume that you are constrained to the signature unsigned long (*)(void*) by some library.  A pointer to const void would be better as the argument, as your hash function really mustn't modify its argument.  Failing that, you can declare k as pointer-to-const:

    hash_key_t const *const k = key;

(It's C, so void* will promote to any other pointer).

The repeated code to hash the two strings can be extracted into a function:

static unsigned long hash_string(const char *s, unsigned long h)
{
    if (!s) return h;
    while (*s)
        h = h * 31 + *s++;
    return h;
}

unsigned long hash_func (void *key)
{
    const hash_key_t *const k = key;
    unsigned long h = 0;

    h = hash_string(k->k2, h);
    h = hash_string(k->k3, h);

    h = h*31 + (unsigned long)k->k1;
    return h*110351524UL;
}

I removed the right-shift of k1 in the above - that doesn't add any extra entropy to the hash, and may (on some platforms) reduce it.  But I have left your magic constant untouched - see next section.

An aspect of your algorithm that's a little suspect is the final constant. It is an even number - worse, it's a multiple of 4; this is throwing away two bits from your result.  I'm not convinced there's a good reason to multiply the final result - you'll need to justify it given the expected inputs and desired distribution of result.

Because you effectively concatenate the two strings, you'll get collisions between "ab", "c" and "a", "bc".  That may or may not be a problem for your inputs, but it's worth noting in a comment in case you want to repurpose this in future.  If it's undesirable, you could either perform another multiplication between the strings or you could provide a different per-character constant to each invocation.

Rather than reinventing the wheel, you may be able to re-use (or at least copy) an existing algorithm.  For example, if your code is GNU GPL compatible, here's the glibc implementation:

/* We assume to have `unsigned long int' value with at least 32 bits.  */
#define HASHWORDBITS 32

/* Defines the so called `hashpjw' function by P.J. Weinberger
   [see Aho/Sethi/Ullman, COMPILERS: Principles, Techniques and Tools,
   1986, 1987 Bell Telephone Laboratories, Inc.]  */
static inline
unsigned long int
hash_string(const char *str_param)
{
    unsigned long int hval, g;
    const char *str = str_param;

    /* Compute the hash value for the given string.  */
    hval = 0;
    while (*str != '\0') {
        hval <<= 4;
        hval += (unsigned long int) *str++;
        g = hval & ((unsigned long int) 0xf << (HASHWORDBITS - 4));
        if (g != 0) {
            hval ^= g >> (HASHWORDBITS - 8);
            hval ^= g;
        }
    }
    return hval;
}


Monitor progress of written lines of code Not long ago, I was asked about how many lines of code I have written in C++ and could not answer that question. For this reason and to become more familiar with Bash shell scripting, I wrote a small system to monitor the lines of code written in a specific subdirectory.

Currently, I count the lines of code at 5 seconds interval and write the difference (if favorable) to a file.

As mention above, I am not familiar with bash scripting and hope to get some helpful feedback.

I would appreciate the advice, especially in the following areas:

Code style (readability, naming conventions)
Efficiency (how to avoid unnecessary complexity)
How can I prevent a long list of extension as used currently in my
approach (-name "*.py" -o -name "*.cpp" -o -name "*.h"). Can I do
something like [py, cpp, h, ...] instead?

count_lines () {
    N="$(find ./ -name "*.py" -o -name "*.cpp" -o -name "*.h" | \
    xargs grep -cve "^\s*$" | grep -oP '(?<=:).*' | \
    awk '{s+=$1} END {print s}')"
    echo "$N"
}

FILE_NAME="counter.log"
touch $FILE_NAME
SUM=0

while true; do
    NUM1=$(count_lines)
    sleep 5;
    NUM2=$(count_lines)
    DIFF=$((NUM2-NUM1))
    if [ "$DIFF" -gt 0 ]; then
        SUM=$((SUM+DIFF))
    fi
    echo "${SUM} ${NUM2}" >> $FILE_NAME
done

This isn't any shorter, but it is more dynamic:

exts=( py cpp h )

count_lines() {
    local find_args=( -false )
    for ext in "${exts[@]}"; do
        find_args+=( -o -name "*.$ext" )
    done

    find ./ "${find_args[@]}" -print0 |
    xargs -o grep -cve "^\s*$" |
    grep -oP '(?<=:).*' |
    awk '{s+=$1} END {print s}'
}

The extensions reside in a bash array that can live in the global scope. It's one place to store your list of interesting files.

In count_lines you can build up an array of arguments to pass on to find. I'm using -false as the first one so that it's simple to add -o -name "*.something in the loop.

You don't need to capture the command output only to print it on the next line. Just let the commands print to stdout directly.

I added -print0 to find and -0 to xargs. These options work well together to be able to handle any filename, regardless of any whitespace in the name.

Note you don't need to use line continuations when the line ends with a pipe (or with a && or ||) -- bash understands that the pipeline continues on the next line.

About your awk command: if your goal is to count lines, don't you want {s+=1} or {s++} instead of {s+=$1}? The latter will only sum any filenames that look like numbers:

$ touch 5 8 a b c
$ ls | awk '{sum += $1; count++} END {print sum, count}'
13 5


function to generate values of a javascript object The following code works but I am wondering if it can be improved both readability and performance.
I am using jquery and underscore.
Any suggestions, comments or feedbacks will be appreciate.
thanks.

var getThemeName = function (theme_name, transparency) {
    if (theme_name === 'light') {
        if (transparency) {
            return translator.get('messages:theme.info.light_with_transparency', {
                percentage: transparency
            });
        }
        return translator.get('messages:theme.info.light');
    } else if (theme_name === 'dark') {
        if (transparency) {
            return translator.get('messages:theme.info.dark_with_transparency', {
                percentage: transparency
            });
        }
        return translator.get('messages:theme.info.dark');
    }
};

var THEME_ITEMS = {
    themeLight: getThemeName('light'),
    themeLightTransparency75: getThemeName('light', 75),
    themeLightTransparency50: getThemeName('light', 50),
    themeLightTransparency25: getThemeName('light', 25),
    themeDark: getThemeName('dark'),
    themeDarkTransparency75: getThemeName('dark', 75),
    themeDarkTransparency50: getThemeName('dark', 50),
    themeDarkTransparency25: getThemeName('dark', 25)
};

If you keep the naming consistent, you could probably do something like:

function getThemeName(themeName, transparency) {
  var name = "messages:theme.info." + themeName;
  if( transparency ) {
    name += "_with_transparency";
    return translator.get(name, { percentage: transparency });
  } else {
    return translator.get(name);
  }
}


Finding the mode of an array I am a beginner in JavaScript and just wanted to get my code reviewed. Here I have created a function that takes in an array and returns the mode. If there is no more then I just want the function to return the first number. It seems a little too long and I feel like there may be more clever ways to do it. Again, keep in mind that I am a beginner, so I don't have a huge toolbox.

var mode = function(arr){
    var numMapping = {};
    for(var i = 0; i < arr.length; i++){
        if(numMapping[arr[i]] === undefined){
            numMapping[arr[i]] = 0;
        }
            numMapping[arr[i]] += 1;
    }
    var greatestFreq = 0;
    var mode;
    for(var prop in numMapping){
        if(numMapping[prop] > greatestFreq){
            greatestFreq = numMapping[prop];
            mode = prop;
        }
    }
    return parseInt(mode);
}

var mode = function(arr){

It's nice to name your functions for debugging purposes, if none other. You could replace this with a function declaration, e.g. function mode(arr) { or just name the function expression: var mode = function mode(arr) {

    var numMapping = {};
    for(var i = 0; i < arr.length; i++){
        if(numMapping[arr[i]] === undefined){
            numMapping[arr[i]] = 0;
        }
            numMapping[arr[i]] += 1;
    }

I would use arr.forEach(), since it lets you operate on the elements directly. You could also make the mapping a one-liner(but both of these things are more nitpicks/personal style choice)

    var greatestFreq = 0;
    var mode;
    for(var prop in numMapping){
        if(numMapping[prop] > greatestFreq){
            greatestFreq = numMapping[prop];
            mode = prop;
        }
    }
    return parseInt(mode);
}

You can do this whole part during your first loop! Also, parseInt, while safe in this case, is not generally safe to use without a radix(you'd have linters complaining about this usage here). You could use parseInt(mode, 10), or some ToNumber() transformation like +mode(http://es5.github.io/#x11.4.6).

Here's a revised example:

var mode = function mode(arr) {
    var numMapping = {};
    var greatestFreq = 0;
    var mode;
    arr.forEach(function findMode(number) {
        numMapping[number] = (numMapping[number] || 0) + 1;

        if (greatestFreq < numMapping[number]) {
            greatestFreq = numMapping[number];
            mode = number;
        }
    });
    return +mode;
}


Splitting and printing comma-separated values I stumbled upon a question on SO asking how to split a comma-separated string into individual values.

Since it's been a while since I've had any good reason to write C I'd like to ask for some feedback.

#include <stdio.h>
#include <string.h>

int main (int argc, const char * argv[]) {
    const char str[] ="coma separated,input,,,some fields,,empty";
    const char tok[] = ",";
    char * tmp = (char *)str;
    size_t count;
    for (count=0; tmp[count]; tmp[count] == tok[0] ? count++ : * tmp++) {
        //Empty loop body.
    }
    tmp = (char *)str;
    for (size_t i = 0, l = 0; i < count; i++) {
        l = strcspn (tmp, tok);
        if (l == 0) {
            printf("\"\"\n");
        } else {
            printf("\"%.*s\"\n", l, tmp);
        }
        tmp += sizeof(char) * (l + 1);
    }
    printf("\"%s\"\n", tmp);
    return 0;
}

Output as expected:

"coma separated"
"input"
""
""
"some fields"
""
"empty"

sizeof(char) by definition is 1. tmp += l + 1; would be as good.
I don't see a need to special case l == 0.
You don't have to initialize l in a loop header: it is immediately reassigned.
gcc complains about precision specifier in printf("\"%.*s\"\n", l, tmp) being size_t; unfortunately, cast is unavoidable.
I don't see a need for 2 separate loops. A single

char * tmp = (char *)str;
do {
    int l = strcspn (tmp, tok);
    printf("\"%.*s\"\n", l, tmp);
    tmp += l + 1;
} while (tmp[-1]);
return 0;

works equally well.


Class to simulate enums in PHP I have been reading this thread on Stack Overflow about simulating enums in PHP and it seems that the most common approach is to use class constants. My problem with that is I can't use it for type hints, so I have added a static function that checks if a value is defined as one of the constants.

class DataType
{
    const BIT = 'BIT';
    const TINYINT = 'TINYINT';
    const SMALLINT = 'SMALLINT';
    const MEDIUMINT = 'MEDIUMINT';
    const INT = 'INT';
    const INTEGER = 'INTEGER';
    const BIGINT = 'BIGINT';
    const FLOAT = 'FLOAT';
    const DECIMAL = 'DECIMAL';
    const NUMERIC = 'NUMERIC';
    const DATE = 'DATE';
    const TIME = 'TIME';
    const TIMESTAMP = 'TIMESTAMP';
    const DATETIME = 'DATETIME';
    const YEAR = 'YEAR';
    const CHAR = 'CHAR';
    const VARCHAR = 'VARCHAR';

    static public function Defines($const)
    {
        $cls = new ReflectionClass(__CLASS__);
        foreach($cls->getConstants() as $key=>$value)
        {
            if($value == $const)
            {
                return true;
            }
        }

        return false;
    }
}

Any thoughts on improving the code are welcome.

Why not just pass the data type you are looking for as a second argument to get_Type() then call the Defines() or similar method inside to verify it is the correct type? You can even provide a default data type should you use one more frequently.

function get_Type( $value, $DataType = VARCHAR ) {
    if( ! $this->Defines( $DataType ) ) { return FALSE; }

    //rest of method
}

Since you only want to test a given "DataType", I'd rewrite Defines() to use in_array() rather than loop through all the constants manually. Which BTW, you don't need to declare the $key=> bit in a foreach loop unless you are actually going to use the key.

function Defines( $const ) { return in_array( $const, $this->getConstants() ); }

I wrote this non-static, don't know if it needs to be static or not, but I tend to avoid static if at all possible.

I'd also like to point out that your method names conflict with PHP functions or aren't very descriptive. With the former you run the chance of your code conflicting with something else, or confusing people as to its purpose. With the latter you just confuse people. define() is a PHP function, so Defines() is close enough that it might cause issues or become confusing. get_Type() looks too much like you are fetching the data type rather than comparing it.

I hope this answer helps, if not then I'd add those last two comments to your actual question so more people might be able to help. That's what helped me to understand what you were trying to accomplish a lot better than the original explanation. If I actually understood it. Anyways, it seems much clearer now. Good Luck!


Extracting queries from controller to model I had a complex query in the controller like:

def outgoing_tasks
  @tasks = current_user.
           assigned_tasks.
           uncompleted.
           includes(executor: :profile).
           order("deadline DESC").
           paginate(page: params[:page], per_page: Task.pagination_per_page)
end

and since I have some other collections in that controller I refactored to this:

def outgoing_tasks
  @tasks = current_user.assigned_tasks.assigned_default(page: params[:page])
end

with these new methods in the model:

def self.ordered
  order("deadline DESC")
end

def self.paginated(page: 1)
  paginate(page: page, per_page: self.pagination_per_page)
end

def self.assigned_default(page: 1)
  uncompleted.includes(executor: :profile).ordered.paginated(page: page)
end

Should I do something differently or is this okay?

This is a good place to use scopes. http://edgeguides.rubyonrails.org/active_record_querying.html#scopes Scopes are just syntactic sugar for class methods that return relations. In this way they can always be chain-able.

scope :ordered -> { order(deadline: 'DESC') }
scope :paginated -> (page: 1) { paginate(page: page, per_page: pagination_per_page) }
scope :assigned_default -> (page: 1) { uncompleted.includes(executor: :profile).ordered.paginated(page: page) }