Finding average word length in a given year I have written a function which takes the year in question and words as a data which is a dictionary that maps words to the list of year/count. Now I am wondering how I can improve the code that I have or how to make it more simpler or make it better performance-wise.

def avgWordLen(year, words):
    totLen = 0
    totword = 0
    for word in words:
        for nary in words[word]:
            if nary.year == year:
                totLen += len(word) * nary.count
                totword += nary.count
    if totword != 0:
        return totLen / totword
    else:
        return 0

totword and totLen are not so good names.
And in any case PEP8 suggests to use snake_case for both variable and function names. So I recommend the following renames:

total_word_length instead of totLen
word_count instead of totword
average_word_length instead of avgWordLen

When you iterate over keys in a dictionary and then lookup the values in every iteration step,
then it's better to iterate over the dictionary items.
That is, instead of:

    for word in words:
        for nary in words[word]:

Do like this:

    for word, nary_list in words.items():
        for nary in nary_list:

This way you avoid unnecessary dictionary lookups.

At the end of the method, the else is unnecessary,
because the if part always returns.
It's slightly simpler this way:

    if word_count != 0:
        return total_word_length / word_count
    return 0


Generating business summary stats in SQL dynamically, based on an inputted date I have the following Oracle SQL query to generate some system-stats. I am interested in making it a more efficient and readable query.

SELECT
    SUM("Req Lines") as "Requisitions Created",
    SUM("Approved Req") AS "Requisitions Approved",
    SUM("Non Approved PO") AS "PO Started",
    SUM("Approved PO") AS "PO Approved",
    SUM("w/ Receipt") AS "Receipts Completed",
    SUM("w/ Invoice") AS "Invoices Completed"
FROM(
  SELECT DISTINCT prha.Segment1 AS "Req#",

    pha.Segment1 as "PO#",
    CASE when prha.creation_date between to_date(:myDate,'DD-MON-YY') and to_date(:myDate,'DD-MON-YY') + 7  then 'Week 1'
        when prha.creation_date between to_date(:myDate,'DD-MON-YY') + 7 and to_date(:myDate,'DD-MON-YY') + 14  then 'Week 2'
        when prha.creation_date between to_date(:myDate,'DD-MON-YY') + 14 and to_date(:myDate,'DD-MON-YY') + 21 then 'Week 3'
        when prha.creation_date between to_date(:myDate,'DD-MON-YY') + 21 and to_date(:myDate,'DD-MON-YY') + 28  then 'Week 4'
      else 'After Month 1' END as "Req_Created",     1  as "Req Lines",
      prha.authorization_status as "Req Status",

    CASE when prha.authorization_status='APPROVED' then 1 else 0 END as "Approved Req",
    CASE when pha.authorization_status='APPROVED' then 1 else 0 END as "Approved PO",
    pha.authorization_status,
    CASE when (pha.authorization_status = 'APPROVED' or pha.authorization_status is null) then 0 else 1 END as "Non Approved PO",
    CASE when pda.quantity_delivered >=1 then 1 else 0 END as "w/ Receipt",
    CASE when pda.quantity_billed >=1 then 1 else 0 END as "w/ Invoice"

  FROM apps.po_requisition_lines_all prl,
    apps.po_requisition_headers_all prha,
    po.po_req_distributions_all prda,
    po.po_distributions_all pda,
    po.po_headers_all pha
  where 1=1
  and prl.requisition_header_id= prha.requisition_header_id
  and prl.requisition_line_id= prda.requisition_line_id
  and prda.distribution_id= pda.req_distribution_id(+)
  and pha.po_header_id(+)=pda.po_header_id

  AND prda.creation_date > to_date(:myDate,'DD-MON-YY')
)

It is possible to simplify this:

CASE when prha.creation_date between to_date(:myDate,'DD-MON-YY') and to_date(:myDate,'DD-MON-YY') + 7  then 'Week 1'
    when prha.creation_date between to_date(:myDate,'DD-MON-YY') + 7 and to_date(:myDate,'DD-MON-YY') + 14  then 'Week 2'
    when prha.creation_date between to_date(:myDate,'DD-MON-YY') + 14 and to_date(:myDate,'DD-MON-YY') + 21 then 'Week 3'
    when prha.creation_date between to_date(:myDate,'DD-MON-YY') + 21 and to_date(:myDate,'DD-MON-YY') + 28  then 'Week 4'
  else 'After Month 1' END as "Req_Created",     1  as "Req Lines",

In Oracle SQL, you can extract a date from the other date. Result is a number, which equals to day count between dates (fractional part is hours, minutes and seconds). So, prha.creation_date - to_date(:myDate,'DD-MON-YY') gives number of days, ceil((prha.creation_date - to_date(:myDate,'DD-MON-YY' - 1) / 7) gives number of week. Here is the code:

case when prha.creation_date - to_date(:myDate,'DD-MON-YY') > 28
       then 'After Month 1'
     else 'Week ' || ceil((prha.creation_date - to_date(:myDate,'DD-MON-YY') - 1) / 7)
     end as "Req_Created"

The following expression:

CASE when prha.authorization_status='APPROVED' then 1 else 0 END as "Approved Req"

could be replaced with decode function:

decode(prha.authorization_status, 'APPROVED', 1, 0) as "Approved Req"

decode is more brief, but has its own limitations. For more details, see documentation.


Creating default object from empty value in PHP? Consider the following code.

$car = (object)[
   'general' => [
        'interior' => [
            'seats' => 'destroyed'
        ]
    ]
];

$exteriorProperties = [
    'hood' => 'shiny',
    'windows' => 'dirty'
];

foreach($exteriorProperties as $key => $prop){
    $car->{'other'}->{'exterior'}->{$key} = $prop;
}

This is a scenario where I'd like to add properties to an object, a few layers deep at a time, while iterating through some other data. If it doesn't exist, it will just create it, if it does it will override. PERFECT!

Unfortunately, this will throw the warning:

  Creating default object from empty value

I could resolve the issue by checking first if the property exists like so:

foreach($exteriorProperties as $key => $prop){
    if(!isset($car->other))
        $car->other = (object)[];
    if(!isset($car->other->exterior))
        $car->other->exterior = (object)[];
    $car->other->exterior->{$key} = $prop;
}

This is not my ideal solution. I'm wondering if there's a more elegant solution. Currently, I'm using this:

@$car->{'other'}->{'exterior'}->{$key} = $prop;

to suppress the warnings but I'm worried about compatibility in the future.

Does anyone have a more elegant solution to this scenario?

Yes, use arrays.

$car = [
   'general' => [
        'interior' => [
            'seats' => 'destroyed'
        ]
    ]
];

$exteriorProperties = [
    'hood' => 'shiny',
    'windows' => 'dirty'
];

foreach($exteriorProperties as $key => $prop){
    $car['other']['exterior'][$key] = $prop;
}
$car = (object) $car;

Anyway, the object conversion will only convert the outermost array. The nested arrays are still arrays.

\is_array($car->general); // true

Why you need it to be shapeless objects anyway? Either define classes for those objects or treat them as arrays is the simple answer.

Btw $car->{'other'} is really weird way of writing $car->other...


Review on core EF repository method What do you think about this core EF method ?

Also, Notice that it returns IQueryable:

Could that be a performance danger when extended improperly ? How ?

Please accept the fact that Includes and OrderBy parameters are string type.

public IQueryable<TEntity> GetEntitySetMultipleTables(Expression<Func<TEntity, bool>> WhereExpression = null,
                                                string OrderBy = null,
                                                int TotalResults = int.MaxValue, int SkippedResults = 0, string[] Includes = null)
    {
        if (OrderBy != null)
        {
            string orderbyProperty = OrderBy;
            if (OrderBy.IndexOf(' ') > -1) // Direction
            {
                orderbyProperty = OrderBy.Substring(0, OrderBy.IndexOf(' '));
            }

            if (typeof(TEntity).GetProperty(orderbyProperty) == null)
                OrderBy = null;
        }
        if ((SkippedResults > 0) && (OrderBy == null))
        {
            OrderBy = GetPrimaryKeyName();
        }

        //Base EntitySet
        IQueryable<TEntity> results;
        ObjectQuery<TEntity> objectSetResults = Context.CreateObjectSet<TEntity>();
        if (Includes == null)
            results = objectSetResults.AsQueryable();
        else
        {
            foreach (var include in Includes)
            {
                objectSetResults = objectSetResults.Include(include.Trim());
            }
            results = objectSetResults.AsQueryable();
        }

        //Filter by Where Expression
        if (WhereExpression != null)
            results = results.Where(WhereExpression);

        //Order Results
        if (OrderBy != null)
        {
            results = results.OrderBy(OrderBy);
        }

        //Skip Records
        if (SkippedResults > 0)
            results = results.Skip(SkippedResults);

        //Take Specific Amount of records
        if (TotalResults != int.MaxValue)
            results = results.Take(TotalResults);

        //Run the LINQ Query and return the results
        return results;
    }

Thanks in advance.

I'm not sure how this code works (I had to invent GetPrimaryKeyName) and I can't find a IQueryable<T> that has an OrderBy that takes a string parameter.  So faking it as much as possible, I came up with this:

    public IQueryable<TEntity> GetEntitySetMultipleTables(
        Expression<Func<TEntity, bool>> whereExpression = null,
        string orderBy = null,
        int? totalResults = null,
        int? skippedResults = null,
        string[] includes = null)
    {
        PropertyInfo orderbyProperty;
        int directionIndex;

        if (orderBy == null)
        {
            directionIndex = -1;
            orderbyProperty = null;
        }
        else
        {
            directionIndex = orderBy.IndexOf(' ');

            // Direction.
            if (directionIndex > -1)
            {
                orderBy = orderBy.Substring(0, directionIndex);
            }

            orderbyProperty = typeof(TEntity).GetProperty(orderBy);
        }

        if ((skippedResults != null) && ((int)skippedResults > 0) && (orderbyProperty == null))
        {
            orderbyProperty = typeof(TEntity).GetProperty(GetPrimaryKeyName());
        }

        // Base EntitySet.
        ObjectQuery<TEntity> objectSetResults = Context.CreateObjectSet<TEntity>();
        var results = includes == null
            ? objectSetResults.AsQueryable()
            : includes.Aggregate(objectSetResults, (current, include) => current.Include(include.Trim())).AsQueryable();

        // Filter by Where Expression.
        if (whereExpression != null)
        {
            results = results.Where(whereExpression);
        }

        // Order Results.
        if (orderbyProperty != null)
        {
            results = directionIndex > -1
                ? results.OrderByDescending(result => orderbyProperty.GetValue(result, null))
                : results.OrderBy(result => orderbyProperty.GetValue(result, null));
        }

        // Skip Records.
        if ((skippedResults != null) && ((int)skippedResults > 0))
        {
            results = results.Skip((int)skippedResults);
        }

        // Take Specific Amount of records.
        if ((totalResults != null) && (totalResults > 0))
        {
            results = results.Take((int)totalResults);
        }

        // Run the LINQ Query and return the results.
        return results;
    }


Inserting text file rows into an Access DB I am trying to speed up the process of inserting all of the rows in my text file into an Access DB. I originally switched to this route because of the size of those text files (that I am generating on another step of my process). I have more ROM than RAM. The overall process is working now, but I am falling into the rabbit hole of "optimizing" and can use someone else's opinion.

Limitations and specifications:

I can't load all of the text file into one variable. I can't get more RAM.
My logic needs to accept any text size (Column x Row).
All the data will be inserted to an existing empty Access table.

I will try to minimize the code posted here. Feel free to ask questions if anything important seems to be missing. Here is the logic in question:

Method that contains the code:

Public Sub InsertTextFileRowsIntoAccessDB2(sAccessTableName As String, sDBFilePath As String, CashePath As String)
'...all the code here
End Sub

Logic A: Best I could do

           'Insert each Row from the text file into the access DB

   Do While (Not reader.EndOfStream)
                Try
                    Dim fullRow() = reader.ReadLine.Split(CChar(vbTab))

                    'Create the parameters
                    Dim Prams As String = "@p1"
                    For i = 2 To fullRow.Count
                        Prams = Prams & " ,@p" & i
                    Next

                    Dim insertQuery As String = "INSERT INTO " & sAccessTableName & " VALUES (" & Prams & ");"
                    Dim cmd2 = New OleDbCommand(insertQuery, conn)

                    For index As Integer = 1 To fullRow.Length
                        Dim tempP As String = "@p" & index  'Array starts from zero
                        cmd2.Parameters.AddWithValue(tempP, fullRow(index - 1))
                    Next
                    cmd2.ExecuteNonQuery()

                Catch ex As Exception
                    Dim rex = ex 'TODO: Write the row and the exception somewhere
                End Try
            Loop

Logic 2

                'Insert each Row from the text file into the access DB
            Do While (Not reader.EndOfStream)
                Try
                    Dim fullRow() = reader.ReadLine.Split(CChar(vbTab))

                    'Build the query string to have all the columns
                    Dim query As String = "INSERT INTO " & sAccessTableName & " VALUES (" & "'" & fullRow(0) & "'"
                    For index As Integer = 1 To fullRow.Length - 1

                        If fullRow(index).Equals("") Then    'Check if the column is empty
                            query = query & ", NULL "
                        Else
                            'Check if the string has a quote in it (which causes issues to SQL)
                            query = query & ", '" & fullRow(index).Replace("'", "''") & "'"
                        End If
                    Next
                    query = query & ");"

                    Dim cmd As OleDbCommand = New OleDbCommand(query, conn)
                    cmd.ExecuteNonQuery()

                Catch ex As Exception
                    Dim rex = ex 'TODO: Write the row and the exception somewhere
                End Try
            Loop

How can I improve my inserts?

Questions asked in comments:

  What version of Access is it? Can you post your connection string?

"Provider=Microsoft.ACE.OLEDB.12.0;Data Source=" & sDBFilePath

Let me know if this could cause any issues with other machines or how to avoid incompatibility.

  Also, what does the table you are inserting into look like?

It depends on the data (table) I am getting. I am trying to get the column count to be dynamic so the method would work with any table. However, I am making the assumption that the column number in the table will be the same as the column number in the text file that I am reading from.

OK, thought I might get a quick win if you were using a version of Access old enough to be using the Jet engine (in that case DAO is inherently much faster).

First thing: Don't go down the road of your "Logic 2".  The fact that you realized you have to escape any quotes in the input file should be a great reminder as to why using parameters is good practice - it sanitizes input and avoids SQL injection.

Second, and this is the main one.  You are looping over code that should always execute with the same results.  Pull these out of you loop so you only do them once:

            'Create the parameters
            Dim Prams As String = "@p1"
            For i = 2 To fullRow.Count
                Prams = Prams & " ,@p" & i
            Next

            Dim insertQuery As String = "INSERT INTO " & sAccessTableName & " VALUES (" & Prams & ");"
            Dim cmd2 = New OleDbCommand(insertQuery, conn)

The dynamic parameter generation isn't ideal, but isn't really avoidable if you need a general purpose function.  I'm guessing that a large amount of the inefficiency is repeatedly creating new OleDbCommand objects (as well as the SQL string) when they can be re-used.  In fact, parameterized OleDbCommand objects are optimized for looping over with multiple inserts. Add your parameter list without values before you go into the main loop, and then assign to them by index.

This should speed you up quite a bit. If that doesn't get you into the ballpark of where you need to be performance-wise, the Access object model will let you automate a bulk insert from a tab delimited file.  You may want to benchmark how long it takes Access to import the file and explore this possibility too, especially if you control the datafile generation.

One last note:  Don't forget to "'TODO: Write the row and the exception somewhere" and keep swallowing the exceptions.

EDIT:

Good starting place for Access automation is here - there are some community links at the bottom of the page that are pretty good. I'd only use this as a last resort as it pulls in lots of dependencies and requires starting an Access process.  I'm fairly sure managing the OleDbCommand outside the loop is going to give the best performance.


Euclid's Algorithm (Greatest Common Divisor) According to Donald Knuth in "The Art Of Computer Programming", the following is how to find the greatest common divisor of two positive numbers, m and n, using Euclid's Algorithm.

  Divide m by n and let r be the remainder.
  If r is 0, n is the answer; if r is not 0, continue to step 3.
  Set m = n and n = r. Go back to step 1.

Below is my implementation of this algorithm.

#include <stdio.h>

int greatestCommonDivisor(int m, int n)
{
    int r;

    /* Check For Proper Input */
    if((m == 0) || (n == 0))
        return 0;
    else if((m < 0) || (n < 0))
        return -1;

    do
    {
        r = m % n;
        if(r == 0)
            break;
        m = n;
        n = r;
    }
    while(true);

    return n;
}

int main(void)
{
    int num1 = 600, num2 = 120;
    int gcd = greatestCommonDivisor(num1, num2);

    printf("The GCD of %d and %d is %d\n", num1, num2, gcd);

    getchar();
    return 0;
}

Though it appears to work just fine, it doesn't seem to be as elegant as I'd like, especially in the do-while loop.  Also, the while(true) seems dangerous, though I'm pretty confident that r will be reduced to 0 eventually and we'll break out of the loop.

Do you have any suggestions on either making the code more elegant or robust?

Firstly, your implementation is (arguably) not quite correct:

 else if((m < 0) || (n < 0))
     return -1;

The GCD of two negative numbers is perfectly well defined. However, if you really don't want the user to pass in negative numbers, make it explicit in the function signature:

unsigned greatestCommonDivisor(unsigned m, unsigned n)

Euclid's algorithm is one of the most basic examples of where recursion shines:

unsigned greatestCommonDivisor(unsigned m, unsigned n)
{
    if(n == 0) return m;
    return greatestCommonDivisor(n, m % n);
}

We first check the base case, which is 2: If r is 0, n is the answer; if r is not 0, continue to step 3. The recursive call actually combines steps 1 and 3 here: m % n sets n to be the remainder r, and we call the function again with n = m.

As a final note, a lot of people avoid recursive functions in C/C++ because most compilers don't optimise tail-calls to use constant stack space. However, Euclid's algorithm will perform only O(log N) steps, hence it's easy to see on any computer with (reasonable) stack size, this won't cause a stack overflow for numbers whose size is constrained by int or unsigned.


Basic SQL user database public void AddClient(Client obj){
  try{
      using(System.Data.SQLite.SQLiteConnection conn = stock.db.SqlLiteConnection.getSQLLiteConnection()){
      System.Data.SQLite.SQLiteCommand cmd = conn.CreateCommand();
      string sql="insert into Client (email,firstName,lastName,telephone,address,city,state,zip,web) values(@email,@firstName,@lastName,@telephone,@address,@city,@state,@zip,@web)";
      cmd.CommandText = sql;
      cmd.Parameters.AddWithValue("@email", obj.Email);
      cmd.Parameters.AddWithValue("@firstName", obj.FirstName);
      cmd.Parameters.AddWithValue("@lastName", obj.LastName);
      cmd.Parameters.AddWithValue("@telephone", obj.Telephone);
      cmd.Parameters.AddWithValue("@address", obj.Address);
      cmd.Parameters.AddWithValue("@city", obj.City);
      cmd.Parameters.AddWithValue("@state", obj.State);
      cmd.Parameters.AddWithValue("@zip", obj.Zip);
      cmd.Parameters.AddWithValue("@web", obj.Web);
      cmd.ExecuteNonQuery();
    }

  }catch (Exception ex){}
}

I have couple of questions for the above code

Do I have to close connection manually?
Do I need to dispose Command object?
If exception occurs, do I have to close connection and command
How do I enhance this code further?

OK, here goes:

No. You're using using which means that the object will be disposed of in the end of the block. As part of the Dispose() method, the connection will be closed and its resources released.
Not manually, you should open another using block to cause the object to Dispose() in the end.
Thanks to the using block, when an exception occurs, the object will be disposed because you're thrown out of the block.
"Enhance" is vague...


Test for an array being subset of another master array I was trying to build a small utility function to check if an array is part of other array. It's testing if an array is a subset of another master array.

const masterArray = [1,2,3,4,5,6];
const candidateArray = [2,5,6];

//Test for subset.

//create a set from the two.
const s1 = new Set(masterArray.concat(candidateArray));

//Compare the sizes of the master array and the created set
//If the sizes are same, no new elements are added that means
//the candidate is complete subset.
s1.size === masterArray.length;

Can this be handled in a better way?

So the key here is that I value code that's obvious to others.

Someone else is likely to try to stuff duplicate elements in unless it requires a type refactor; making const masterSet: Set<number> and using that throughout the code is a Good Idea. That "someone else" can often be you two months down the line, too.

In this case I would not say that the point of this length comparison is obvious; it will work but it takes some thinking -- that is why it has bugs with duplicate elements. In this case I would write something  which I find more straightforward like,

function isSubsetOf<x>(sub: Iterable<x>, sup: Set<x>): boolean {
  for (const x of sub) {
    if (!sup.has(x)) {
      return false;
    }
  }
  return true;
}

I am also hinting through the type system how this function works by insisting that the subset is any iterable -- so I must be iterating through it.

With a restriction to an array and a set, one can get a little swankier by using the reduce function,

function isSubsetOf<x>(sub: Array<x>, sup: Set<x>): boolean {
  return sub.reduce((acc, x) => acc && sup.has(x), true);
}

but probably not all other readers will find that as intuitive as I do, and the former has slightly better runtime characteristics on very large lists that are obviously not subsets.


Game of life - JavaScript I would like to kindly ask others to review my code. I'd like suggestions, criticisms, and discussions on what is good and what could be done better.

const canvas = document.querySelector('.gameOfLife');
const ctx = canvas.getContext('2d');

// create an array
const canvasWidth = 100;
const canvasHeight = 100;
canvas.width = 3 * canvasWidth;
canvas.height = 3 * canvasHeight;
let gameBoard = createBoard(canvasWidth, canvasHeight);
let gameBoard2 = createBoard(canvasWidth, canvasHeight);
let gameBoard3 = createBoard(canvasWidth, canvasHeight);

function createBoard(width, height) {
  let board = new Array(0);
  for (let i = 0; i < width; i++) {
    board [i] = [];
    for (let j = 0; j < height; j++){
      board[i][j] = 0;
    }
  }
  return board;
}

function randomNumber(max) {
  return Math.floor(Math.random() * max);
}

function randomPos() {
  const x = randomNumber(canvasWidth);
  const y = randomNumber(canvasHeight);
  return [x, y];
}

function randomBoard(arr) {
  for (let i = 0; i <= 0.2 * canvasHeight * canvasWidth; i++) {
    const [x, y] = randomPos();
    arr[x][y] = 1;
  }
}

function drawBoard(arr) {
  ctx.clearRect(0, 0, canvas.width, canvas.height)
  for (let i = 0; i < canvasWidth; i++) {
    for (let j = 0; j < canvasHeight; j++){
      if (arr[i][j] === 1) {
        ctx.fillStyle = "#012345";
        ctx.fillRect(i*3, j*3, 3, 3);
      }
    }
  }
}

// calculate new cycle

function checkSurrounding(x, y) {
  let sum = 0;

  startX = x-1 < 0 ? x : x-1;
  stopX = x+1 >= canvasWidth ? x : x+1;
  startY = y-1 < 0 ? y : y-1;
  stopY = y+1 >= canvasHeight ? y : y+1;

  for (let i = startX ; i <= stopX; i++) {
    for (let j = startY ; j <= stopY; j++) {
      if (i !== x || j !== y) {
        sum += gameBoard[i][j];
      }
    }
  }

  return sum;
}

function deadOrAlive(x, y) {
  const surround = checkSurrounding(x, y);

  if (gameBoard[x][y] === 1 && (surround === 2 || surround === 3)) return 1;
  else if (gameBoard[x][y] === 0 && surround === 3) return 1;
  else return 0;
}

function nextGene() {
  let newBoard = createBoard(canvasWidth, canvasHeight);

  for (let i = 0; i < canvasWidth; i++) {
    for (let j = 0; j < canvasHeight; j++) {
      newBoard[i][j] = deadOrAlive(i, j);
    }
  }

  gameBoard = newBoard;
  drawBoard(newBoard);

}

randomBoard(gameBoard);
drawBoard(gameBoard);

setInterval(nextGene, 150);
<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>Game of Life - Canvas</title>
</head>
<body>
  <canvas class="gameOfLife" width="400" height="400"></canvas>
  <script type="text/javascript" src="scripts.js"></script>
</body>

<style>
  body {
    background-color: #ffc600;
  }
  .gameOfLife{
    margin: 15px auto;
    background-color: #ff6c00;
    display: block;
  }
</style>
</html>

I have gone over the code briefly and looked at a couple optimizations, but haven't come up with any drastic changes. I will try to go over it again when I find time.
What is good
I like how the various functions are generally short. And the CSS/HTML is very simple too! It is a nice use of the canvas.
Suggestions
Iterations
Anytime you iterate over an array using a for loop, consider using a functional approach like .forEach(), .map(). That way, you won't have to handle the bookkeeping of iterating the variable (e.g. var i) and indexing into the array. For more information, I recommend going through these exercises. Though be aware that functional approaches are not always optimal/faster.
Creating board:
Inspired by the comment from Longfei Wu on this question, the function to create the board could take on a functional approach using Array.fill() and Array.map()
function createBoard(width, height) {
  return Array(width).fill(0).map(function() {
    return Array(height).fill(0);
  });
}

And if you are open to using ES-6 arrow functions, that can be even shorter:
function createBoard(width, height) {
  let board = Array(width).fill(0).map(() => Array(height).fill(0));
  return board;
}

See comparison in this jsPerf. When I ran it in Chrome the array fill-map approach was faster, though the opposite was true in Firefox, Mobile Safari and emphasized textEdge.
Drawboard:
.forEach() can be used here instead of the for statements:
function drawBoard(arr) {
  ctx.clearRect(0, 0, canvas.width, canvas.height)
  arr.forEach(function(innerArray, i) {//  for (let i = 0; i < canvasWidth; i++) {
    innerArray.forEach(function(cell, j) {//for (let j = 0; j < canvasHeight; j++){
      if (cell === 1) {
        ctx.fillStyle = "#012345";
        ctx.fillRect(i*3, j*3, 3, 3);
      }
    });
  });
}

And the same is true for nextGene().
deadOrAlive()
Because checkSurrounding() checks 3-9 spaces around the given cell, I see cases where it returns values greater than 3. Should deadOrAlive() have logic in those cases? It appears that it only has logic for return values of 2 and 3.
Updated snippet:

const canvas = document.querySelector('.gameOfLife');
const ctx = canvas.getContext('2d');

// create an array
const canvasWidth = 100;
const canvasHeight = 100;
canvas.width = 3 * canvasWidth;
canvas.height = 3 * canvasHeight;
let gameBoard = createBoard(canvasWidth, canvasHeight);
let gameBoard2 = createBoard(canvasWidth, canvasHeight);
let gameBoard3 = createBoard(canvasWidth, canvasHeight);

function createBoard(width, height) {
  return Array(width).fill(0).map(function() {
    return Array(height).fill(0);
  });
}

function randomNumber(max) {
  return Math.floor(Math.random() * max);
}

function randomPos() {
  const x = randomNumber(canvasWidth);
  const y = randomNumber(canvasHeight);
  return [x, y];
}

function randomBoard(arr) {
  for (let i = 0; i <= 0.2 * canvasHeight * canvasWidth; i++) {
    const [x, y] = randomPos();
    arr[x][y] = 1;
  }
}

function drawBoard(arr) {
  ctx.clearRect(0, 0, canvas.width, canvas.height)
  arr.forEach(function(innerArray, i) {//  for (let i = 0; i < canvasWidth; i++) {
    innerArray.forEach(function(cell, j) {//for (let j = 0; j < canvasHeight; j++){
      if (cell === 1) {
        ctx.fillStyle = "#012345";
        ctx.fillRect(i*3, j*3, 3, 3);
      }
    });
  });
}

// calculate new cycle

function checkSurrounding(x, y) {
  let sum = 0;

  startX = x-1 < 0 ? x : x-1;
  stopX = x+1 >= canvasWidth ? x : x+1;
  startY = y-1 < 0 ? y : y-1;
  stopY = y+1 >= canvasHeight ? y : y+1;

  for (let i = startX ; i <= stopX; i++) {
    for (let j = startY ; j <= stopY; j++) {
      if (i !== x || j !== y) {
        sum += gameBoard[i][j];
      }
    }
  }

  return sum;
}

function deadOrAlive(x, y) {
  const surround = checkSurrounding(x, y);

  if (gameBoard[x][y] === 1 && (surround === 2 || surround === 3)) return 1;
  else if (gameBoard[x][y] === 0 && surround === 3) return 1;
  else return 0;
}

function nextGene() {
  let newBoard = createBoard(canvasWidth, canvasHeight);

  newBoard.forEach(function(innerArray, i) {//  for (let i = 0; i < canvasWidth; i++) {
    innerArray.forEach(function(cell, j) {//for (let j = 0; j < canvasHeight; j++){
      newBoard[i][j] = deadOrAlive(i, j);
    });
  });

  gameBoard = newBoard;
  drawBoard(newBoard);

}

randomBoard(gameBoard);
drawBoard(gameBoard);

setInterval(nextGene, 150);
<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>Game of Life - Canvas</title>
</head>
<body>
  <canvas class="gameOfLife" width="400" height="400"></canvas>
  <script type="text/javascript" src="scripts.js"></script>
</body>

<style>
  body {
    background-color: #ffc600;
  }
  .gameOfLife{
    margin: 15px auto;
    background-color: #ff6c00;
    display: block;
  }
</style>
</html>


Amalgamation of the repository and command / query pattern I am coding a web based solution which consists of N number of tiers, these are:

UI
Web API
Business

The question I have is related to validation, repository and CRUD operations. I am adopting the command / query pattern whereby I have my repositories for data retrieval only (see below) and its query dependencies injected in. I have seen people use repositories for CRUD too, but then I get the impression it is doing too much thus warrants further abstraction.

public interface IAddressRepository
{
    Address GetById(int id);

    IEnumerable<Address> GetByPersonId(int personId);

    IEnumerable<Address> GetByPostalCode(string postalCode);
}

public AddressRepository(IGetAddressByIdQuery getAddressByIdQuery
                       , IGetAddressesByPersonIdQuery getAddressesByPersonIdQuery
                       , IGetAddressesByPostalCodeQuery getAddressesByPostalCodeQuery)
{
    this.getAddressByIdQuery = getAddressByIdQuery;
    this.getAddressesByPersonIdQuery = getAddressesByPersonIdQuery;
    this.getAddressesByPostalCodeQuery = getAddressesByPostalCodeQuery;
}

Should my repository return validation errors? For example, the GetById(int id) method will be checking if the id > 0 then make a DB call or simply return a new instance, likewise with a postal code lookup. However, I also think going by the SOLID design principle everything should be autonomous thus this approach of returning no validation errors is the correct choice. I choose to NOT return null on all my methods simply because checking for null is considered as an anti-pattern.

The encompassing of the CRUD logic.  I have seen uses of a Manager and/or Service classes, for example:

public interface IAddressManager
{
    IEnumerable<Error> Delete(Address address);

    IEnumerable<Error> Save(Address address);

    IEnumerable<Error> IsValid(Address address);
}

public AddressManager(IAddressValidator addressValidator, IAddressCommand addressCommand)
{
    this.addressValidator = addressValidator;
    this.addressCommand = addressCommand;
}

Is the general consensus to have a manager class which encompasses these operations but have the dependencies (the commands injected in)?  This way, before I call save(), I validate the object before persisting it. I can also control this behavior via TDD.

Query code example:

public interface IGetAddressByIdQuery
{
    Address Execute(int id);
}

public class GetAddressByIdQuery : IGetAddressByIdQuery
{
    private readonly string connectionString;

    public GetAddressByIdQuery(string connectionString)
    {
        this.connectionString = connectionString;
    }

    public Address Execute(int id)
    {
        using (var conn = new SqlConnection(this.connectionString))
        {
            return conn.Get<Address>(new { Id = id });
        }
    }
}

The data access tier is not apparent as at the moment as the business library is fairly small. I am a firm believer in not creating several libraries for the sake of it. Only create them when it is warranted to do so, i.e. a library is getting too big.

What are your thoughts as to whether or not a better approach / pattern exists?

I'll address your separate classes for the queries first. I definitely see where you're coming from with this approach and I can dig it, but it gives too much duplication as it is.

I assume you'll have more entities than just Addresses so using your approach, you would have to create a new interface for each entity you want to get by its ID. If you instead added some generics, you can do all of this with one interface:

interface IGetEntityByIdQuery<T> {
    T execute(int id);
}

Now you can have these implementations:

class GetAddressByIdQuery : IGetEntityByIdQuery<Address> {
    Address execute(int id) { }
}

class GetOrderByIdQuery : IGetEntityByIdQuery<Order> {
    Order execute(int id) { }
}

However even though I first said I can dig it, this will still be very cumbersome to work with. Having to create a new class for the retrieval for each entity for each parameter? That's too much.

I can agree on a setup where you combine them by entity:

interface IGetEntity<T> {
    T GetEntityById(int id);
    T GetEntityByPersonId(int id);
}

Now on to your remark about CRUD:

  I have seen people use repositories for CRUD too but then I get the impression it is doing too much thus warrants further abstraction.

The entire idea behind repositories is CRUD. In fact, your GetXByY classes are simply executing the R part in CRUD. Repositories exist to provide communication from your model to your datasource, which is what we understand under CRUD.

Another red flag here should have been your name choice: when you have a class named XManager you have to really overthink your design and be sure that this is the way to go. Often it is an indication something is wrong because in essence, a "manager" class doesn't convey much meaning.

What does this class do? Other names for such encompassing classes are a lot more informative, for example XFactory indicates the class will construct instances of X. A manager.. manages?

Therefore these actions should be added to your repository. This will now keep these related actions (CRUD) together which prevents any confusion with other programmers.

That being said: it might very well be a suitable name for a class, this is definitely not a general rule. I have had to use XManager classes as well before, but this was more because I simply couldn't come up with a better one (and no, it wasn't a repository ;)).

For more reflection upon the subject, I suggest reading this post.

As for your final question on the topic of errors: why are you doing validation logic in your data access layer? That is way too late!

You should have the guarantee that when an object is constructed, it has valid data. The entire idea behind getters and setters (aka: properties) is that the actual data is hidden and that the data can be validated.

When you construct a new object of type Address, it should be validated. This can be done in several places: properties, constructor, the factory that creates Address objects using some service layer, etc. There are many options to do this validation, but once the object is constructed you should have the implicit guarantee that the object contains valid data.

Lastly, for the boundary checks (like id > 0): this is up to your own preference. You could add a service layer in front of your repository that validates the incoming data and if you have several validation requirements then this might be a good solution. If on the other hand you only have this one boundary check then it's a little minor.

But I haven't come across the explicit recommendation to use a service layer in front of your data access layer just for validation yet, so as far as I'm concerned this is up to yourself.


Using a separate class in Symfony to handle a list of entities with controller injection In my Symfony (v2.7.9) project I have a set of entities which share some view functionality i.e being displayed in a table with some of their values as columns.

Following the separation of concerns principle, I've made a ListTable class which handles getting the table headers, rows and column values and I'm extending this for entities which need a little bit of extra functionality e.g ProductListTable

class ListTable extends AbstractLibrary {
    protected $items = [];
    protected $query;
    protected $controller;

    protected $tableKeys = [
        'name',
        'status',
    ];

    public function __construct( BaseController $controller ) {
        $this->controller = $controller;
        $this->query = $controller->request->query;
    }

    public function getTableKeys() {
        return $this->tableKeys;
    }

    public function getItems() {
        if ( empty( $this->items ) ) {
            foreach ( $this->controller->getResults() as $entity ) {
                $this->items[] = [
                    'id'      => $entity->getId(),
                    'href'    => $this->controller->getUrl( 'edit', [ 'id' => $entity->getId() ] ),
                    'columns' => $this->getColumns( $entity ),
                ];
            }
        }

        return $this->items;
    }
    ...

-

class ProductListTable extends ListTable {
    protected $tableKeys = array(
        'name',
        'sku',
        'price',
        'brand',
        'status',
        'date_added',
    );
    ...

Then within a controller method I create an instance of the class with:

new ProductListTable( $this );

There's just something I don't like about having to pass in the entire controller instance so I can use some of the methods/services within it. On the other hand, there are quite a few dependencies including some services I'd need to inject through the constructor if I don't inject the controller.

So what I'd like to know is, is it bad practice to inject the controller or is that preferable to injecting 5/6 dependencies as separate function arguments for the constructor? Or does the fact I require these dependencies within the ListTable class mean I'm going about it the wrong way?

controller injection

There are almost none use cases where injecting the controller is a good practice in my opinion.

controller should be in control, and not to be controlled. By design you should glue all the part inside the controller and give order in a way.
I assume that most of the functionality you need from the controller, are just another services and not functionality of the controller itself. If it is so then it better to inject the Container object and adding the ContainerAwareInterface
Still in the end it would be best to inject all 5-6 separate dependencies, because you won't hide your dependencies and your code would be easier to test.

multiple dependencies

If you start to inject many dependencies to an object most of the times it means you are giving to many responsibilities to that object. In general I would say that after 5 dependencies stop and think if it couldn't be grouped in new objects responsible for some parts of the logic.

Lets have an scenario:

you are working with data that are all in one language, suddenly your client wants to add multilang support.
you pass all the separate dependencies to our ListTable class, lets say repository, authUser, somethingOther ...
now you realize that you need to pass the current language, so you need to add dependency (object or string doesnt matter).

Best solution in this case would be to create a new class to handle all the responsibilities of retrieving the correct data to display.
Data gathering is one of the things that your ListTable should not do, as its main responsibility should be preparing/digesting the data for easy visualization.

On an example:

Class SecureDataProvider implements LocaleAwareInterface
{
   public function __construct(ObjectRepository $repository, User $authentificatedUser, $locale)
   {
    ...
   }
   public function getLatestRecords()
   {
      // checks if authenticated user got access to this operation or whatever
      // call the repository the get the record sorted by createdAt, passing the locale as query parameter to have the results in requested locale
      // ...

      return ArrayCollection($entities);
   }
}

now you would need to pass only this object to your ListTable.

new ProductListTable(new SecureDataProvider($articlesRepository, ..., ...) );

design of your class

As I said and above you need to separate your responsibilities. The listTable tries to do too much.

On another note visualizing a table is the VIEW in the MVC, and the best place to handle it is in the template.

The purpose of your class should be preparing the data, the way how these data would be shown is the point of View layer (or response).1
The data could in the end be in

table or list
graph (headers as labels),
json, xml or whatever other format from API.

That said the name should be probably changed to reflect the aspect of preparing the data and leave out the part what it will be in the end (table).

If you use twig, you probably would want to make macros for handling the different visual representation

A great reading material on OOP design: http://williamdurand.fr/2013/07/30/from-stupid-to-solid-code/


More convenient way to use comparators in Kotlin In Kotlin comparison operators can be used instead of compareTo method:

a > b   // a.compareTo(b) > 0
a < b   // a.compareTo(b) < 0
a >= b  // a.compareTo(b) >= 0
a <= b  // a.compareTo(b) <= 0

It's convenient and makes comparisons a lot more readable. However, these operators cannot be used if there is no suitable compareTo method, but there is  a suitable Comparator.

To make it possible to use them in this case, I've created a use extension function for a Comparator interface:

inline fun <T, R> Comparator<T>.use(block: ComparatorDSL<T>.() -> R) =
    ComparatorDSL(this).run(block)

Where ComparatorDSL is the following class:

inline class ComparatorDSL<T>(private val comparator: Comparator<T>) {
    operator fun T.compareTo(other: T) = comparator.compare(this, other)
}

Now I can use it like this:

comparator.use { a > b } // comparator.compare(a, b) > 0

Questions:

Is comparator.use { a > b } more readable than comparator.compare(a, b) > 0?
Is it a good idea to replace use with invoke operator or make this function infix?
Any improvement suggestions that you can think of

Is comparator.use { a > b } more readable than comparator.compare(a, b) > 0?

Readability is relative and more a point of view. Usually something is considered more readable, the more people say it is.

In my personal opinion both solutions are easy to read, while I like the syntax of the first more. Actually something very similar exists in the language already: compareBy

list.sortedWith(
  compareBy{ it.a }.thenBy { it.b }.thenByDescending { it.c }
)

It has all combinations you need and provides a nice, well readable syntax.

Is it a good idea to replace use with invoke operator or make this function infix?

use isntead of invoke

It is opinion based, just like readability. Its up to you and your team.

Infix

Making a function an infix function is just another customisation option for your syntax. The only important thing to remeber is:

  Infix function calls have lower precedence than the arithmetic
  operators, type casts, and the rangeTo operator. The following
  expressions are equivalent:

  1 shl 2 + 3 is equivalent to 1 shl (2 + 3)
  0 until n * 2 is equivalent to 0 until (n * 2)
  xs union ys as Set<> is equivalent to xs union (ysas Set<>)

  On the other hand, infix function call's precedence is
  higher than that of the boolean operators && and ||, is- and
  in-checks, and some other operators. These expressions are equivalent
  as well:

  a && b xor c is equivalent to a && (b xor c)
  a xor b in c isequivalent to (a xor b) in c

Kotlinlang.org

-

Any improvement suggestions that you can think of

Strongly consider to use compareBy like described before.


Calculate the day when Date is given as input using the Doomsday algorithm Was too bored in holidays...here's a code which finds the day of the week using the algorithm in this video. Please give a logical date in the correct format cause I didn't account for improper inputs cause I'm too lazy. My code is short and sweet.

day=["Sunday","Monday","Tuesday","Wednesday","Thursday","Friday","Saturday"]    #some database numbers and strings
mdoom=[3,28,14,4,9,6,11,8,5,10,7,12]
while True:
    entry=input('\nEnter a date in DD/MM/YYYY format:')         #Input date in a specified format
    date,month,year = map(int,entry.split('/'))                 #Get the date,month and year
    centdoom=[3,2,0,5]
    a=centdoom[((year//100)%4)-3]
    b=(year%100)//12
    c=(year%100)%12
    d=c//4                                                      #Random calculations for doomsday :)
    e=a+b+c+d

    if (year%4==0) and ((year%100!=0) or (year%400==0)):        #Checks for leap year
        mdoom[0],mdoom[1]=4,29

    ddoom=e%7                                                   #finds a date in the 1st week which has the same day as the doomsday
    r=mdoom[month-1]                                            #Gives the doomsdate in the input month
    r=r%7
    r=ddoom-r
    date=date%7                                                 #finds a date in the 1st week which has the same day as the input date
    print('\n',day[(r+date)%7])                                 #prints the output
    k=input('\nTry again?(y/n)')
    if k=='y':
        continue
    else:
        break

And any suggestions or improvements are welcome.

Your code is hard to understand, even after watching the video.

Make some functions. years_doomsday would help move some of the hard to understand information to be self-contained.
If you need to floor divide and get the remainder use divmod.
You can check the leap year using calendar.isleap.
You have a bug, if you ever enter a leap year then the non-leap years will return incorrect values for January and February.
You should make a function that calls years_doomsday, and returns the weekday.
You should make centdoom a global constant.
By rotating centdoom once you can remove the need for the -3.

import calendar

WEEKDAYS = ["Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"]
MDOOMSDAY = [3, 28, 14, 4, 9, 6, 11, 8, 5, 10, 7, 12]
MDOOMSDAY_LEAP = list(MDOOMSDAY)
MDOOMSDAY_LEAP[:2] = [4, 29]
CDOOMSDAY = [2, 0, 5, 3]

def years_doomsday(year):
    a, b = divmod(year, 100)
    c, d = divmod(b, 12)
    e = d // 4
    return (CDOOMSDAY[a % 4] + c + d + e) % 7

def doomsday(year, month, day):
    y_doomsday = years_doomsday(year)
    mdoomsday = MDOOMSDAY_LEAP if calendar.isleap(year) else MDOOMSDAY
    return WEEKDAYS[(day - mdoomsday[month-1] + y_doomsday) % 7]

if __name__ == '__main__':
    tests = [
        (2305, 7, 13, 'Thursday'),
        (1776, 7, 4, 'Thursday'),
        (1969, 7, 20, 'Sunday'),
        (1984, 1, 6, 'Friday'),
        (1902, 10, 19, 'Sunday'),
    ]
    for test in tests:
        if doomsday(*test[:3]) != test[-1]:
            print('Broken for', test)

If you don't want any fun, you can replace your code with datetime.date.isoweekday.

import datetime

for test in tests:
    if WEEKDAYS[datetime.date(*test[:3]).isoweekday()%7] != test[-1]:
        print('Broken for', test)


C program that recovers lost JPEG files My overall strategy is to load a block size (512bytes) into memory and check if that blocks first 4 bytes match a JPEGS first 4 bytes.

If it does then, I'll open a new out file and start writing the bytes into the out file. I'll repeat this for every "lost" JPEG.

You can assume that the "lost" JPEGs are stored consecutively.

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

int main( int argc, char *argv[] )
{
  //check for proper usage
  if ( argc != 2 )
  {
      fprintf(stderr, "Proper usage: ./recover file\n" );
      return 1;
  }

  //Remember name of file
  char *infile = argv[1];

  //open infile
  FILE *inptr = fopen( infile, "r");
  if ( inptr == NULL )
  {
      fprintf(stderr, "Could not open %s, please try again\n", infile );
      return 2;
  }

  //INIT block size and buffer
  int blockSize = 512;
  unsigned char buffer[blockSize];

  //Storage for file name and file
  char fileBuffer[8];
  FILE *outptr = NULL;

  //count how many images have been found
  int imageCount = 0;

  //iterate over the infile 512 bytes at a time
  while ( fread( buffer, blockSize, 1, inptr ) == 1 )
  {
      //look for jpeg header
      if ( buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0 ) {

          //close output file if its open
          if( outptr != NULL)
          {
              fclose(outptr);
          }

          //create the new file name and open that file
          sprintf( fileBuffer, "%03d.jpg", imageCount );

          outptr = fopen( fileBuffer, "w" );
          if ( outptr == NULL )
          {
              fprintf( stderr, "Could not create %s", fileBuffer );
              return 3;
          }

          //INCREMENT Image count
          imageCount++;
      }

      //write to out file if an image was found
      if( outptr != NULL )
      {
           fwrite( buffer, blockSize, 1, outptr);
      }

  }

  //close last photo
  fclose(outptr);

  //close input file
  fclose(inptr);

  //success
  return 0;

}

It looks like the choice of area to scan and iteration through the disk is outside the scope of this program, so we'll assume that this is a decent way of finding and recovering a jpeg file.

Implementation:

It looks like your program is set up to scan/find multiple images within the given region. As it is currently written, it will output 512 byte blocks per file if a given 512 byte block happened to start with the JPEG header. For this to be more or less general purpose, you should scan the block for the jpeg header, and then output, potentially across multiple blocks, to the same output file until you reach the end of the JPEG (FF D9, although I don't know if that can be found as random data within the image - if it can, you must parse the size fields of the format).
Going off of the Wikipedia article you can use more fields than FF D8 FF E0 (for instance, 4A 46 49 46 00 at a 4 byte offset after the initial magic. You also don't need to perform the (buffer[3] & 0xf0) operation - it looks like the whole E0 is part of the spec (although maybe some image readers don't care, but that's another story). Using a longer magic will reduce your number of false positives.
Maybe a "resume on failure" functionality could be useful? An output of where you were when the failure happened, as well as an allowed optional argument that specifies where to start within the block (you'll have to strtoull it or whatever, and then fseek in the file).

Code:

sprintf( fileBuffer, "%03d.jpg", imageCount ); can overflow the buffer. %03d guarantees that the name will be at least 3 characters (+4 from ".jpg" and +1 for the terminating '\0' for a total of 8). If your program finds more than 999 images, this sprintf will start corrupting memory. Use snprintf instead (which will truncate memory before corrupting if the correct arguments are passed - a good choice to pretty much always use it instead). In theory you could make your buffer size based on the maximum printable length of the imageCount, but for it to be truly portable you'd have to use uint32_t (or some fixed size) and can then base the buffer size on the printable length of UINT32_MAX.
If you use snprintf with the current buffer size, your program will start mangling names (to "1000.jp") as soon as it hits 1000 images.
You can put a "max check" in your loop so that you don't start overriding files at some point, although you'll probably never pass in input data that writes 4 mil images. E.g. if (imageCount == UINT32_MAX) (or whatever is appropriate for the type chosen).
fwrite( buffer, blockSize, 1, outptr); can fail, treat/warn/error.
fclose(outptr); can fail as well (in both spots), for the same reason as above. fwrite/fclose will buffer "under the hood" and call fflush at certain points to write out to disk. These fflush operations can fail at the whim of the filesystem.
fclose(inptr); in theory this can return some failure code as well, but since it's an input file I'm not 100% sure it's possible at all.


Simple DNA sequence finder w/ mismatch tolerance The goal with this function is to find one DNA sequence within another sequence, with a specified amount of mismatch tolerance.  For example:

dnasearch('acc','ccc',0) shouldn't find a match, while
dnasearch('acc','ccc',1) should find one.

EDIT: Function should also be able to find pat sequence in a sequence larger than the pat sequence; furthermore it should be able to find multiple matches within that sequence if they exist.  For example, dnasearch('acc','acaaatc',1) would find two matches: 'aca' and 'act'.  Thanks to @weronika for pointing this out.

Here's my working draft:

def dnasearch(pat,seq,mismatch_allowed=0):
patset1=set([pat])
patset2=set([pat])
for number in range(mismatch_allowed):
    for version in patset1:
        for a in range(len(version)):
            for letter in 'actg':
                newpat=version[:a]+letter+version[a+1:]
                patset2.add(newpat)
    patset1|=patset2
for n in patset1:
    if seq.find(n) != -1: print 'At position', seq.find(n),\
       'found',n

I was wondering how to write this function:

With fewer for-loops.
Without the second patset.  Originally I tried just adding new patterns to patset1, but I got the error: RuntimeError: Set changed size during iteration.
Simpler in any other way.

This is a solved problem in bioinformatics and there are specific algorithms depending on your specific use-case:

Do you allow gaps?
How many mismatches do gaps incur?
… etc.

In the general case, this is a global pairwise alignment which is computed by the Needleman–Wunsch algorithm. If you are just interested in the score of the alignment, rather than the complete alignment, this can be simplified by not performing the backtracking.

Now, if you have a threshold and are only interested in finding matches below said threshold, then a more efficient variant employs the Ukkonen trick – unfortunately, references for this are hard to find online, and the print literature is quite expensive. But what it does is essentially break the recurrence in the above-mentioned algorithm once the score exceeds the threshold chosen before (using the insight that the error score can only increase, never decrease in a given column).

But all this is unnecessary if you don’t allow gaps. In that case, the algorithm is as straightforward as walking over both strings at once and looking whether the current positions match.

And this can be expressed in a single line:

def distance(a, b):
    return sum(map(lambda (x, y): 0 if x == y else 1, zip(a, b)))

and:

def distance_less_than(k, a, b):
    return distance(a, b) < k


Makefile for a x86 operating system in C++ I recently had submitted my bashscript for building an OS for review and got valuable feedback, so I rewrote the entire script for a makefile:

Makefile

# Make File for Bulding Ultron

# Sets up the Assembler
AS := nasm
ASFLAGS := -f elf
ASFILES := $(wildcard kernel/boot/*.s kernel/arch/*.s)
ASOBJECTS := $(ASFILES:.s=.s.o)

# Compile C++ Files
CC := g++
CCFLAGS := -c -nostdlib -nostdinc -fno-builtin -fno-stack-protector -ffreestanding -m32 -I./sdk/include
CCFILES := $(wildcard *.cc sdk/lib/*.cc)
CCOBJECTS := $(CCFILES:.cc=.o)

# Linker
LINKER := ld
LINKERFLAGS := -m elf_i386 -T link.ld

# The Final Product will be ultron.img in the build directory
all: ultron.img

# Ultron.img : It is the Final Product formed by linking all the object files together
ultron.img : $(ASOBJECTS) $(CCOBJECTS)
        $(LINKER) $(LINKERFLAGS) -o build/$@ $^

# Run the Kernel : First Make the Kernel and then run it
run: ultron.img
        qemu-system-i386 -kernel 'build/ultron.img'

# Assemble the Assembly Files into object Files
# This Assembles all the Files into thier respective object files
# $< : Names the first prerequiste
# $@ : Names the current Target
%.s.o: %.s
        $(AS) $(ASFLAGS) $< -o $@

# Compile the C++ Files into corresponding object Files and place it in same directory
%.o: %.cc
        $(CC) $(CCFLAGS) -o $@ $<

# Clean this Mess
clean: $(ASOBJECTS) $(CCOBJECTS)
        $(RM) $^

This is my first makefile and I have a couple of questions:

Should I make the makefile as verbose as it is now?
Is my coding style Ok?
What are the things that I should keep in mind while writing makefiles?

Dependency Propagation

One thing that's missing is dependency propagation. So each .o file will be recompiled only if its corresponding .cc file has changed. This is good, I don't want to have to recompile foo.o just because I changed bar.cc. However, what if both foo.cc and bar.cc include some other file quux.h, and you changed some of the function signatures. Currently, neither foo.o nor bar.o will be recompiled! This is bad. However, you don't want to manually have to go through and add:

bar.o : quux.h
foo.o : quux.h

Since that is tedious, error-prone, and unmaintanable. You want to make the compiler do this for you.You can start with reading about automatic prerequisites, but rather than follow the guidelines there, note that gcc can both build the .o files and generate the .d files concurrently with -M (check out all the -M* arguments).

Targets

run and clean should be .PHONY targets.

I also like to get in the habit of setting .DEFAULT_GOAL instead of just making sure that all is at the top. You never know.

Comments

Very helpful for the most part. But I would omit these:

# $< : Names the first prerequiste
# $@ : Names the current Target

People who don't know what those mean likely won't understand anything about the makefile anyway, not sure the comments will help. Those are very commonly used.


Controller making request to an external API I'm starting on a new node/express backend and really want to keep everything clean and well tested. My previous project got a bit out of hand (in terms of test coverage and highly coupled code). So I'm going slow this time :-)

To be brief, this function currently sends a request to an API that responds with JSON data. It's a time based API (live results from football games) so I'm making use of some mocking tools so I don't have to spend my sunday afternoon's testing my app !

At the moment the function only returns a JSON object. Up next I want to serialise this to a database (Mongo probably).

Here's v0.1 of the controller:

function getFixture(cb){
  var url = "http://football-api.com/api/?Action=today&APIKey=" + secrets.APIKey + "&comp_id=1204";
  async.waterfall([
    function(callback){
      request.get(url, function(err, response, body){
        if (err) return callback(err);
        if (response.statusCode === 200) {
          var parsedJSON = JSON.parse(body);
          callback(null, parsedJSON);
        } else {
          var errorMsg = "Error in response with code: " + response.statusCode;
          callback(null, errorMsg);
        }
      });
    }
  ], cb)
}

module.exports = getFixture;

I'll include my tests below however they take a lot of space so I'll keep writing here. To be frank I'm still pretty hazy with exactly what's going on with the callbacks in node. So apologies from the outset if there are any obvious errors.

I've used async.waterfall from the get-go since I anticipate having another function that will save this parsedJSON object to a database, which naturally can only occur after the request/response cycle is complete.

Some context: this function will not be matched with a route. Instead it'll be hooked up to a cron job that will run once a Friday to get the scheduled matches for the weekend. So performance isn't really an issue, more so error handling and so on.

I'm starting to ramble, here are the tests (they all pass):

/* Module dependencies */
var nock = require('nock'),
  should = require('should');

/* Local dependencies */
var getFixture = require('../../controllers/api/fixture'),
  secrets = require('../../config/secrets'),
  jsonData = require('../mockData');

describe('Todays matches :controller', function() {

  describe('request/response life cycle', function() {
    it('sends a successful request', function (done) {
      var api = nock('http://football-api.com')
                  .get('/api/?Action=today&APIKey=' + secrets.APIKey + '&comp_id=1204')
                  .reply(200, {});

      getFixture(function (err, result) {
        should.not.exist(err);
        done();
      });
    });

    it('correctly handles an error response', function (done) {
      var api = nock('http://football-api.com')
                  .get('/api/?Action=today&APIKey=' + secrets.APIKey + '&comp_id=1204')
                  .reply(500, {});

      getFixture(function (err, result) {
        result.should.equal("Error in response with code: 500");
        done();
      });
    });
  });

  describe('Parsing JSON response', function() {
    it('stores the response in result', function (done) {
      var api = nock('http://football-api.com')
                  .get('/api/?Action=today&APIKey=' + secrets.APIKey + '&comp_id=1204')
                  .reply(200, jsonData.match_array);

      getFixture(function (err, result) {
        result.should.not.be.empty;
        done();
      });
    });

    it('is done correctly (attributes match)', function (done) {
      var api = nock('http://football-api.com')
                  .get('/api/?Action=today&APIKey=' + secrets.APIKey + '&comp_id=1204')
                  .reply(200, jsonData.match_array);

      getFixture(function (err, result) {
        var expectedMatchId = jsonData.match_array.matches[0].match_id;
        var actualMatchId = result.matches[0].match_id;

        actualMatchId.should.equal(expectedMatchId);
        done();
      });
    });

  });
});

After running the code through JSHint, you are missing a semicolon on line 16 of the first code swatch (], cb)). The tests look good on JSHint.

I suggest making the following changes:

Remove async.waterfall in getFixture

As you said in your own words, "performance isn't really an issue". As such, I'd recommend to just remove the waterfall function to make the code cleaner and easier to maintain. You probably won't run into a noticeable speed difference anyways, if it is on cron.

Shorten callbacks

This may be more opinionated, but I would suggest in the first swatch to change your callbacks from this format:

var errorMsg = "Error in response with code: " + response.statusCode;
callback(null, errorMsg);

to this format:

callback(null, "Error in response with code: " + response.statusCode);

It is just as easy to read (if not easier) and works just the same, except without the variable instantiation.

Shorten a line

function getFixture(cb) {...
module.exports = getFixture;

Change to

module.exports = function(cb) {...

Or, even (according to choice):

module.exports = function getFixture(cb) {...

Good luck!


FindQScintilla.cmake I'm using QScintilla in my project.  There's no good FindQScintilla.cmake out there, so I'm writing one myself.  However, I don't really know what I'm doing.  This is my attempt; it works, but I don't know how it'll work in reality:

find_path(QSCINTILLA_INCLUDE_DIR
  NAMES Qsci qsciglobal.h
  PATHS "${Qt5Core_INCLUDE_DIRS}" /usr/include/x86_64-linux-gnu/qt5
  PATH_SUFFIXES Qsci
)

find_library(QSCINTILLA_LIBRARY
  NAMES qt5scintilla2 libqt5scintilla2
  PATHS ${Qt5Core_LIBRARIES}
)

include(FindPackageHandleStandardArgs)

find_package_handle_standard_args(
  QScintilla
  FOUND_VAR QSCINTILLA_FOUND
  REQUIRED_VARS QSCINTILLA_INCLUDE_DIR QSCINTILLA_LIBRARY
  VERSION_VAR QSCINTILLA_VERSION
)

if(QSCINTILLA_LIBRARY AND QSCINTILLA_INCLUDE_DIR)
  set(QSCINTILLA_LIBRARIES ${QSCINTILLA_LIBRARY})
  set(QSCINTILLA_INCLUDE_DIRS ${QSCINTILLA_INCLUDE_DIR})
endif()

mark_as_advanced(QSCINTILLA_INCLUDE_DIR QSCINTILLA_LIBRARY)

My development environment is Ubuntu 16.04, but I want this package to work on all major platforms.

More specifically:

Am I missing anything?
How will this hold up on different operating systems?

I'm targeting Windows, Mac, and Linux, though if there's a quick and easy way I can target another OS then I'll do that too.

Overall I think you have all the major pieces there, but I would also taking your find module a step further and create a target if QScintilla is found. You can do this by adding the following:

if(QSCINTILLA_LIBRARY AND QSCINTILLA_INCLUDE_DIR)

  set(QSCINTILLA_LIBRARIES ${QSCINTILLA_LIBRARY})
  set(QSCINTILLA_INCLUDE_DIRS ${QSCINTILLA_INCLUDE_DIR})

  if(NOT TARGET QScintilla::QScintilla)
      add_library(QScintilla::QScintilla UNKNOWN IMPORTED)
      set_target_properties(QScintilla::QScintilla PROPERTIES
           IMPORTED_IMPLIB ${QSCINTILLA_LIBRARY}
           INTERFACE_INCLUDE_DIRECTORIES ${QSCINTILLA_INCLUDE_DIR})
  endif(NOT TARGET QScintilla::QSintilla)

endif(QSCINTILLA_LIBRARY AND QSCINTILLA_INCLUDE_DIR)

Now you can use the library like so: target_link_libraries(MyProject PUBLIC QScintilla::QScintilla). Feel free to change the imported target name.

One minor thing I would suggest (that you can see in the code above ^) it to have your if() conditions match inside the corresponding end() call. So if you have if(condition), the matching end() should also look like end(condition).

As for cross platform support it's difficult to say without trying it on multiple platforms. I'm not sure about your find_path call. The use of ${Qt5Core_INCLUDE_DIRS} should be portable enough but it's tough to say.

One other nuance with find modules that I personally find can be troublesome is that on windows, if the library is a shared library you need to find the path to the .lib file and the .dll since that binary file isn't necessarily on the user's environment path. This is more of a nice to have and something I do for all find modules I write, but I work mostly on windows anyway.


Python adding machine I was wondering if there is any way to make this code smaller/condensed. It is for a school project which says it has to be as small as it can be and I am not sure whether it can go any smaller.

num1 = float(input("Give first number "))#Get first number
num2 = float(input("Give second number ")) #Get second number
ans = num1 + num2 #Work out answer
if ans % 1 == 0: #If its a whole number
    print (int(ans)) #Convert to int so its for example 14 instead of 14.0
else: #If not a whole number
    print (ans) #Print the answer

Without actually code-golfing, there are still a few shortenings possible:

def ask(n):
    return float(input("Give %s number " % n))

ans = ask("first") + ask("second")
print(int(ans) if not ans % 1 else ans)

This uses a function for asking for the numbers, does not store the intermediate values, uses a ternary expression for printing and the fact that zero evaluates to False.

If you actually want it as small as possible (in bytes), this is a start, but can probably be golfed down a lot further (e.g. by encoding the strings):

a=lambda n:float(input("Give %s number "%n))
r=a("first")+a("second")
print((int(r),r)[r%1>0])

Have a look at the tips for golfing in python.

If you also want to forego the interface:

r=sum(float(input())for _ in"_"*2)
print((int(r),r)[r%1>0])


Copying contents between BufferedStreams in Java This is a simple function that copies the content of one stream to another. My question is simple. I am currently, copying between the streams one byte at a time, but the streams are both Buffered. What are some ways to optimize it for speed? Any other advice would be appreciated as well.

private boolean copyToStream(BufferedInputStream inputStream, BufferedOutputStream fileStream) throws IOException {
    int byt;
    while ((byt = inputStream.read()) != -1) {
        fileStream.write(byt);
        }
    }
    return true;

}

Instead of reading the buffered stream byte-by-byte inputStream.read() you might use the advantage of buffering. This will speed up the large content copy.

private boolean copyToStream(BufferedInputStream inputStream, BufferedOutputStream fileStream) throws IOException {
    byte[] buffer = new byte[BUFFER_SIZE];
    int n;
    while ((n = inputStream.read(buffer)) != -1) {
        fileStream.write(buffer, 0, n);
    }
}

Also, consider java.nio.FileChannel transferTo() which may allow using DMA.


Move an element up / down an array in Javascript I have an array called list bound to the current application scope $scope that is a user order-able list of UI components. When clicking the up / down arrays these functions are responsible for moving the element by re-inserting it in the correct place.

$scope.moveUp = function(index)
{
    if (index === 0) {
        return;
    }
    $scope.list.splice(index - 1, 0, $scope.list.splice(index, 1)[0]);
};

$scope.moveDown = function(index)
{
    if (index === $scope.list.length - 1) {
        return;
    }
    $scope.list.splice(index + 1, 0, $scope.list.splice(index, 1)[0]);
};

What you are doing isn't very efficient. First you remove the element from the array, and to close the gap, every single element after it has to move position. Then you insert the element again, and to make room for it, every single element after it has to move position again, back to where they started.1

What you are doing is simply swapping to elements in the array. There is no need to do any array manipulation or touch any of the other elements.

$scope.moveUp = function(index)
{
    if (index === 0) {
        return;
    }
    [$scope.list[index], $scope.list[index-1]] = [$scope.list[index-1], $scope.list[index]];
};

$scope.moveDown = function(index)
{
    if (index === $scope.list.length - 1) {
        return;
    }
    [$scope.list[index], $scope.list[index+1]] = [$scope.list[index+1], $scope.list[index]];
};

I'm using destructuring to swap the values. It's basically a way to assign multiple values at the same time without needing a temporary variable.

1 That's how it works in theory at least. It will depend on the internal working of the JavaScript engine, and whether it has been optimized for this kind of task.


Listener/Observer Model in PHP I've taken a look at the SPL Listener/Observer model but found that it doesn't work when using static methods. So I wrote my own but it's very simply.  Can anyone suggest ways to make it more like the Model standards and if there is anything obvious that I've missed?

<?php
namespace modular\core;

class Listeners{
    private static $listeners = array();
    private static $triggered = array();
    private static $broadcasts = array();

    public static function attach($event, $key, $callback){
        $config = \modular\core\ModuleLoader::getModuleConfig($key);
        $config['instance'] = get_class($callback[0]);
        $callback['config'] = $config;
        static::$listeners[$event][md5($event.$key)] = array('module' => $key, 'callback' => $callback);
    }

    public static function detach($event, $key, $callback){
        unset(static::$listeners[$event][md5($event.$key)]);
    }

    public static function listeners($event = ''){
        return ($event == '') ? static::$listeners : static::$listeners[$event];
    }

    public static function listenersRegistered(){
        return static::$listeners;
    }

    public static function listenersTriggered(){
        return static::$triggered;
    }

    public static function getBroadcasts(){
        return static::$broadcasts;
    }

    public static function broadcast($event){
        $rendered = \modular\core\Renderer::rendered();

        $caller = debug_backtrace();//true, 1 - check version and implement this
        array_shift($caller);

        static::$broadcasts[$event][] = $caller;

        if(isset(static::$listeners[$event])){
            if(!$rendered)
                ob_start();

            foreach(static::$listeners[$event] as $listener){
                $myEvent = (\modular\core\Router::routed() ? ($listener['module'] == \modular\core\ModuleLoader::getActiveModule()) : true);
                if($myEvent){
                    static::$triggered[$event] = $listener;
                    $test = \modular\core\ModuleLoader::runModule($listener);
                }
            }

            if(!$rendered)
                ob_end_clean();
        }
    }
}
?>

The code to add a listener:

public static function addListener($event, $callback){
        $modules = \modular\core\ModuleLoader::getModules();

        if(($key = array_search($callback[0], $modules)) !== false){
            \modular\core\Listeners::attach($event, $key, $callback);
        }
        else{
            //problem - object not found...
        }
    }

_::addListener('pre-render', array(&$this, 'init'));

Callback is a callback function set as an array to specify the instance

The MD5 is used to combine both the $event and $key as a $key (module) can register to multiple events and vice-versa.

Broadcast is purely checking if any output has already occurred, if not then block all output if so then allow output whilst it calls the module callback method.

Because it's a framework static methods are used as my preferred method to access a class from multiple classes without having to worry about creating variables of instances that I either have to remember globally or re-instantiating the class constantly.

Because it's a framework static methods are used as my preferred method to access a class from multiple classes without having to worry about creating variables of instances that I either have to remember globally or re-instantiating the class constantly.

I don't do php, but I'll give this one a try.

Being a framework shouldn't be a reason to prefer static methods everywhere.

The Observer Pattern can be depicted as follows:

  I've taken a look at the SPL Listener/Observer model but found that it doesn't work when using static methods.

There's a reason for that. If I read the code correctly, then when you have a class called SubjectA, and register 3 observers statically, every single instance of SubjectA will have the 3 observers registered; this is unlikely to be the intended behavior (not to mention threading issues), and it's only happening because of the static stuff.1

It looks like your Listeners class is intended to be used in object composition, like, you'll create a Car class that has a Listeners instance, and you'll register listeners for an Explode event. Then you might have a Dog class that has a[nother] Listeners instance, and you'll register listeners for a Bark event. If I assessed the situation correctly, that will give you a barking Car and an exploding Dog, on top of the expected events.

1 that is, if static in php has the same meaning as static in c#, which I'm assuming here.


Course Grade Calculator Here is my objective:

Ask how many categories there are for the class (ex: Labs, Tests, Homework, Quizzes), and then ask what total percentage each make up for the class (ex: Lab are 15%, tests are 60%, so on), and then ask how many items there are under each category (ex: 7 labs, 3 tests, so on), and then ask for the max points of each items in each category, and finally ask what I got on each one.

I could make a pretty simple one for each class, but I wanted to be able to have one program that does it for any class that I can continue to use through my years at university. I'm not advanced by any means, which is why I want to take on this side project. I was hoping some of you could help me out in finishing it up.

Also, I don't know how to make arrays as big as the user wants, so I just put large enough numbers so that I won't ever need more. Probably not good practice, not sure.

int main()
{
    string categories[10];
    int percent[10];
    int totalPoints[50];
    int totalItems[50];
    int a = 0;

    cout << "How many grade categoies do you have: ";
    cin >> a;

    for(int i = 1; i <= a; i++)
    {
        cout << "Enter the category title: ";
        cin >> categories[i];
    }
    for(int i = 1; i <= a; i++)
    {
        cout << "Enter the total percentage for " << categories[i] << ": ";
        cin >> percent[i];
    }
    for(int i = 1; i <= a; i++)
    {
        cout << "How many items are there for " << categories[i] << ": ";
        cin >> totalItems[i];
    }

    return 0;
}

What do you plan to do if a class doesn't grade like this?  I've had classes where each assignment, test, and quiz had its own weight.  Are you just going to make each its own category?  Note that you'll have to calculate the percentages prior to entering the data here.  Unfortunately, not every professor will return grades in the form of percentages.

If I had this as a task, I think that I'd just use a spreadsheet for it.  A spreadsheet already has the relevant computing capacity and is very flexible in the face of arbitrary rules.  It also supports saving, which this program doesn't.  You'll have to enter the categories and such every time you use the program.  With a spreadsheet, you'd save and would just need to update as grades arrive.

All that said, for learning purposes, I'm going to go over the code that you have.

string categories[10];
int percent[10];

This looks risky.  The fact that you are using 10 in two places suggests that it has special significance.  If so, you should declare a constant to hold the value:

const int MAX_CATEGORIES = 10;

Another problem is that you apparently want categories and percents to be linked, one percent per category.  So the C++ way to handle this is to make a class, but in this case, let's make a struct instead:

struct Category {
    public std::string name;
    public float percentOfGrade;

We use a struct because I'm not up to explaining classes in an answer.  Anyway, now you can just declare one array rather than two (yes, std::vector would be better, but it would bypass some of the things that I want to criticize).

Category categories[MAX_CATEGORIES];

Notice that categories is plural.  I use this to tell me that categories refers to a collection of objects.  If I were referring to just one category, I'd use a singular name.

Let's continue looking at your code:

int totalPoints[50];
int totalItems[50];

OK, first we have the same problem as before.  It looks like 50 has some significance, so we want to make it a constant, say MAX_ITEMS_PER_CATEGORY.  However, looking at your code, totalItems is an array showing how many items in a category.  That suggests that it should not have MAX_ITEMS_PER_CATEGORY entries.  In fact, we should add it to the category struct:

    public int numberOfItems;
};

Note that I renamed it.  I don't like totalItems as a name because total is singular but you're defining an array.  I would expect something named total to be the sum of the items or something like that.

I'm going to leave off describing what to do with totalPoints, as you haven't written code using it.  Again though, I don't like the name.

cout << "How many grade categoies do you have: ";

Note that categories is misspelled there.  Also, you probably should have written std::cout`` rather than plaincout`.

for(int i = 1; i <= a; i++)
{
    cout << "Enter the category title: ";
    cin >> categories[i];
}
for(int i = 1; i <= a; i++)
{
    cout << "Enter the total percentage for " << categories[i] << ": ";
    cin >> percent[i];
}
for(int i = 1; i <= a; i++)
{
    cout << "How many items are there for " << categories[i] << ": ";
    cin >> totalItems[i];
}

First, why do three for loops when one would do?  Avoid things where two or more parts of your program have to synchronize.  These lead to bugs as synchronicity is hard to maintain.

Second, C++ arrays are zero-indexed.  You're starting with 1 but should be starting with 0.  The end changes as well.  Note the < where there was a <=.

for ( int i = 0; i < numberOfCategories; i++ )  {
    std::cout << "Enter the category title: ";
    std::cin >> categories[i].name;
    std::cout << "Enter the total percentage for " << categories[i].name << ": ";
    std::cin >> categories[i].percent;
    std::cout << "How many items are there for " << categories[i].name << ": ";
    std::cin >> categories[i].numberOfItems;
}

I also changed a to numberOfCategories for readability.

And of course, I started using the struct fields that we declared.

return 0;

You don't need to return from main at the end of execution.  The compiler will handle this for you.  Only return if you need to do so early.

If you do explicitly return, then do it as follows:

return EXIT_SUCCESS;

or

return EXIT_FAILURE;

This is cross-platform and doesn't rely on the OS understanding 0 the same as you do.

Note:  I hinted at this previously, but I'll say it explicitly now.  A class would be a better solution in general.  I only skipped it because it's too complicated to explain.  The struct solution is inferior but closer to how your existing code worked.  Similarly, a std::vector would be a better data structure but is less like your current code.  I prioritized critiquing your current code over restructuring it.

Another problem that I slid past is that you're doing no validation of your input.  If someone enters input that doesn't fit, you should either handle it or abort the program.  For example, if numberOfCategories is greater than MAX_CATEGORIES, you will get a runtime error when adding the first extra category.


List creation of BST's nodes I want to create lists of the nodes of a binary tree at each depth.
So if the depth is X I would have X lists.
Please see my approach. I think it is correct. Am I right to believe it is correct? Any input is welcome:

public List<List<BinaryNode>> getNodesPerDepth(BinaryNode root){
     if(root == null) {
         throw new IllegalArgumentException();
     }
     List<LinkedList<BinaryNode>> result = new LinkedList<LinkedList<BinaryNode>>();
     result.add(new LinkedList<BinaryNode>());
     getNodesPerDepth(result, root, 0);
     return result;
}

private void getNodesPerDepth(List<LinkedList<BinaryNode>> lists, BinaryNode root , int depth){
      if(root == null){
        return;
      }
      lists.get(depth).add(root);
      if(lists.size() < depth + 1 && (root.left != null || root.right != null)){
           lists.add(new LinkedList<BinaryNode>());
      }
      getNodesPerDepth(lists, root.left, depth + 1);
      getNodesPerDepth(lists, root.right, depth + 1);
}

Do you really want to go recursive on this? Binary trees in the worst case may be a linear list. A better idea is to do some thing like this:

getNodesPerDepth(rootNode) {
    list.add(rootNode); // Start with the root node,
    dlst = new List() // depth lit
    while(list.size() != 0) {
        nextlst = new List(); // make a list of children to iterate next phase.
        for(e : list) {
          for (c : e.children())
             nextlst.add(c);
        }
        dlst.add(list);
        list = nextlst;
    }
    return dlst;
}


Dynamic Programming with Project Euler problem 67 I am a beginner with python and programming in general. I started teaching myself about 2 months ago.

I've been working my way through Project Euler and a few similar sites to build my chops, and because I find it fun/rewarding. Below is my first attempt at some simple dynamic programming to come up with the solution to problem 67.

I would appreciate any criticism or advice. One thing in particular that I struggle with is readability (naming variables, comments, etc.). Also, I am largely ignorant when it comes to convention/best practices, so guidance here would be helpful.

pyr = []
with open('problem_67.txt') as f:
    for line in f:
        pyr.append(map(int, line.split()))
f.closed

def best_path(li):
    """Find the max total from top to bottom in a triangular array."""
    saver = {}
    n = len(li)

    # Set values for all points to 0
    for y in xrange(2, n+1):
        for x in xrange(len(li[n-y])):
            saver[(n-y, x)] = 0

    # Establish the max values for the bottom row
    for x in xrange(len(li[n-2])):
        a, b, c = li[n-2][x], li[n-1][x], li[n-1][x+1]
        saver[(n-2, x)] += max(a+b, a+c)

    # Add the highest dict of the two lower options for each other row
    for y in xrange(3, n+1):
        for x in xrange(len(li[n-y])):
            a, b, c = li[n-y][x], saver[(n-(y-1), x)], saver[(n-(y-1), x+1)]
            saver[(n-y, x)] += a + max(b, c)

    return saver[0, 0]

def main():
    print best_path(pyr)

if __name__ == "__main__":
    main()

I recommend running flake8 occasionally, and cleaning up what it complains about. And consider using python3 rather than python2.

If you really want to define a global with pyr = [], then at least name it PYR. Much better for main to call

    print(best_path(get_pyramid()))

and push the init code into a getter. Then you won't be polluting the global namespace with f. (And no need to retain the f.closed debugging statement, once you've convinced yourself that with closed it as advertised.)

EDIT: On reflection, I was viewing this in too mechanical a way. 200_success is correct, all caps is for manifest constants. The real trouble here is gratuitous globals. Scope things down appropriately without polluting the top-level namespace.

Naming the triangular pyramid pyr was a fine choice, better than the parameter name li.

Using xrange is a bit odd, nowadays. Just stick to python3, and range will be perfect.

A name like cost (or distance, or dist) would have been more natural than saver.

If you have your heart set on a triangular map of zeros, this would be more pythonic:

# Set costs for all points to 0
for y, row in enumerate(li):
    for x, val in enumerate(row):
        saver[(x, y)] = 0

But you're using it with +=, so better to instead define saver = collections.defaultdict(int), allowing increment to lazily populate the map.

This line is fairly clear:

        saver[(n-y, x)] += a + max(b, c)

The n-y could have been just y (in several expressions) if you had supplied a third optional argument of -1 for the range. And it seems likely you could move the max(a+b, a+c) expression into this loop, as well, perhaps by padding the bottom of the pyramid with a line of zeros.

Explicitly supplying a tuple as key is a good practice, one worth sticking to here:

return saver[0, 0]

This is a fine comment:

"""Find the max total from top to bottom in a triangular array."""

Writing lots of comments may not be necessary if your identifiers accurately tell the story. Consider renaming the function to best_path_cost, since it returns a scalar cost rather than a path through the pyramid.

This is a fine comment, though I would prefer "costs" over the vague "values":

# Set values for all points to 0

You might consider deleting it, and moving the code into a helper with a descriptive name like get_triangle_of_zeros(). Writing fewer comments leads to writing fewer comments that lie (or bit-rot). Functions typically should be defined with a docstring comment, as you supplied. If a function is very simple, it needs a descriptive name but perhaps no comment.


Functions to handle big numbers in C I'm working to implement some simple bigint functions for addition, multiplication and powers. They work correctly but the larger the numbers the longer the time it takes.
For example 210000 takes a very long time.
Here are all the functions I'm using.

char    *ft_strnew(size_t size)
    {
    char    *temp;
    size_t  i;

    if (!(temp = (char*)malloc(sizeof(char) * (size + 1))))
        return (NULL);
    i = 0;
    while (i < size)
    {
        temp[i] = '\0';
        i++;
    }
    temp[i] = '\0';
    return (temp);
}

char    *ft_strjoin(char const *s1, char const *s2)
{
    char    *temp;
    int     i;
    int     j;

    if (!(temp = (char*)malloc(sizeof(char) *
        (ft_strlen(s1) + ft_strlen(s2) + 1))))
        return (NULL);
    i = 0;
    j = 0;
    while (s1[i])
    {
        temp[j] = s1[i];
        i++;
        j++;
    }
    i = 0;
    while (s2[i])
    {
        temp[j] = s2[i];
        i++;
        j++;
    }
    temp[j] = '\0';
    return (temp);
}

int         left_one(int *i, char c)
{
    if (--(*i) < 0)
        return (0);
    else
        return (c - '0');
}

char        *add_char(char *nbr1, char *nbr2)
{
    int     i;
    int     j;
    int     left[4];
    char    *temp;

    left[0] = 0;
    temp = ft_strnew(0);
    i = ft_strlen(nbr1);
    j = ft_strlen(nbr2);
    if (i > j)
        left[3] = i;
    else
        left[3] = j;
    while (--left[3] >= 0)
    {
        left[1] = left_one(&i, nbr1[i - 1]);
        left[2] = left_one(&j, nbr2[j - 1]);
        left[0] += left[1] + left[2];
        temp = ft_charjoin((left[0] % 10) + '0', temp, 'l');
        left[0] /= 10;
    }
    if (left[0] > 0)
        temp = ft_charjoin((left[0] % 10) + '0', temp, 'l');
    return (temp);
}

char        *multi1_char(char *nbr1, int nbr2)
{
    int     i;
    char    *temp;

    if (nbr1[0] == '0' || nbr2 == 0)
        return ("0");
    i = -1;
    temp = ft_strdup(nbr1);
    while (++i < nbr2 - 1)
        temp = add_char(temp, nbr1);
    return (temp);
}

char        *multi_char(char *nbr1, char *nbr2)
{
    int         i;
    char        *temp;
    int         j;
    char        *temp1;
    int         t;
    char        *mul;

    mul = ft_strnew(0);
    temp = ft_strnew(1);
    j = 0;
    i = ft_strlen(nbr2);
    while (--i >= 0)
    {
        t = -1;
        temp1 = multi1_char(nbr1, nbr2[i] - '0');
        //temp1 = multi1_char(temp1, 10*j);
        /*while (++t < j)
            temp1 = ft_strjoin(temp1, "0");*/
        temp1 = ft_strjoin(temp1, mul);
        temp = add_char(temp, temp1);
        mul = ft_strjoin("0", mul);
        j++;
    }
    return (temp);
}

char    *power_char(int nbr1, int nbr2)
{
    char        *temp;

    temp = ft_strnew(0);
    if (nbr2 == 1)
        return (ft_itoa(nbr1));
    if (nbr2 % 2 == 0)
    {
        temp = power_char(nbr1, nbr2 / 2);
        return (multi_char(temp, temp));
    }
    else
    {
        nbr2 -= 1;
        temp = power_char(nbr1, nbr2 / 2);
        return(multi_char(ft_itoa(nbr1), multi_char(temp, temp)));
    }
}

I came up with a way to make the function that handles powers work faster but still the multiplication function is slow for big numbers.

Is there any way of improving my functions or are they bad from the start and I should start all over?

It seems you're allergic to for loops for some reason; for example, in ft_strnew:

for (size_t i = 0; i < size; i++)

There are examples elsewhere.

Don't predeclare your variables, e.g.

int     i;
int     j;

This hasn't been needed for 20 years. Declare them where they're used.

Replace this entire loop:

size_t  i;
i = 0;
while (i < size)
{
    temp[i] = '\0';
    i++;
}
temp[i] = '\0';

With this:

memset(temp, '\0', size+1);

Otherwise, your entire approach to arbitrary-size integer math is a quite slow one. (Other libraries have solved this, but it's nice that you're trying to learn.) You can't be doing this character by character; rather you need to do it integer by integer. Do some googling about arbitrary-precision integer arithmetic on 64-bit architectures.

A comment on something you said:

  They work correctly but the larger the numbers the longer the time it takes.

No arbitrary-precision library can escape this. The execution time of even the best libraries will scale according to the size of the input when it exceeds the size of the machine word.


ROC and AUC calculation Using Python 2.7 and here is my code to calculate ROC/AUC and I compare my results of tpr/fpr with threshold, it is the same result of whay scikit-learn returns.

My questions, (1) any ideas for improvements (especially for performance in my existing code) (2) any smart ideas to calculate of AUC?

import numpy as np
from sklearn import metrics

def homeMadeROC(scores, targets):
    totalPositive = sum(targets)
    totalNegative = len(targets) - totalPositive
    sortedTargets = []
    tprs = []
    fprs = []

    # order by score desc, and get related target
    for (s,t) in sorted(zip(scores, targets), reverse=True):
        sortedTargets.append(t)
    sortedScores = sorted(scores, reverse=True)
    predictedTruePositiveSofar = 0
    predictedFalsePositiveSofar = 0

    for i in range(len(sortedScores)):
        if sortedTargets[i] == 1:
            predictedTruePositiveSofar += 1
        else:
            predictedFalsePositiveSofar += 1
        tprs.append(float(predictedTruePositiveSofar)/totalPositive)
        fprs.append(float(predictedFalsePositiveSofar)/totalNegative)

    print fprs
    print tprs
    print sortedScores

if __name__ == "__main__":

    y = np.array([0, 0, 1, 1])
    pred = np.array([0.1, 0.4, 0.35, 0.8])
    fpr, tpr, thresholds = metrics.roc_curve(y, pred)
    print fpr
    print tpr
    print thresholds
    print metrics.auc(fpr, tpr)

    print "======"

    homeMadeROC([0.1, 0.4, 0.35, 0.8],[0, 0, 1, 1])

I don't know any machine learning, however, there are some ways your code can be improved.
PEP8 is Python default style guide,
and so we use lower_case_with_underscores rather than mixedCase for most names.
The major exception to this is classes that use CapitalizedWords.

If we look at your variable names:

total_positive is incorrect, it's the sum of targets, so could be called total_targets.
total_negative is even more incorrect, it's the length minus the total.
sortedTargets is correct.
tprs seems like it means something well known, so it's a good name. Also the plural is great as it's a list.
predictedTruePositiveSofar is long, instead I'd move this into it's own function and then use it once in a loop.

And so I don't like most of your names.
Try to pick good names, like tprs, for all your variables.
Naming is hard, so write a concise description of the variable in the context of the function.
But if that's too big, you may want to make a new function.

The code is mostly ok, but I'd move the initialization of your lists to where they are used.
I'd also change your two sorted calls to a single one with two zips.
To do this you need to use the * operator, to expand a list to arguments.
Which is quite well explained here.
But in short, range(1, 10, 2) is the same as range(*[1, 10, 2]).
This simplifies the creation of sorted_scores and sorted_targets.

After this I'd make a function accumulate_truth.
This will yield the amount of truthy and falsy values.
And so can move the predicted_true_positive_so_far into it's own known function.
As naming it something useful will be hard in the current context.
I'd also change this function to iterate through the list,
rather than through a range and index,
and to remove the == 1 as that can change how the function works.
This is as bool([1]) is True, but [1] == 1 is False.
So your current implementation removes some truthy values.

To simplify the creation of tprs and fprs I'd change the creation of total_positive to be a float.
This changes total_negative to a float too, and makes all the items in tprs become floats too.
Alternately you can use __future__.division.

Finally try not to merge printing and doing work in a single function.
Instead you should return tprs and fprs, and print them in the __main__.

This can result in:

from __future__ import division

def accumulate_truth(iterable):
    true = 0
    false = 0
    for i in iterable:
        if i:
            true += 1
        else:
            false += 1
        yield true, false

def home_made_ROC(scores, targets):
    total_targets = sum(targets)
    length_sub_targets = len(targets) - total_targets

    scores, targets = zip(*sorted(zip(scores, targets), reverse=True))
    tprs = []
    fprs = []
    for true, false in accumulate_truth(targets):
        tprs.append(true / total_targets)
        fprs.append(false / length_sub_targets)

    return tprs, fprs

if __name__ == '__main__':
    tprs, fprs = home_made_ROC([0.1, 0.4, 0.35, 0.8], [0, 0, 1, 1])
    print tprs
    print fprs


Calculating int next power of two Here is one of the possible solutions:

  public static int getNextPowerOfTwo(int value) {
    int result = value;
    result -= 1;
    result |= result >> 16;
    result |= result >> 8;
    result |= result >> 4;
    result |= result >> 2;
    result |= result >> 1;
    return result + 1;
}

As in your opinion is it good enough or may you suggest better one?

Java has Integer.highestOneBit that can make this a bit simpler:

int highestOneBit = Integer.highestOneBit(value);
if (value == highestOneBit) {
    return value;
}
return highestOneBit << 1;