Google Maps API - reading locations from file and displaying markers on map I'm working with Google Maps and want to read in location objects into the map. I'm loading them in from a .csv file and create Shelter objects for each location that I save in an ArrayList. To actually add them to the map I iterate through this ArrayList and create marker objects with each object's data. Each actual Marker object created is also saved into another ArrayList so that I can reference them later (show/hide them on map when wanted). I feel like I'm doing something wrong. I included the relevant methods below.

private ArrayList<ShelterObject> shelterObjects;
private ArrayList<Marker> shelterMarkers;

// Generates the Shelter objects from the csv file and adds them to the  ArrayList.
private void generateShelterObjects()
{
    shelterObjects = new ArrayList<>();

    Log.i("SHELTER_TEST", "METHOD generateShelterObjects() STARTED");

    InputStream is = getResources().openRawResource(R.raw.shelters_csv_file);
    BufferedReader reader = new BufferedReader(
            new InputStreamReader(is, Charset.forName("UTF-8")));
    String line = "";

    try {
        while ((line = reader.readLine()) != null) {
            // Split the line into different tokens (using the comma as a separator).
            String[] tokens = line.split(",");

            String address = tokens[0];
            double latitude = Double.parseDouble(tokens[1]);
            double longitude = Double.parseDouble(tokens[2]);
            int numberOfOccupants = Integer.parseInt(tokens[3]);

            ShelterObject object = new ShelterObject(address, latitude, longitude, numberOfOccupants);
            shelterObjects.add(object);

        }

    } catch (IOException e1) {
        Log.e("MapsActivity2", "Error" + line, e1);
        e1.printStackTrace();
    }
    Log.i("SHELTER_TEST", "The size of the arraylist after reading in objects: " + shelterObjects.size());
}

// Iterates through the ArrayList of Shelter objects and adds a marker with data from that object to the map. The markers are saved in another ArrayList so they can be referenced later.
private void addShelterMarkersToMap()
{
    shelterMarkers = new ArrayList<>();

    for (ShelterObject obj : shelterObjects) {
        LatLng latLng = new LatLng(obj.getLatitude(), obj.getLongitude());
        Marker marker = mMap.addMarker(new MarkerOptions()
                .position(latLng)
                .title(obj.getAddress())
                .snippet("Antal platser: " + obj.getNumberOfOccupants()));
        shelterMarkers.add(marker);
    }
}

Your Marker and ShelterObject instances have the same data inside. (except the additional hardcoded (!) string "Antal platser:", whatever that means) Now you have two islands of data. You collect them in two different lists, shelterObjects and shelterMarkers. That is bad, High memory usage and difficult modifications are the drawback. If you want to add/delete/modify one you have to to it in two lists and keep it synchronized. I recommend to only have one island of data (ShelterObjects-array) and whenever you need ShelterMarkers, you convert them. For example you can add a method getAsMarker() to your ShelterObject class that returns itself as Marker. If you have enough memory and want to save performance you only create them when needed and store them for later re-use(see Lazy Instantiation), like so

class ShelterObject {
   ...
   int Marker marker;
   public Marker getAsMarker(... mMap) { // or pass mMap in constructor.
      if (marker != null) {
         return marker;
      }
      LatLng geoPosition = new LatLng(this.getLatitude(), this.getLongitude());
      marker = mMap.addMarker(...); // creates marker via MarkerOptions
      return marker;
   }
   ...
}

What is "LatLng"? You barely can speak that out. If you mean just a pair of latitude and longitude coordinates, then just name it like that: geoPosition, coordinates, latitudeAndLongitude. Try to use short and meaningful names, but no abbreviations. In old times, even Microsoft used code abbreviations like "lptcstr" (google it). Because of bad experience, this is discouraged now. I can't count how many times I have looked up which combination of manager and number the managerNumber was abbreviated: "num", "nbr", "no", "numero", "man", "mgr", "mangr" etc.
Delete the code line below. You overwrite it right away, so you can leave it out for less code clutter.

String line = "";
You forgot to close your stream in a finally-statement of your try-block. This is a severe bug since it's hard to detect when files remain open and suddenly your program crashes when it runs out of file handles. Alternatively you can use a try-with-resources statement.
Avoid using member variables like they are global. Just pass them as parameters. You can better test it and make your code more stable.
I will give you an example what I mean:
Assume you have following lines inside your init-method:

generateShelterObjects();
addShelterMarkersToMap();

Now somebody changes it to following code that crashes or does not add the shelterMarkers:

addShelterMarkersToMap();
generateShelterObjects();

Does he know that he is not allowed to swap the lines when he needs to refactor?
Maybe he does it by accident?

You can avoid that by coding:

ArrayList<ShelterObject> newShelterObjects = generateShelterObjects();
ArrayList<Markers> newShelterMarkers = addShelterMarkersToMap(shelterObjects);
shelterMarkers = newShelterMarkers;
shelterObjects = newShelterObjects;

Now when you swap the first two lines, it will not compile anymore. Also there is no possibility that you can have a nullPointerException in addShelterMarkersToMap() anymore. (I assume that you return an empty Map instead of null in generateShelterObjects() or throw an exception there).
You also cannot have inconsistent data anymore. Just imagine in generateShelterObjects() your map is half-filled when an error occurs. Or your markers are half created when an error occurs. You need to catch the exception and clean both maps. Having inconsistent data is worse than just keeping the old, consistent ones.

Use "/** comment **/" instead of "// comment" on top of your methods, so you can create javadocs easily and the IDE can show it as mouseover-message


Checking that data has been created in multiple tables I have 4 tables that I want to check for data. When there are Rows in all the tables, I want to print a message for success. When there aren't any rows in any table I print an error (when 1 or more table is missing data), I want to warn (unless they are all missing data).

The following script does exactly what I want but I think it could be done in a clearer, more succinct manner.

USE STG_RM_Connector
GO

DECLARE @tblAttCode sysname, @tblAttSess sysname, @tblStaffServ sysname, @tblStuSer sysname
DECLARE @rowCntAttCode int,  @rowCntAttSess int, @rowCntStaffServ int, @rowCntStuSer int

SET @tblAttSess = 'Attendance Session Pupil Data'
SET @tblAttCode = 'Attendace Code Service Attendace Codes'
SET @tblStaffServ = 'Staff Service Staff Members'
SET @tblStuSer = 'Student Service Pupil Data'

SELECT @rowCntAttCode = SUM(PART.rows)
FROM sys.tables TBL
INNER JOIN sys.partitions PART ON TBL.object_id = PART.object_id
INNER JOIN sys.indexes IDX ON PART.object_id = IDX.object_id
AND PART.index_id = IDX.index_id
WHERE TBL.name = @tblAttCode
AND IDX.index_id < 2
GROUP BY TBL.object_id, TBL.name;

SELECT @rowCntAttSess = SUM(PART.rows)
FROM sys.tables TBL
INNER JOIN sys.partitions PART ON TBL.object_id = PART.object_id
INNER JOIN sys.indexes IDX ON PART.object_id = IDX.object_id
AND PART.index_id = IDX.index_id
WHERE TBL.name = @tblAttSess
AND IDX.index_id < 2
GROUP BY TBL.object_id, TBL.name;

SELECT @rowCntStaffServ = SUM(PART.rows)
FROM sys.tables TBL
INNER JOIN sys.partitions PART ON TBL.object_id = PART.object_id
INNER JOIN sys.indexes IDX ON PART.object_id = IDX.object_id
AND PART.index_id = IDX.index_id
WHERE TBL.name = @tblStaffServ
AND IDX.index_id < 2
GROUP BY TBL.object_id, TBL.name;

SELECT @rowCntStuSer = SUM(PART.rows)
FROM sys.tables TBL
INNER JOIN sys.partitions PART ON TBL.object_id = PART.object_id
INNER JOIN sys.indexes IDX ON PART.object_id = IDX.object_id
AND PART.index_id = IDX.index_id
WHERE TBL.name = @tblStuSer
AND IDX.index_id < 2
GROUP BY TBL.object_id, TBL.name;

IF @rowCntAttCode = 0 AND @rowCntAttSess = 0 AND @rowCntStaffServ = 0 AND @rowCntStuSer = 0
BEGIN
 RAISERROR ('Failed to pull data for all tables', 16, 1)
END
ELSE IF @rowCntAttCode = 0 OR @rowCntAttSess = 0 OR @rowCntStaffServ = 0 OR @rowCntStuSer = 0
    BEGIN
        RAISERROR ('Failed to pull data for one of the RM staging tables', 10, 1)
    END
ELSE
BEGIN
  PRINT 'Successfully pulled data for all RM staging tables'
END
GO

As I mention in this other SQL answer:

  Any SQL query has up to eight primary clauses.  WITH, SELECT, INTO, FROM, WHERE, GROUP BY, HAVING, and ORDER BY. Everything else is a part of one of these clauses. These 8 clauses should have zero indentation relative to the query as a whole, and everything else should be indented by at least one level.

A little indentation can go a very long way to making your query far more readable.

So, just using your first SELECT as an example, proper indentation would look something like this:

SELECT @rowCntAttCode = SUM(PART.rows)
FROM sys.tables TBL
    INNER JOIN sys.partitions PART ON TBL.object_id = PART.object_id
    INNER JOIN sys.indexes IDX ON PART.object_id = IDX.object_id AND PART.index_id = IDX.index_id
WHERE TBL.name = @tblAttCode AND IDX.index_id < 2
GROUP BY TBL.object_id, TBL.name;

If you insist on turning any of these single lines into multiple lines to reduce the width, then the extra lines should get an extra level of indentation.  For example

SELECT @rowCntAttCode = SUM(PART.rows)
FROM sys.tables TBL
    INNER JOIN sys.partitions PART ON TBL.object_id = PART.object_id
    INNER JOIN sys.indexes IDX ON PART.object_id = IDX.object_id
        AND PART.index_id = IDX.index_id
WHERE TBL.name = @tblAttCode
    AND IDX.index_id < 2
GROUP BY TBL.object_id, TBL.name;

And some people like breaking the ON part of a join onto its own line, which should look like this:

SELECT @rowCntAttCode = SUM(PART.rows)
FROM sys.tables TBL
    INNER JOIN sys.partitions PART
        ON TBL.object_id = PART.object_id
    INNER JOIN sys.indexes IDX
        ON PART.object_id = IDX.object_id
            AND PART.index_id = IDX.index_id
WHERE TBL.name = @tblAttCode
    AND IDX.index_id < 2
GROUP BY TBL.object_id, TBL.name;

Any of these three indention styles are fine, but be consistent, no matter what you pick.  Hopefully, you can see how this sort of indention style helps guide the reader's eye to the important parts of the query.

Now, with the indentation out of the way... I'm a little concerned about how exactly your query is even working.

Normally, when we're doing a variable assignment in a select, we want to use TOP 1 just to be certain we're only getting one row back.  Given that you're using an aggregation query, you don't necessarily need to do this... but why are you using a GROUP BY?  What sort of results do you expect?  The TBL.name is redundant in the GROUP BY since you already have a WHERE TBL.name = @tblAttCode, so there can only be one value in that column anyway.  What are you hoping to accomplish with the GROUP BY TBL.object_id?


VBA Data Conversion (random printout -> CSV file) I have reviewed the FAQ and I believed it was yes to all 5 q's of "What questions are on-topic for this site?"

Basically, the code below is used to perform the operation shown in the picture. I'm wondering what I can improve in the attached code to clean it up for future reference. Thanks in advance!

Sub Button1_Click()

Dim orgFilename As String
Dim temp As String
Dim strarray(3) As String
Dim vert(4) As String
Dim vert2(3) As String
Dim newFilename As String
Dim numRows As Integer
Dim i As Integer
Dim j As Integer
Dim k As Integer
Dim segCount As Integer
Dim vertex(3, 100) As Double

Dim oldwb As Workbook
Dim newwb As Workbook

orgFilename = Application.GetOpenFilename(FileFilter:="All files (*.), *.", Title:="Please select a file")
If orgFilename = "False" Then Exit Sub
Workbooks.OpenText Filename:=orgFilename, _
            Origin:=950, StartRow:=1, DataType:=xlDelimited, TextQualifier:= _
            xlDoubleQuote, ConsecutiveDelimiter:=True, Tab:=True, Semicolon:=False, _
            Comma:=False, Space:=True, Other:=False, FieldInfo:=Array(Array(1, 1), _
            Array(2, 1), Array(3, 1)), TrailingMinusNumbers:=True
Set oldwb = ActiveWorkbook
Set newwb = Workbooks.Add

oldwb.Activate
Cells(5, 1).Select
numRows = Cells(5, 1).End(xlDown).Row

' Parse through data
segCount = 0
j = 1
For i = 5 To numRows
    If Cells(i, 1) <> "VRTX" And segCount <> 0 Then
        For k = 1 To segCount - 1
            newwb.Worksheets("Sheet1").Cells(j, 1) = "GLINE"
            With newwb.Worksheets("Sheet1")
                .Cells(j, 2) = vertex(1, k)
                .Cells(j, 3) = vertex(3, k)
                .Cells(j, 4) = vertex(2, k)
                .Cells(j, 5) = vertex(1, k + 1)
                .Cells(j, 6) = vertex(3, k + 1)
                .Cells(j, 7) = vertex(2, k + 1)
            End With
            j = j + 1
        Next k
        segCount = 0
    ElseIf Cells(i, 1) = "VRTX" Then
        ' Save vertices to save an endpoint
        vertex(1, segCount + 1) = Cells(i, 3)
        vertex(2, segCount + 1) = Cells(i, 4)
        vertex(3, segCount + 1) = Cells(i, 5)
        segCount = segCount + 1
    End If
Next i

' Save as a new file
temp = Mid$(orgFilename, InStrRev(orgFilename, "\") + 1)
temp = Replace$(temp, ".pl", ".csv")
strarray(1) = Left(orgFilename, InStrRev(orgFilename, "\"))
strarray(2) = "processed_"
strarray(3) = temp
newFilename = Join(strarray, "")
newwb.SaveAs Filename:=newFilename, _
            FileFormat:=xlCSV, _
            Password:="", WriteResPassword:="", ReadOnlyRecommended:=False, _
            CreateBackup:=False
End Sub

Your code does not look bad overall. Here are my 2 cents:

To improve readability, I would split your procedure in sub procedures / functions (does not make a difference if this is the only function in the module, but makes a difference in larger projects). The main procedure could look like this for example (simplified):

Sub main()
  openPlFile
  readPlFile
  writeCsvFile
  saveCsvFile
End Sub

You could add some error handling logic:

Right after your declarations:

On Error GoTo error_handler

And at the end of the code:

Exit Sub
error_handler:
  'code to handle the error for example:
  MsgBox "There was an error: " & Err.Description
End Sub

If performance is an issue there are a few steps to improve the code:

a. You can turn off ScreenUpdating and turn Calculation to manual (don't forget to turn them back on before exiting AND in the error_handler block if there is one).

b. If you want to improve performance further, you could use arrays instead of reading from / writing to the spreadsheet directly in a loop (see http://www.avdf.com/apr98/art_ot003.html)

c. If you want to improve performance even more, you could read and write the files directly without opening / creating workbooks.

d. I generally avoid using Integer as they use the same memory space as Longs anyway (on 32 or 64 bit systems) - VBA simply applies different overflow rules. Declaring Longs instead of Integers can result in a slight performance gain (not so much in your case, but can be useful when declared in a loop for example)


Shrinking header on scroll I have the following jQuery which applies a class to my fixed header to shrink it when the user is scrolled any distance down the page:

$(window).scroll(function() {
if ($(this).scrollTop() > 1){
    $('div.single-level-nav').addClass("shrunk");
  }
  else{
    $('div.single-level-nav').removeClass("shrunk");
  }
});

This is working, but of course is constantly firing whenever the user is scrolling. This seems a bit of overkill, and not a great use of resources.

What I have may be the best way of achieving what I want, but I was wondering if there was a better way.

// init your element
var $nav = $('div.single-level-nav');
// use a boolean var to check if the element is already shrinked
var navShrinked = false;

$(window).on('scroll', function() {
  if ($(this).scrollTop() > 1){
    // add class only once
    if(!navShrinked) {
      $nav.addClass("shrunk");
      navShrinked = true;
    }
  }
  else{
    // remove class only once
    if(navShrinked) {
      $nav.removeClass("shrunk");
      navShrinked = false;
    }
  }
});
body {
  height: 1500px;
}
.single-level-nav {
  height: 100px;
  width: 100%;
  position: fixed;
  top: 0;
  left: 0;
  background-color: #000;
  transition: height .3s;
}
.single-level-nav.shrunk {
  height: 50px;
}
<!DOCTYPE html>

  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width">
  <title></title>


Compress and decompress a string from a file I need to compress some text, I'm wondering if my program could be made more efficient. The only things I could think of would be the 'import re' and turning the filename input and read part as a function.

import re
from ast import literal_eval

################## Compression #####################
def comp():
    while True:
        try:
            fileName=input('Please input text file name in current directory: ')+'.txt'
            a=open(fileName)
            break
        except:
            print('No Such text file in current directory name: '+fileName)

    content = a.read()
    a.close()

    p = re.compile(r'[\w]+|[\W]')
    split = p.findall(content)

    b = []
    wordList = []

    for word in split:
        try:
            r = wordList.index(word) + 1
        except ValueError:
            wordList.append(word)
            r = len(wordList)
        b.append(r)

    f=open('compressed.txt', 'w')
    f=open('compressed.txt', 'r+')
    f.write(str(wordList)+'\n'+str(b))
    f.close()

####################################################

################## De-Compression ##################

def decomp():
    while True:
        try:
            fileName=input('Please input text file name in current directory: ')+'.txt'
            a=open(fileName)
            break
        except:
            print('No Such text file in current directory name: '+fileName)

    words = literal_eval(a.readline().rstrip('\n'))
    pos = literal_eval(a.readline())

    temp = []

    for index in pos:
        temp.append(words[index-1])
    sentence = ''.join(temp)
    print(sentence)
####################################################

As you said, not much to do, I only defined the file read as a function, used the with context manager to read and write files instead of opening and closing, some PEP8 enhancements and string.format() instead of concatenation.

import re
import os
from ast import literal_eval

def get_filename():
    while True:
        filename = input('Please input text file name in current directory: \n')
        filename = filename if filename.endswith('.txt') else '{}.txt'.format(filename)
        if os.path.isfile(filename):
            return filename
        else:
            print('No Such text file in current directory: {}'.format(filename))

def comp():
    with open(get_filename(), 'rb') as file_in:
        p = re.compile(r'[\w]+|[\W]')
        split = p.findall(file_in.read())

    word_list, b = []
    for word in split:
        try:
            r = word_list.index(word) + 1
        except ValueError:
            word_list.append(word)
            r = len(word_list)
        b.append(r)

        with open('compressed.txt', 'r+') as file_out:
            file_out.write('{}\n{}'.format(str(word_list), str(b)))

def decomp():
    with open(get_filename(), 'rb') as file_in:
        words = literal_eval(file_in.readline().rstrip('\n'))
        pos = literal_eval(file_in.readline())

    temp = []

    for index in pos:
        temp.append(words[index-1])
    print(''.join(temp))

I might help some more, but need some more context on what you are trying to accomplish. Maybe working on multiple files, taking filenames as arguments or some more improvements.

EDIT:

After reviewing the assignment the previous solution won't take care of punctuation, this will:

import string
import os

def get_filename():
    """Function to get the file name from user input"""
    # Infinite loop to ask the user for a file name that must be valid in order to break the loop.
    while True:
        # Ask the user for a file name.
        filename = input('Please input text file name in current directory: \n')
        # Append .txt extension if not provided.
        filename = filename if filename.endswith('.txt') else '{}.txt'.format(filename)
        # Test if the file exist and return it, print an error and continue loop if not.
        if os.path.isfile(filename):
            return filename
        else:
            print('No Such text file in current directory: {}'.format(filename))

def comp():
    filename = get_filename()
    with open(filename, 'r') as file_in:
        full_text = file_in.read()

    # Making a translation table to get rid of punctuation and keep only letters.
    table = str.maketrans(string.punctuation, ' ' * len(string.punctuation))
    only_words = full_text.translate(table)
    # Create a sorted list of unique elements.
    word_list = sorted(set(only_words.split()))
    # Replace every word of the original text by it's index in the word list.
    for index, word in enumerate(word_list):
        full_text = full_text.replace(word, '{{{}}}'.format(index))  # Double brackets are ignored by .format()

    # Create a separate file with the same name but _COMP_, to dump the compressed text.
    with open('_COMP_{}'.format(filename), 'w') as file_out:
        file_out.write('{}\n***{}***'.format(full_text, '|'.join(word_list)))  # The word list reference is appended.

def decomp():
    with open(get_filename(), 'r') as file_in:
        # Read each line to a list element.
        full_text = file_in.readlines()
        # Remove the last line of the text and convert it to a sorted list of the reference words.
        reference = sorted(full_text.pop(-1).strip('*').split('|'))
    # Print the full text replacing the indexes by the corresponding word in the list.
    print(''.join(full_text).format(*reference))


Screenshot-scaling code Please review for readability.

/*
Takes a screenshot of the current tab, then scales it to width and height.
Returns null if the tab is smaller than width and height.
Crops the screenshot to maintain aspect ratio.
Also crops the screenshot to avoid scaling down by more maximumScaleFactor.
*/
var GetCurrentScreenshot = function (width, height, maximumScaleFactor) {
    return new RSVP.Promise(function (resolve) {
        chrome.tabs.captureVisibleTab(null, { format: 'png' }, function (dataURL) {
            var sourceImage = new Image();
            sourceImage.onload = function () {
                var canvas = document.createElement("canvas");
                var cropWidth = sourceImage.width;
                var cropHeight = sourceImage.height;
                if (cropWidth < width || cropHeight < height) {
                    resolve(null);
                    return;
                }
                var outputRatio = width / height;
                var inputRatio = cropWidth / cropHeight;
                if (outputRatio > inputRatio) {
                    cropHeight = cropWidth / outputRatio;
                } else {
                    cropWidth = cropHeight * outputRatio;
                }

                //This should be impossible
                if (cropWidth > sourceImage.width) cropWidth = sourceImage.width;
                if (cropHeight > sourceImage.height) cropHeight = sourceImage.height;

                if (cropWidth > width * maximumScaleFactor) {
                    cropHeight = cropHeight / (cropWidth / (width * maximumScaleFactor));
                    cropWidth = width * maximumScaleFactor;
                }

                canvas.width = width;
                canvas.height = height;
                canvas.getContext("2d").drawImage(sourceImage, 0, 0, cropWidth,
                    cropHeight, 0, 0, width, height);
                resolve(canvas.toDataURL());
            };
            sourceImage.src = dataURL;
        });
    });
};

Breaking out the callback function

I would break out that function as well, though I would give it a better name than func, mayhaps drawScaledScreenshot?

Functionality

CR is not the best place to discuss functionality, but why would you not draw the image if the tab size is too small, it does not seem to make sense.
Also, why would you only check for width going over the maximumScaleFactor ? I would check height as well.

Scope

I feel the following vars could have been declared outside of the drawScaledScreenshot function, since they only depend on width and height.

var outputRatio      = width / height;
var maximumCropWidth = width * maximumScaleFactor; //You calc this several times

Paranoia

If you truly think it is impossible, then it should not be in the code

//This should be impossible
if (cropWidth > sourceImage.width) cropWidth = sourceImage.width;
if (cropHeight > sourceImage.height) cropHeight = sourceImage.height;

If you think it is possible, then I would suggest you convert this to the more readble

//This should not be needed
cropWidth  = Math.min( cropWidth,  sourceImage.width);
cropHeight = Math.min( cropHeight, sourceImage.height);

Notice that if width * maximumScaleFactor is exceeded, you can still get a cropHeight > sourceImage.height..

Furtermore, I would be more paranoid that the caller forgets to provide maximumScaleFactor and would add a

maximumScaleFactor = maximumScaleFactor || 10; //Or whatever is reasonable

Finally, since you are dividing by height, I would verify that height is not zero.


Simple mutex and conditional variable signal in C I'm new to multithreading in C so I made a toy program that uses a mutex and a conditional variable to communicate between two threads. do_work performs a task every 1 second (task could take longer than 1 second). Is this implementation free from deadlocks and race conditions? Anything else I might be missing?

#include <pthread.h>
#include "errors.h"

pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
int predicate_value = 0;

void *do_work(void *work) {
    int status = 0;
    int i = 0;
    while (1) {
        status = pthread_mutex_lock(&mutex);
        if (status != 0) {
            err_abort(status, "Lock mutex");
        }
        while (predicate_value == 0) {
            printf("%s\n", "waiting");
            status = pthread_cond_wait(&cond, &mutex);
            if (status != 0) {
                err_abort(status, "Wait on condition");
            }
        }
        if (predicate_value != 0) {
            printf("doing some work: %d\n", i);
            ++i;
            predicate_value = 0;
        }
        status = pthread_mutex_unlock(&mutex);
        if (status != 0) {
            err_abort(status, "Unlock mutex");
        }
    }
}

int main () {
    int status = 0;
    pthread_t work_thread_id;
    status = pthread_create(&work_thread_id, NULL, do_work, NULL);
    if (status != 0) {
        err_abort(status, "Create work thread");
    }
    while (1) {
        if (predicate_value == 0) {
            status = pthread_mutex_lock(&mutex);
            if (status != 0) {
                err_abort(status, "Lock mutex main");
            }
            printf("%s\n", "changed value");
            predicate_value = 1;
            status = pthread_cond_signal(&cond);
            if (status != 0) {
                err_abort(status, "signal condition");
            }
            status = pthread_mutex_unlock(&mutex);
            if (status != 0) {
                err_abort(status, "Unlock mutex main");
            }
            printf("%s\n", "time to sleep");
        }
        sleep(1);
    }
}

Needs volatile keyword
The way your program is written, predicate_value needs to be a volatile int instead of a normal int.  This is because you are reading it without locking the mutex in main(), and writing to it in do_work() from another thread.
Another way to avoid the problem is to ensure that you only ever read the variable with the mutex locked.


Project Euler Problem 1 This is my python solution to the first problem on Project Euler:

n = 1
rn = 0
while n < 1000:
    if n%3 == 0 or n%5 == 0:
        rn += n
    n = n + 1
print(rn)

I would like to find a way to keep everything in this python code to as little number of lines as possible (maybe even a one liner??), and possibly improve the speed (it's currently around 12 ms). By the way, this is the problem: If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3, 5, 6 and 9. The sum of these multiples is 23.

Find the sum of all the multiples of 3 or 5 below 1000.Suggestions?Thanks.

Python hint number 1:

The pythonic way to do :

n = 1
while n < 1000:
    # something using n
    n = n + 1

is :

for n in range(1,1000):
    # something using n

Python hint number 2:

You could make your code a one-liner by using list comprehension/generators :

print sum(n for n in range(1,1000) if (n%3==0 or n%5==0))

Your code works fine but if instead of 1000, it was a much bigger number, the computation would take much longer. A bit of math would make this more more efficient.

Math hint number 1 :

The sum of all the multiples of 3 or 5 below 1000 is really the sum of (the sum of all the multiples of 3 below 1000) plus (the sum of all the multiples of 5 below 1000) minus the numbers you've counted twice.

Math hint number 2 :

The number you've counted twice are the multiple of 15.

Math hint number 3 :

The sum of the multiple of 3 (or 5 or 15) below 1000 is the sum of an arithmetic progression.


Mapping a product its corresponding image file I'm writing this method that elaborates an image name..

I think my logic is not being the best one here but I'm not very savvy in programming yet.

Right now it looks like this:

if($this->isCustom($product)){
            $image_path = PRODUCT_IMAGE_PATH . $this->order['order_number'] . $index . '.jpg';
            $index++;
        }
        else{
            $image_path = PRODUCT_IMAGE_PATH . $product['sku'] . '.jpg';
        }

        if($this->checkRemoteImage($image_path)){
            return $image_path;
        }

        else{
            if (!$this->isCustom($product)){
                $this->notifyOnNonCustoms($image_path);
            }
            return "No Image";
        }

I believe something like this would be better:

if ($this->isCustom($product)){
            $image_path = PRODUCT_IMAGE_PATH . $this->order['order_number'] . $index . '.jpg';
            $index++;

            if (!$this->checkRemoteImage($image_path)){
                return "No Image";
            }
            return $image_path;
        }

        else{
            $image_path = PRODUCT_IMAGE_PATH . $product['sku'] . '.jpg';

            if (!$this->checkRemoteImage($image_path)){
                $this->notifyOnNonCustoms($image_path);
                return "No Image";
            }
            return $image_path;
        }
    }

The flow is meant to be like this:

If the product is custom, the image path should be the path + order number + ascending numbers ( using outer index here )

If not, it should be the image path + the product's sku

There should be a check if the image does exist in the server. If it doesn't it should return "No Image" and if it's not custom, call the method notifyOnNonCustoms I'm not sure on the best way to structure this.

I am going to shorten up some of your code a little bit,

if ($this->isCustom($product)){
            $image_path = PRODUCT_IMAGE_PATH . $this->order['order_number'] . $index . '.jpg';
            $index++;

            if (!$this->checkRemoteImage($image_path)){
                return "No Image";
            }
            return $image_path;
        }

        else{
            $image_path = PRODUCT_IMAGE_PATH . $product['sku'] . '.jpg';

            if (!$this->checkRemoteImage($image_path)){
                $this->notifyOnNonCustoms($image_path);
                return "No Image";
            }
            return $image_path;
        }
    }

you should allow your image path variable to live a little longer and use it to return once.

this is kind of what I was thinking

if ($this->isCustom($product)){
    $image_path = PRODUCT_IMAGE_PATH . $this->order['order_number'] . $index . '.jpg';
    $index++;

    $image_path = $this->checkRemoteImage($image_path) ? "No Image" : $image_path;
}
else{
    $image_path = PRODUCT_IMAGE_PATH . $product['sku'] . '.jpg';

    if (!$this->checkRemoteImage($image_path)){
        $this->notifyOnNonCustoms($image_path);
        $image_path = "No Image";
    }
}
return $image_path;

a little less indentation inside the if statement block.  I couldn't do much with the else statement block.

Your first set of code is nice in that it creates the path and checks it, but it evaluates

$this->isCustom($product)

twice in some circumstances, and you don't want that because it adds more operations to your algorithm.

Your second set of code is much more precise, it still only creates the $image_path once, but does it differently depending on the value of the truthy statement.  also you have different operations that need to be done for custom and not custom if the image path doesn't exist.

Your second block of code is better than the first.


Computing the square root of a 64-bit integer When sieving primes up to some 64-bit number n I need to determine an upper bound for the greatest potential factor of n, which is the greatest integer that is not greater than the (true) square root of n.

Computing this as std::sqrt(double(n)) is fraught with pitfalls. The standard IEEE double has only 53 bits in its significand, which means that some rounding is often inevitable. One consequence is that the result cannot safely be converted to uint32_t even though the true result could never exceed 32 bits. For example, sqrt(double(uint64_t(0) - 1)) usually rounds up to \$2^{32}\$ and if that is cast to uint32_t then the result becomes 0.

Currently I'm using a function like this:

uint32_t max_factor32 (uint64_t n)
{
   double r = std::sqrt(double(n));

   if (r < UINT32_MAX)
   {
      uint32_t r32 = uint32_t(r);

      return r32 - (uint64_t(r32) * r32 > n);
   }

   return UINT32_MAX;
}

Obviously, this works only if the value returned by sqrt() is reasonably close to the true result, so that the cast to integer yields either the desired result or a number that is exactly one greater.

I think this code should be reasonably safe, even in the face of floating-point hardware/software operating with unknown rounding modes and at an unknown level of strictness. But is that truly so?

Different rounding modes should not affect the correctness of the function, but what about different compiler settings for optimisation and floating point strictness? It seems to me that normally a compiler should only err on the side of excess precision by not rounding intermediate results to IEEE double, in which case my code should work fine as it is. Can different compiler switches throw a monkey wrench into my scheme or is the code safely portable to worlds and compilers unknown?

P.S.: what I want to avoid is the 'tail wagging the dog' style caveats like "this code works only with a strictly-conforming compiler and with strict floating-point precision enabled". On the other hand, if the function max_factor32() were to mess up quietly then the result of the containing program would likely be wrong, and even crashes are possible in cases like the above example where 0 is returned when the true result would be UINT32_MAX.

Usually you can just test n < r*r instead which also has the benefit of being much cheaper than a sqrt. You do need to handle potential integer overflow for n > 264 - 2*232 + 1.

You can't hoist r*r out of the loop the way you can hoist sqrt(n) out but integer multiplication is cheap on any modern processor.


Ruby code to identify 2-byte characters I just wrote this fuzzy snippet to identify any 2-byte character in my data file which assumes only 1-byte characters (ANSI). Please review and suggest me any better solution!

File.open('data.txt', 'r') do |f|
    line_number = 0
    while line = f.gets
        line_number = line_number + 1
        line.each_byte do |c|
            if c & 0xff80 != 0
                puts 'A non-ascii character is found at line ' + line_number.to_s
                break
            end
        end
    end
end

File.open('data.txt', 'r') do |f| ... end

Good job. This is indeed the preferred way to read a file in Ruby. You get to read the file as a stream, and the file gets closed automatically. One thing to note is that the r flag is the default and can be left out. It is fine to leave it there for clarity, however.

Don't forget to set the correct encoding. If the real encoding is compatible with low-ASCII, you can assume ASCII-8BIT but it will treat characters in multibyte encodings as multiple characters. See later, however. You can set a default one, or you can do it while opening the file with :external_encoding => ....

If you strive for simplicity rather than speed (or if the file is small enough to be read all at once), you can also load the file into memory at once, then iterate over it:

f = IO.read('data.txt')

line_number = 0
while line = f.gets
    line_number = line_number + 1
    ...
end

Apart from the fact that assignment within conditional is frowned upon in many languages because it can be confused with accidental assignment instead of comparison,

File inherits several iterators from IO (don't forget to check for inherited methods - from superclasses AND modules) when looking up documentation). One of them IO#each_line, formerly IO#lines. For the line number, you can use IO#lineno. You can also use f.each_line.with_index{|line, index|...}, but that is zero-based.

line.each_byte do |c|
    if c & 0xff80 != 0
        puts 'A non-ascii character is found at line ' + line_number.to_s
        break
    end
end

I think you're looking for String#each_codepoint instead of String#each_byte. A UTF-16 non-ASCII character could well be composed of two ASCII bytes. UTF-8 non-ASCII characters are readily composed of multiple bytes, so you'd be getting multiple reports per character. If you assume a single-byte encoding, both will do (and each_byte is more explicit), but ...

If your intention is to test single-byte encodings, then the mask 0x80 will suffice. If your intention is to support any encoding of the Unicode character set, then the mask 0xff80 will not suffice as Unicode code points are 20 bits long. Some rare characters actually lie in the U+10000 .. U+1007F region. Perhaps the surest test is to use > 0x7F

Better yet, realise you are working with strings, and use string methods to manipulate them.

Even if you use each_byte, you still have to specify the external encoding. If the external encoding is set to UTF-8 but the source file isn't, you'll get an exception. That being said, you could set a 7-bit encoding (US-ASCII) and rely on the exception being thrown, but that won't get you the line number the character occurs on.

While string concatenation is the way to go in languages that lack string interpolation, string interpolation is the preferred way in Ruby. The nice thing is that it performs automatic string conversion where concatenation requires you to convert manually. Just don't use huge blocks or nested interpolation inside unless you care for character count rather than readability. Compute the part first, then interpolate it in.

One last thing to note is that you are not very consistent in the use of one-letter / descriptive variable names. Descriptive names never hurt, and single-letter variables should only be used if their scope is small enough (or if the single letter is descriptive per se).

This is how I would modify your code:

File.open('data.txt') do |file|
    file.each_line.with_index do |line, index|
        if /[^\x00-\x7F]/ =~ line
            puts "A non-ascii character is found at line #{index + 1}"
            break
        end
    end
end

If you are not afraid of perlisms,

File.open('data.txt') do |f|
    while f.gets
        if /[^\x00-\x7F]/
            puts "A non-ascii character is found at line #{$.}"
            break
        end
    end
end


Counting paths in an n-dimensional grid that stay within the boundaries Problem Statement

  You are situated in an N dimensional grid at position (x1, x2, …, xN).
  The dimensions of the grid are (D1, D2, …, DN). In one step, you can
  walk one step ahead or behind in any one of the N dimensions. (So
  there are always 2×N possible different moves). In how many ways can
  you take M steps such that you do not leave the grid at any point? You
  leave the grid if at any point xi, either xi ≤ 0 or xi > Di.

  Input Format

  The first line contains the number of test cases T. T test cases
  follow. For each test case, the first line contains N and M, the
  second line contains x1, x2, …, xN and the 3rd line contains D1, D2, …, DN.

  Output Format

  Output T lines, one corresponding to each test case. Since the answer
  can be really huge, output it modulo 1000000007.

  Constraints

  \$1 \le T \le 10\$

  \$1 \le N \le 10\$

  \$1 \le M \le 300\$

  \$1 \le D_i \le 100\$

  \$1 \le x_i \le D_i\$

  Sample Input

1
2 3
1 1
2 3

  Sample Output

12

Code

I have applied dynamic programming, but am exceeding the time limit in most cases. How can I make it faster? I am also eager to know if there is any other method to memoize because these types of grid problems are asked more often.

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.Arrays;
import java.util.HashMap;

public class Grid_Walking {
    private static String[] n;
    private static int moves;

    private static HashMap<String, Integer> hm;
    private static BufferedReader br;

    private static String[] s;

    private static int dimen;
    private static int[] present;
    private static int[] dimlen;

    public static void main(String args[]) throws IOException {

        br = new BufferedReader(new InputStreamReader(System.in));
        int t = Integer.parseInt(br.readLine());
        while (t > 0) {
            n = br.readLine().split(" ");
            dimen = Integer.parseInt(n[0]);
            present = new int[dimen];
            dimlen = new int[dimen];
            moves = Integer.parseInt(n[1]);

            s = br.readLine().split(" ");
            for (int i = 0; i < dimen; i++) {
                present[i] = Integer.parseInt(s[i]) - 1;
            }
            s = br.readLine().split(" ");
            for (int i = 0; i < dimen; i++) {
                dimlen[i] = Integer.parseInt(s[i]);
            }
            hm = new HashMap<String, Integer>();
            System.out.println(solve(present, moves) % 1000000007);
            t--;
        }

    }

    private static int solve(int[] x, int moves) {
        // TODO Auto-generated method stub
        int result = 0;
        String s = Arrays.toString(x) + moves;
        if (hm.containsKey(s)) {
            return hm.get(s);
        }
        if (moves == 0) {
            return 1;
        }
        for (int i = 0; i < dimen; i++) {
            // System.out.println("fjfvnfjbv");
            if (x[i] > 0) {
                x[i] = x[i] - 1;
                result = result + solve(x, moves - 1);
                x[i] = x[i] + 1;
            }
            if (x[i] < dimlen[i] - 1) {
                x[i] = x[i] + 1;
                result = result + solve(x, moves - 1);
                x[i] = x[i] - 1;
            }
        }
        hm.put(s, result % 1000000007);
        return result % 1000000007;
    }
}

Naming

The name Grid_Walking is discouraged in Java. You should use GridWalking (without the _). The names hm and br are terrible.

Members

The field br isn't used outside of the main method and you can simply make it local.

Leaking resources

You really should use Try-with-resources to auto-close the readers.

try(InputStreamReader is = new InputStreamReader(System.in);
    BufferedReader br = new BufferedReader(is)){

to make sure resources are properly closed in all scenarios.

Performance

Here's something you can try. Consider this you're taking \$M\$ steps and have \$2N\$ possible ways to go each step. Assuming for a second that the grid is so large that you never exit, then you have \$(2N)^M\$ different ways to step inside the grid. It might be easier to calculate in how many ways you can end up outside the grid...

Better algorithm hints

Lets look at a simple version of the problem see if we can't find some ideas. Let \$N=1\$ and \$M=3\$ and lets draw the states we can be in, specifically lets count in how many ways we can end up in each cell:

We start with step zero, there is only one possible state we can be in. Taking one step we can either go left or right so there is one way we can end up in either cell shown under \$M=1\$. From there we can either take left or right from both cells. So there are two ways we can end up back at the starting position and one way we can reach each edge. Try to do the steps yourself you'll see you'll get the same values.

So by now you see how this is going. The number of ways you can end up in a cell is the sum of the number of ways you can end up in the neighboring cells. For \$M=4\$ this would be 1 0 4 0 6 0 4 0 1.

There are two things to note here: First, the sum of all the values in the cells is actually equal to the possible routes you can take with \$M\$ steps. Take a moment to think about it, it makes sense, this is your answer (after applying suitable modulus). Second, notice anything familiar with the shape of the numbers? Yepp, that's Pascal's triangle with zeros injected between each cell! The answer for \$N=1\$ is the sum of \$M\$th row of pascals triangle after cutting off the edges due to the size of the grid (the zeros wont affect the result)!

So solving this problem for arbitrary \$N\$ you just need to generalize Pascal's triangle to \$N\$ dimensions. You can easily do this by using a hyper-cube and using the above realization that:

  The number of ways you can end up in a cell is the sum of the number of ways you can end up in the neighboring cells.

I will leave the details up to you to figure out, let me know if you need more help.


Promise.all, but with enforced limit on concurrent promises I'm trying to implement parallelLimit() to limit the number of parallel tasks that can be performed at any time. Personally, I'm using this function for limiting the parallel uploads from my code.

const _ = require('lodash');

async function _parallelLimit(tasks, limit) {
  const _tasks = Object.assign([], _.zip(tasks, _.range(tasks.length)));
  const results = [];
  const awaitWorker = async () => {
    while (_tasks.length > 0) {
      const [task, idx] = _tasks.pop();
      results[idx] = await task();
    }
  };
  await Promise.all(_.range(0, limit).map(awaitWorker));
  return results;
}

You can remove Object.assign as _.zip will eventually give you the required array.

Also _.range(0, limit) is equivalent to _.range(limit).


Python implementation of Back Propagation Algorithm without bias This program implements the back propagation algorithm of neural network with an example. Can we make it more efficient?
from setuptools.namespaces import flatten
import numpy as np
import math

inputvalues = [0.1, 0.9, 0.5, 0.2]

initialweights = [[0.1, 0.2], [0.3, 0.4], [0.5, 0.6], [0.7, 0.8]]

hidden_initial_weights = [[[0.2, 0.3, 0.4], [0.5, 0.6, 0.7]],
                          [[0.8, 0.9, 0.1, 0.2], [0.3, 0.4, 0.5, 0.6], [0.7, 0.8, 0.9, 0.1]],
                          [[0.2, 0.3, 0.4, 0.5, 0.6], [0.7, 0.8, 0.9, 0.1, 0.2], [0.3, 0.4, 0.5, 0.6, 0.7], [0.9, 0.8, 0.7, 0.6, 0.5]]]

Hidden_Output_Connection_weights = [[0.5, 0.4, 0.3], [0.6, 0.7, 0.8], [0.1, 0.3, 0.5], [0.7, 0.8, 0.9], [0.45, 0.34, 0.32]]
Target_Value = 0.9
learning_rate = 0.4
weight_matrix = [[initialweights],hidden_initial_weights,[Hidden_Output_Connection_weights]]

# Forward Propagation
intermediate = [inputvalues]   # Intermediate contains output from each layer
for j in range(len(weight_matrix)):
    for l in range(len(weight_matrix[j])):
        result = np.dot(inputvalues,weight_matrix[j][l])
        result1 = [(1-math.exp(-k))/(1+math.exp(-k))for k in result]
        intermediate.append(result1)
        inputvalues = result1

Final_error_from_output_layer_nodes = []
for i in intermediate[-1]:
    Error_in_final_output = (Target_Value - i)*(i)*(1-i)
    Final_error_from_output_layer_nodes.append(Error_in_final_output)

rr = [] # rr is the derivative
for i in reversed(range(len(intermediate)-1)): # subtract one because it is length of output layer
    temp = []
    for j in range(len(intermediate[i])):
        temp.append(intermediate[i][j]*(1-intermediate[i][j]))
    rr.append(temp)

transposed_weight_matrix = [] #transposed_weight_matrix
for i in reversed(range(len(weight_matrix))):
    for j in reversed(range(len(weight_matrix[i]))):
        transposed_weight_matrix.append(list(map(list, zip(* weight_matrix[i][j]))))

Backward_propagation_output = []
final_errors = [Final_error_from_output_layer_nodes]
for i in range(len(transposed_weight_matrix)-1):
    intermediate_error = np.dot(Final_error_from_output_layer_nodes,transposed_weight_matrix[i])
    temp =[]
    for num1, num2 in zip(intermediate_error,rr[i]):
        temp.append(num1 * num2)
    Backward_propagation_output.append(temp)
    Final_error_from_output_layer_nodes = Backward_propagation_output[i]
    final_errors.append(Final_error_from_output_layer_nodes)

final_errors.reverse()

change_in_weights = []
for i in range(len(intermediate)-1):
    temp1 = []
    for j in range(len(intermediate[i])):
        temp = []
        for k in range(len(final_errors[i])):
            temp.append(learning_rate*intermediate[i][j]*final_errors[i][k])
        temp1.append(temp)
    change_in_weights.append(temp1)
# print(change_in_weights)

# Updated weights
updated_weights = []
r = list(flatten(weight_matrix))
for i in range(len(r)):
    temp = []
    for j in range(len(r[i])):
        temp1 = []
        for k in range(len(r[i][j])):
            temp1.append(r[i][j][k]+change_in_weights[i][j][k])
        temp.append(temp1)
    updated_weights.append(temp)

First, I wouldn't rely on flatten from a module like setuptools. Its purpose isn't in line with the interface of the module, so it's likely an "implementation detail". There isn't necessarily any guarantee that it will exist in the future.
Also, if you check its definition, you'll be pointed to a better option:
flatten = itertools.chain.from_iterable

It's simply a alias for from_iterable on itertools.chain.

Please respect Python's naming conventions. Normal variable names should be in snake_case.

I would try and split this up into functions. Even if done simply, a procedure of
def main():
    initialize_net()
    propagate()
    backpropagate_errors()

makes it a lot easier to immediately understand the flow of the program and what code is a part of what step. That would also make it easier to control when the code in the script runs. You don't necessarily want the whole thing to run just because you loaded the file into your IDE.

Sorry, I wish I had the energy to do a good review of this. I remember my first NN from scratch. That was a fun project.


JavaScript jQuery plugin options. Defaults, User Options, and immutable settings pattern? I'm writing a jQuery plugin.

I'm allowing the user to pass options (optionally-- argument 2 to extend) but I also require some defaults (argument 1), and some things user is not allowed to modify (argument 3).

I follow the global override pattern recommended by most jQuery plugins.  But I'm running into something I've never seen discussed which is where you have defaults that need to be set in a specific scope (caption being set to self._tableName, for example).

This forces you to do an extend pretty much everywhere you need options + internal state.  It's not that terrible, but my code review sense is tingling.

Anything in the snippet is up for debate.  Leave your thoughts, please.

For context, how I'm doing options in the constructor

       this.options = $.extend(true, $.fn.myPlugin.Defaults, options);

And here is the argument object I'm creating to be passed to a plugin.  To reiterate, I'm forming this object much later when it's time to create the jqgrid.

       $.extend({ caption: self._tableName },
                self.options ? self.options.jqgrid : {},
                {
                    datatype: function (postdata) {
                        hubProxy.server.requestPage(postdata);
                    },
                    ColNames: ColNames,
                    colModel: ColModel,
                    pager: '#' + pager,
                    ondblClickRow: function (rowid) {
                        $(this).jqGrid('editGridRow', rowid, editTemplate);
                    }
                });

Not a lot ( of context ) to review;

this.options = $.extend(true, $.fn.myPlugin.Defaults, options); <- This will modify the defaults of the plugin, if you did not want that, then you should do

this.options = $.extend(true, {}, $.fn.myPlugin.Defaults, options);

You are talking about passing a third parameter for immutable settings/options/parameters, the 3rd parameter in your $.extend call will do no such thing, you will not even know which properties come from the 3 object.
myPlugin <- as plugin names go, not very original ;)
datatype is probably an unfortunate name, I would expect a function with that name to return a data type, also I would expect it to follow lowerCamelCase ( dataType )
It is risky to not check for jqGrid in self.options ? self.options.jqgrid : {},, I would have gone for (self.options && self.options.jqgrid) ? self.options.jqgrid : {},
Finally, I have to say that the last code block looks like a large amorphous blob, I would have much rather seen chained setters.

All in all, I am sure that you could have gotten more out of codereview if you had posted a bit more code.


Parsing pairs of a number or number range I need to process an array of number pairs. Here are the constraints on the pairs:

The second item may be missing, in this case 0 will be used as second item - - 38 is the same as 38,0
The pair items may be a number, or a range - 149,0, 149,[3,7], [5,9],11, [3,6],[2,9], [4,8]
The range may be inclusive or exclusive - [8,11], [3,9), (4,10]
There may be any number of spaces between items - [ 3 , 6 ]
The second item may be >=0, this is the same as bullet #1 - 36, >=0

Why do I need the ranges? If I have, say, 149,[3,5], then I need to have 3 pairs - 149,3, 149,4, 149,5. If I have [8,10], then the pairs are 8,0, 9,0, 10,0. You get the idea.

I wrote a regex to get the items from the input. I think regex is not the best choice here, maybe I'll change it. Here's my program:

#include <string>
#include <regex>
#include <vector>

using intRange = std::pair<int, int>;
std::vector<std::pair<int, int>> pairs;

intRange getRange(const std::string &value1, const std::string &value2, const std::string &value3)
{
    if (value1.length())
        return intRange(std::stoi(value1), std::stoi(value1));

    if (value2.length() && value3.length())
    {
        const short startIncrement = value2.front() == '(' ? 1 : 0;
        const short endDecrement = value3.back() == ')' ? 1 : 0;

        // remove the parentheses
        std::string val2 = value2, val3 = value3;
        val2.replace(0, 1, std::string{});
        val3.pop_back();

        return intRange(std::stoi(val2) + startIncrement, std::stoi(val3) - endDecrement);
    }

    return {};
}

bool parseRange(const std::string &input)
{
    pairs.clear();

    static const std::regex rx{ R"numrange(\s*(?:(\s*\d+\s*)|([\[\(]\s*\d+)\s*,\s*(\d+\s*[\]\)]))(?:,\s*(?:(\d+\s*)|\s*([\[\(]\d+)\s*,\s*(\d+\s*[\]\)])|(\s*>\s*=\s*0\s*)))?\s*)numrange" };

    std::smatch matches;
    if (std::regex_match(input, matches, rx))
    {
        // if 1st (4th) match group exists, first (second) item is a single number
        // if 2nd and 3rd (5th and 6th) match groups exist, first (second) item is a range
        // if neither 4th nor 5th and 6th groups exist, then second item isn't specified, and default value (0) is used
        auto firstRange = getRange(matches[1], matches[2], matches[3]);
        auto secondRange = getRange(matches[4], matches[5], matches[6]);

        // save all pairs
        for (auto first = firstRange.first; first <= firstRange.second; ++first)
            for (auto second = secondRange.first; second <= secondRange.second; ++second)
                pairs.emplace_back(first, second);
    }
    else
        return false;

    return true;
}

int main()
{
    std::vector<std::string> inputs = {
        "38",
        "149,0",
        "149,[3,7]",
        "[5,9],11",
        "[3,6],[2,9]",
        "[4,8]",
        "[3,9)",
        "(4,10]",
        "[ 3 , 6 ]",
        "36, >= 0"
    };

    for (const auto &input : inputs)
    {
        if (parseRange(input))
        {
            std::cout << "input: " << input << '\n';
            for (const auto &pair : pairs)
                std::cout << pair.first << ", " << pair.second << '\n';
            std::cout << "-------------------\n";
        }
        else
        {
            std::cout << "Error occurred.";
            return -1;
        }
    }

    std::cin.get();
}

I think there is room for improvement for this code. Please let me know where I can improve. Thanks.

Just answering this question since it is unanswered. Hope, it would help:

Since your code is mainly about RegEx, which you'd most likely know that you can use this tool:

Your original RegEx based on an exact match method takes 265 steps and 1ms to match your 10 inputs, which is good.

\s*(?:(\s*\d+\s*)|([\[\(]\s*\d+)\s*,\s*(\d+\s*[\]\)]))(?:,\s*(?:(\d+\s*)|\s*([\[\(]\d+)\s*,\s*(\d+\s*[\]\)])|(\s*>\s*=\s*0\s*)))?\s*

Sometimes, exact match may not be necessary, not sure about your case or if you might have certain boundaries, but maybe you could use a faster RegEx with a lower number of iterations, maybe something similar to this, if you are not looking for an exact match, which is pretty fast and might match your 10 inputs with only 50 steps:

^[0-9\,\[\]\(\) >=]*$

The drawback of this method is that it is based on your input characters, i.e., numbers, [, ], (, ), ,, space and >=.


Checking whether two strokes overlap I have these two functions and I was just wondering if there is any way to shorten the line of code with the many && statements. Shortening of the rest of the code would be cool too, but it's not necessary. I'm trying to learn if there is a better way to accomplish this though.

# Accepts two stroks and determines if they overlap with one another
def overlap? stroke1, stroke2
    min_x = 0
    max_x = 1
    min_y = 2
    max_y = 3

    bounds1 = get_bounds stroke1
    bounds2 = get_bounds stroke2

    return true if (bounds1[max_x] >= bounds2[min_x]) && (bounds1[max_y] >= bounds2[min_y]) && (bounds2[max_x] >= bounds1[min_x]) && (bounds2[max_y] >= bounds1[min_y])

    return false
end

# Returns an array of bounds [x_min, x_max, y_min, y_max] for the stroke
def get_bounds stroke
    # Arrays for storing x and y values for the stroke currently being inspected
    xvals = Array.new
    yvals = Array.new

    stroke.each_slice(3) do |point|
        xvals.push(point[0])
        yvals.push(point[1])
    end

    # A temporary array that stores the min/max x/y for the stroke being inspected
    bounds = Array.new
    bounds << xvals.min << xvals.max << yvals.min << yvals.max

    return bounds
end

I actually did not write this code, but I offered to clean it up and I came upon this and thought that there was probably a better way.

The code in the if is duplicate, so we can extract it to a method that I will call gte (greater than or equal). The maxs and mins values are used at this comparison, so they will be moved to the new method too:

def gte(bounds1, bounds2)
    min_x, max_x, min_y, max_y = 0, 1, 2, 3

    bounds1[max_x] >= bounds2[min_x] && bounds1[max_y] >= bounds2[min_y]
end

The content in the if expression changes to:

gte(bounds1, bounds2) && gte(bounds2, bounds1)

But the if is not necessary because this expression already returns a boolean value, then we will return it directly. The keyword return is not required, then the method overlap? code looks like this:

def overlap?(stroke1, stroke2)
    bounds1 = get_bounds stroke1
    bounds2 = get_bounds stroke2

    gte(bounds1, bounds2) && gte(bounds2, bounds1)
end

Maybe you have a better name than gte, but remember that it must have a meaningful name!


Function to split text The following pascal function (compiled with Delphi) will split strings. It works perfectly, but how to improve the code? For example, to avoid using the repeat-until loop.

type
  TSarray = array of string;

function Split(Texto, Delimitador: string): TSarray;

var
  o: integer;
  PosDel: integer;
  Aux: string;

begin

  o := 0;
  Aux := Texto;
  SetLength(Result, Length(Aux));

  repeat

    PosDel := Pos(Delimitador, Aux) - 1;

    if PosDel = -1 then
    begin
      Result[o] := Aux;
      break;
    end;

    Result[o] := copy(Aux, 1, PosDel);
    delete(Aux, 1, PosDel + Length(Delimitador));
    inc(o);
  until Aux = '';
end;

Example:

var texto,deli:string;
    all_array:TSarray;
begin
deli := 'test';
texto := deli+'hi world 1'+deli+'hi world 2'+deli;
end;

all_array := Split(texto,deli);
ShowMessage(all_array[1]);
ShowMessage(all_array[2]);
end;

My plan is to use no classes, only the "uses" default

What alternatives do I have to repeat-until?

First, I don't quite understand what you mean with not using repeat-until, because in order to split string you have to use some kind of loop.

I assume that you want to have some speed improvements of your code.

If you are not changing string parameters passed to routine you should declare them as const parameters to make call run faster

function Split(const Texto, Delimitador: string): TSarray;

You are reserving way too much memory with SetLength(Result, Length(Aux)); You can freely increase length of resulting array as you go SetLength(Result, Length(Result) + 1); and it will also run faster.

And finally, copying string you are splitting and then deleting parts of the string is not optimal implementation. You should be using overloaded version of Pos function where third argument is offset from which it starts searching, and eliminate Aux string completely.

Also, using o as variable name is not recommended because it can easily be misread as 0, so I replaced it with i, that is commonly used as index variable.

So final code with all optimizations implemented might look like this:

function Split(const Texto, Delimitador: string): TSarray;
var
  i: integer;
  Len: integer;
  PosStart: integer;
  PosDel: integer;
begin
  i := 0;
  SetLength(Result, 1);
  Len := Length(Delimitador);
  PosStart := 1;
  PosDel := Pos(Delimitador, Texto);
  while PosDel > 0 do
    begin
      Result[i] := Copy(Texto, PosStart, PosDel - PosStart);
      PosStart := PosDel + Len;
      PosDel := Pos(Delimitador, Texto, PosStart);
      inc(i);
      SetLength(Result, i + 1);
    end;
  Result[i] := Copy(Texto, PosStart, Length(Texto));
end;


Is my LINQ query safe and efficient? I am relatively new to LINQ queries as I normally just use plain SQL to get my data back but i am changing for the good - Anyway obviously with SQL you pass in parameters so you do not get any SQL injections etc. Now the LINQ query I have written I think is safe but I am thinking it may be able to done a little easier and more efficient.

So I am passing in my data in the query from the model which is being sent from the view and my tables columns are being called from the ProjectLinqDataContext class. Is this safe? is there anything which I am doing which is not best practice.

 var context = new ProjectLinqDataContext();
 var user = from y in context.Customers
               where y.CustomerUsername == model.Email && y.CustomerPassword == model.Password
               select y;

Your ProjectLinqDataContext is disposable so make sure to wrap the usage in a using statement. Efficiency will be based on how your database is indexed. The query itself looks fine. You can also use method syntax for a more fluent and object oriented look. This doesn't affect anything, but I think it's much more readable.

using(var context = new ProjectLinqDataContext())
{
    var user = context.Customers.FirstOrDefault(y => y.CustomerUsername == model.Email && y.CustomerPassword == model.Password);
    //do something with user if it's not null
}

Make sure your password has been hashed and salted of course.


Find first 10 pairs of amicable numbers I took it as a challenge to write a C++ program to find the first 10 amicable-number pairs.
Amicable numbers
Let's take 4. What are the proper divisors of 4?. They are 1 and 2. Their sum is 3.
Now let's do the same thing for the number 220. The sum of the proper divisors of 220 is 284.
The sum of proper divisors of the number 284 is 220.
If the sum of proper divisors of two numbers are equal to each other then they are amicable.
For example 284 and 220, whose proper factors sum to 220 and 284 respectively, are amicable.
This is my C++ program to find the first 10 amicable numbers.
#include<iostream>

int GetSumOfFactors(int num){
    int sum = 0;
    for(int i = 1;i < num/2+1;i++){
        if(num % i==0){
            sum+=i;
        }
    }
    return sum;
}
int main(){
    int sum_of_factors = 0;
    int counter = 0;
    int num = 0;
    for(;;){
        num++;
        sum_of_factors = GetSumOfFactors(num);
        if(num == sum_of_factors) continue;
        if (GetSumOfFactors(sum_of_factors) == num && num > sum_of_factors){
            std::cout << "Pair: " << num << " " << sum_of_factors << "\n";
            counter+=1;
        }
        if(counter == 10) break;
    }
    return 1;
}

To make sure I don't find the same pair twice, that means 220 to 284, just like 284 to 220, I keep an extra condition where the number should be greater than its sum of factors.
Output:
Pair: 284 220
Pair: 1210 1184
Pair: 2924 2620
Pair: 5564 5020
Pair: 6368 6232
Pair: 10856 10744
Pair: 14595 12285
Pair: 18416 17296
Pair: 66992 66928
Pair: 71145 67095

Process returned 1 (0x1)   execution time : 4.955 s
Press any key to continue.

I'll add some remarks (adding to what Miguel Avila already said) and then focus on the performance aspect.

Use consistent naming: you mix camel case (FactorsSum) and snake case (sum_of_factors).
Use consistent spacing (after keywords like if and around operators).
Declare variables and the narrowest possible scope. As an example, sum_of_factors is only needed inside the for-loop.
Use proper exit codes. A non-zero exit code indicates the failure of a program. You'll want to return 0; or return EXIT_SUCCESS;. In fact you can simply omit the return statement: Reaching the end of main() does an implicit return 0; in C++.

If you care about portability: C++ does not specify the size of int, only its minimum range (which is -32767 to 32767). You should use long (with a minimum range -2147483647 to 2147483647) or one of the fixed-size types (int32_t or int64_t) depending on the needed range.
Performance improvement #1
Computing the sum of all (proper) divisors of a number can be significantly improved by observing that if \$ i \$ divides \$ n \$ then both \$ i \$ and \$ n/i \$ are divisors of \$ n\$. Therefore it suffices to test all \$ i \le \sqrt n\$. See for example Sum of all proper divisors of a natural number. A possible implementation is
// Returns the sum of all proper divisors of `n`.
int divisor_sum(int n) {
    if (n <= 1) {
        return 0;
    }

    int count = 1; // 1 is always a divisor.
    int sqrt_n = (int)sqrt(n); // Upper bound for the loop.
    for (int i = 2; i <= sqrt_n; i++) {
        if (n % i == 0) {
            // `i` and `n / i` are divisors of `n`.
            count += i;
            if (i !=  n / i) {
                count += n / i;
            }
        }
    }
    return count;
}

Performance improvement #2
In your main loop, you compute the divisor sum of sum_of_factors even if that is larger than num:
if (GetSumOfFactors(sum_of_factors) == num && num > sum_of_factors)

A simple improvement would be to change the order of the expressions:
if (num > sum_of_factors && GetSumOfFactors(sum_of_factors) == num)

Another option is to remember the divisor sums of numbers which are possible candidates of  an amicable pair, so that they need not be computed again. This can for example be done with a
std::unordered_map<int, int> abundant_divsums;

which holds all abundant numbers with their divisor sums encountered so far. A number is abundant if its proper divisor sum is larger than the number. These are candidates for an amicable pair with higher numbers.
A possible implementation is
#include <unordered_map>

int main()
{
    std::unordered_map<int, int> abundant_divsums;

    int num = 1;
    for (int counter = 0; counter < 10; num++) {
        int divsum = divisor_sum(num);
        if (divsum > num) {
            abundant_divsums[num] = divsum;
        } else if (divsum < num) {
            if (abundant_divsums.find(divsum) != abundant_divsums.end() && abundant_divsums[divsum] == num) {
                std::cout << "Pair: " << num << ' ' << divsum << '\n';
                counter++;
            }
        }
    }
}

Benchmarks
The tests were done on a MacBook Air (1.1 GHz Quad-Core Intel Core i5), with the code compiled with optimizations (“Release” configuration).
I measured the time for computing the first 10/20/50 amicable pairs. All times are in seconds.

# of amicable pairs
10
20
50

Original code:
3.8
24

After improvement #1:
0.08
0.2
3.8

After improvement #2:
0.05
0.15
2.5


Expanding a string to a certain length I wrote a function for repeating a string to a certain length.

For example:

ExpandString("Test", 11);

// output: "TestTestTes"

Are there any optimizations or fixes that can be done to this function?

private string ExpandString(string str, int length)
{
    if (length <= str.Length) return str.Substring(0, length);
    var result = str;
    for (var i = str.Length; i < length; i++)
    {
        result += str[i % str.Length];
    }
    return result;
}

OK, first, for the most straightforward optimization.  Use a StringBuilder.

The way your code currently works, it creates length - str.Length intermediate strings.  Besides creating lots of unnecessary garbage, you're copying longer and longer strings over and over again to add one character each time.

If you use a StringBuilder instead, you can avoid most of those copies.
You could try something like...

using System.Text;

...

private string ExpandString(string str, int length)
{
    if (length <= str.Length) return str.Substring(0, length);
    var result = new StringBuilder(str);
    for (var i = str.Length; i < length; i++)
    {
        result.Append(str[i % str.Length]);
    }
    return result.ToString();
}

(I don't particularly like the StringBuilder starting with a copy of str already in it. I'm literally just replacing the String.)

But you're also copying one character at a time. While this isn't as big an issue as the concatenation, you can do better by inserting as many whole copies of str as you can.

private string ExpandString(string str, int length)
{
    // BTW, you already know how big the result should be, so just
    // tell the StringBuilder its capacity to prevent unneeded resizes.
    // Side benefit: if the result is too long, you'll find out fairly quickly.
    var result = new StringBuilder(length, length);

    var wholeCopies = length / str.Length;
    for (var i = 0; i < wholeCopies; ++i)
    {
        result.Append(str);
    }

    // now append the last chunk, a possibly-zero-length prefix of `str`
    result.Append(str, 0, length % str.Length);

    return result.ToString();
}

Note that the short-string optimization has been removed. You can add it back in if you really really want to micro-optimize, but at this point, it's not gaining you nearly as much as it was.


Reading shards from a Kinesis stream Here is part of my implementation of reading a Kinesis stream. I am not confident that this is the best way to implement synchronizing returns from goroutine.

Assuming there are N shards in the stream, the code spawns goroutines for each shard in the stream for processing. Goroutine-N works indefinitely until the underlying context is canceled or until an error is encountered during processing.

The code below is supposed to wait for all N routines to terminate successfully (i.e without any of them writing to errc) or wait for at least one routine to write to errc (return an error).

If all routines terminate without writing to errc, the WaitGroup sync lock is released, closing the errc and done channels. Which would, in turn, resume the current thread. However, if one of the routines terminate writing to errc, we force all routines to terminate by calling the underlying context's cancel function and wait for done channel to close.

func (c *Consumer) scanShards(col Collector, streamName string, ids []string) error {
    checkpoints, err := col.GetSequenceNumberForShards(streamName, ids)
    if err != nil {
        return fmt.Errorf("error retrieving stored checkpoints for shards: %v", err)
    }

    errc := make(chan error, 1)
    done := make(chan error, 1)
    wg := sync.WaitGroup{}
    wg.Add(len(ids))
    for _, id := range ids {
        seqNum := checkpoints[id]

        ctx, cancel := context.WithCancel(context.Background())
        c.cancelFuncs = append(c.cancelFuncs, cancel)
        go func(ctx context.Context, shardID, startSeqNum string) {
            defer wg.Done()
            if err := c.scanShard(ctx, shardID, startSeqNum); err != nil {
                errc <- fmt.Errorf("error in shard %q: %v", shardID, err)
            }
        }(ctx, id, seqNum)
    }
    go func() {
        wg.Wait()
        close(errc)
        close(done)
    }()
    err = <-errc
    if err != nil {
        // Cancel all scans, to release the worker goroutines from
        // the pool.
        for _, cancel := range c.cancelFuncs {
            cancel()
        }
    }
    <-done // Wait for all goroutines to exit.
    return err
}

Alright, there's a number of issues with the code you posted here. I'll go through it all, and point out issues, step by step:

func (c *Consumer) scanShards(col Collector, streamName string, ids []string) error {

Alright, so we have a *Consumer (exported type), with a non-exported scanShards function. That's fine. The thing I'm slightly baffled by is why the Collector is passed as an argument here. In AWS Kinesis terms, a Collector is what you use to send stuff to Kinesis in a single request. To me, it's sort of like a buffered writer. This collector, however, seems to provide the consumer with data about the shards that you're about to consume. The collector is actually a core dependency of your consumer, it seems. It'd make sense for the consumer to have the underlying collector assigned to a field:

type Consumer struct {
    collector   Collector // Rename needed, though
    cancelFuncs []context.CancelFunc // more on this later!
}

Because you're dealing with streams, I'd strongly recommend you use the context package, too (I know you are, but you're not getting the most out of it - see below). Convention dictates that the context argument comes first, so the function would now look like this:

func (c *Consumer) scanShards(ctx context.Context, streamName string, ids []string) error {

The arguments could probably do with some renaming, but I'm probably the worst person to suggest a good variable name. Anyway, onwards:

    checkpoints, err := col.GetSequenceNumberForShards(streamName, ids)
    if err != nil {
        return fmt.Errorf("error retrieving stored checkpoints for shards: %v", err)
    }

Right, so the "collector" has an exported function getting sequence, and you're expecting the collector to be passed in as an argument. This is kind of weird. It'd make much more sense for someone to use a Consumer object and call something like consumer.GetShards(), because that actually tells me something: I know that I'm working with a consumer that is taking its data from a shards X, Y, and Z.
I've looked at what comes next, but I'll be returning to the checkpoints variable later on. First let me just recommend you look at this package for errors. You're essentially returning a specific error (which is OK), but it's a hard one to detect on the caller-side. The string value of the error still contains all of the raw/initial error value. With the package I linked to, you'd be able to write the same like so:

return errors.Wrap(err, "error retrieving stored checkpoints for shards")

Replace the string constant with an exported constant like so

const ErrRetrievingCheckpoints = "error retrievign stroed checkpoints for shards")
// and use:
return errors.Wrap(err, ErrRetrievingCheckpoints)

And you'll have a much easier time actually detecting what error was returned, without losing the underlying error details.

OK, let's move on to the channel stuff now:

    errc := make(chan error, 1)
    done := make(chan error, 1)
    wg := sync.WaitGroup{}
    wg.Add(len(ids))

Right, So you're creating a waitgroup, and add the length of ids to it, so there's going to be 1 routine per ID. Fine. Then you create an error and done channel, both with a buffer of 1. I'll explain how in a bit, but you can (and should) get rid of the done channel entirely. Having said that, channels like done are generally defined as done := make(chan struct{}): no buffer needed, and an empty struct is defined in the golang specs as a 0-byte type.

The error channel, however, has a bit of a problem: Suppose I'm going to start 10 routines, and I've got 3 errors. The first routine to err will write its error value onto the channel, and be done with it. The second and third routines to fail will be blocked, waiting for you to read the error from the channel. You have the code to do this, so you'll get that error, and the second routine will now write a new error value to the channel. The third routine is still blocked!

What your code does next, is cancel all routines (sort of), and wait for the wg.Wait() call to return. Because the third routine is blocked, this will never happen. You've got a deadlock on your hands. The done channel won't get closed, and thus this function won't return, all because your error channel is blocking routines from completing. That's not what we want. There's a quick (and not always ideal) solution to this problem: increase the channel buffer:

errCh := make(chan error, len(ids) -1) // we'll always read 1, hence -1 is big enough

Of course, we're assuming ids is not empty. Just in case, it's always a good idea to add some basic checks like if len(ids) == 0 and return early.

Still, I'll explain later how we fix this without having to increase the channel buffers too much. For now, let's carry on with the line-by-line review:

    for _, id := range ids {
        seqNum := checkpoints[id]

Yes, I have an issue with this already. checkpoints clearly is a var of type map[string]string. I know this because ids is []string, and the seqNum variable is passed to a goroutine with the type string. Why, then, iterate over a slice, and then perform a lookup in a map to get the corresponding value? Why not simply write:

for id, seqNum := range checkpoints {

That's a lot cleaner, and reduces the risk of someone else looking at your code and adding something like this:

seqNum, ok := checkpoints[id]
if !ok {
    return errors.New("checkpoint missing")
}

Nevermind the risk of someone changing the GetSequenceNumberForShards function to not return an error if not all id's were found! Suppose your ids var holds {"foo", "bar", "zar", "car"}, but the checkpoints  map only was returned without the zar key...

For the same reason, you really should replace wg.Add(len(ids)) with wg.Add(len(checkpoints))

Right, onwards, let's get stuck in with the context:

        ctx, cancel := context.WithCancel(context.Background())
        c.cancelFuncs = append(c.cancelFuncs, cancel)

Why? Why on earth? You're creating a cancel context for each routine, and append the cancel func to a slice without a way to retrieve a specific cancel function. The slice is also stored on the Consumer type. The function uses a pointer receiver, so there's a genuine risk of race conditions here! Suppose I do this:

go consumer.scanShards(col1, "foo", []string{"a", "b", "c"})
go consumer.scanShards(col2, "bar", []string{"foo", "bar", "car", "zar"})

Both routines are appending to the same slice of cancel functions. They're both calling all of them blindly, too. These routines are messing things up for eachother. And that's just one example, imagine someone coming along and writing this:

func (c *Consumer) Reset() {
    // restoring some fields
    c.cancelFuncs = []context.CancelFunc{}
    // more fields here
}

These kind of things can be really hard to debug, and your code is very vulnerable to these issues. You may think that the Reset func I've written here is an unlikely scenario, but given that there's nothing in your code that actually removes the cancel values from the slice, your slice will grow, and grow, and grow, and things will get slowed down.

OK, I had to rant about these things somewhat, but remember how I said that you ought to have your first argument be a context? Right, do it, it allows the caller to pass in a context.WithCancel, or context.WithTimeout, for example, so the caller can determine whether they want to/need to wait on your function to return. Secondly: why not have all your routines share the same context?. You're just re-wrapping the Background context anyway. Instead of doing that, I'd simply wrap the context from the argument once (outside the loop):

rctx, cfunc := context.WithCancel(ctx)

If the context passed as an argument gets cancelled, the cancellation will propagate, if you want to cancel the context, call cfunc, and all your routines will receive the cancellation signal (<-ctx.Done()). The caller is unaffected.

So with this in mind, let's rewrite the loop a bit (more improvements to follow below, but what we have thusfar):

func (c *Consumer) scanShards(ctx context.Context, stream string, ids []string) error {
    if len(ids) == 0 {
        return ErrNoIdsProvided
    }
    checkpoints, err := c.collector.GetSequenceNumberForShards(stream, ids)
    if err != nil {
        return errors.Wrap(err, ErrRetrievingCheckpoints)
    }
    if len(checkpoints) == 0 {
        return ErrNothingToCheck // something like this, should be handled properly
    }
    // note ids is irrelevant, checkpoints is the source of truth now
    errCh := make(chan error, len(checkpoints) - 1)
    // we'll get rid of this, but I've not explained how, so it's here still:
    done := make(chan struct{})
    wg := sync.WaitGroup{}
    wg.Add(len(checkpoints))
    // wrap around ctx argument once!
    rctx, cfunc := context.WithCancel(ctx)

    for id, seqNum := range checkpoints {
        go func(ctx context.Context, shardID, startSeqNum string) {
            defer wg.Done()
            if err := c.scanShard(ctx, shardID, startSeqNum); err != nil {
                errc <- fmt.Errorf("error in shard %q: %v", shardID, err)
            }
        }(rctx, id, seqNum)
    }

OK, that's where we are now. Because the rctx is not in the for scope, and is shared by all routines, you can simply write the routine like so:

go func(id, seqNum string) {
    defer wg.Done()
    // note: rctx, not ctx!
    if err := c.scanShard(rctx, id, seqNum); err != nil {
        errCh <- err
    }
}(id, seqNum)

The routine centers around c.scanShard, a function you've not posted, and thus I have no idea what it does (how it uses the context etc...). What is clear to me, though, is that it's an unexported function, and you have control over it. You could just as easily make it into a function that behaves exactly like the routine you wrapped the call in, so you can replace the go func() bit simply with this:

go c.scanShard(rctx, &wg, id, seqNum, errCh)
// given scanShard looks like this:
func (c *Consumer) scanShard(ctx context.Context, wg *sync.WaitGroup, id, seqNum string, errCh chan<- error)

Anyway, the rest of your code is about tying things together:

    go func() {
        wg.Wait()
        close(errc)
        close(done)
    }()

So waiting for the routines to return before closing the channels, which is fair enough, seeing as writing to a closed channel is bad. Closing the done channel here, though is what actually allows your function to return. But does it add any value?

    err = <-errc
    if err != nil {
        for _, cancel := range c.cancelFuncs {
            cancel()
        }
    }
    <-done // Wait for all goroutines to exit.
    return err
}

And this is where things get realy messy: you're first checking the error channel (fair enough), but because you're not sure whether or not you've passed the blocking read (err = <-errCh) because the channel was closed or not, you have to check whether or not you actually have an error. If not, you're still checking the  done channel, completely pointlessly. Even with your code as it stands, the done channel is completely redundant, the error channel already does the same thing.

Get rid of done, even with your current code, it serves no purpose whatsoever.

Alternative approach

I've been ranting plenty for now, and I think this is a decent starting point. When I started looking at your code, my mind did jump to select statements, and using the ctx.Done() to stop calling the scanShard function. I'm not sure if it's actually the better way for this particular case, but it might be worth considering. I'll just include a quick write-up of how I'd write this function using ctx.Done and select to control the flow. It's a bit more verbose, and actually looks more complex than it needs to be IMHO, but it's just to show an alternative approach, that in some cases might be worth considering. I have to say, all code in the answer is untested, and just written as I went along, so typo's and bugs are possible:

package main

import (
    "context"
    "fmt"
    "sync"
)

func (c *Consumer) scanShards(ctx context.Context, streamName string, ids []string) error {
    checkpoints, err := c.collector.GetSequenceNumberForShards(streamName, ids)
    if err != nil {
        // zerolog
        c.logger.Error().Err(err).Msg("Error retrieving stored checkpoints for shards")
        return err
    }

    errc := make(chan error, 1)
    defer close(errc)
    wg := sync.WaitGroup{}
    wg.Add(len(checkpoints))
    rctx, cfunc := context.WithCancel(ctx)
    for id, seqNum := range checkpoints {
        go func(shardID, startSeqNum string) {
            defer wg.Done()
            select {
            case <-rctx.Done():
                // context cancelled, no need to do anything
                return
            default:
                if err := c.scanShard(rctx, shardID, startSeqNum); err != nil {
                    c.logger.Error().Err(err).Msg("Error in shard %q", shardID)
                    errc <- fmt.Errorf("error in shard %q: %v", shardID, err)
                }
            }
        }(id, seqNum)
    }
    go func() {
        // wg cleared -> cancel context
        wg.Wait()
        cfunc()
    }()
    select {
    case <-ctx.Done():
        // ctx was closed after waitgroup, so nothing to do here
        return nil
    case err := <-errc:
        // error -> cancel routines
        cfunc()
        return err // return, this will close channel
    }
}


Producing a matrix class that supports any size I'm doing exercise 11.8 from Scala for impatient, asking to write a matrix class:

  Provide a class Matrix - you can choose whether you want to implement 2
  x 2 matrices, square matrices of any size, or m x n matrices. Supply operations + and *. The latter should also work with scalars, for example mat * 2. A single element should be accessible as mat(row, col).

The code is working, but may be there can be improvements to make code more functional or more stable:

class Matrix(val n: Int, val m: Int, fun: (Int, Int) => Double) {
  private val matrix = Array.tabulate[Double](n,m)(fun)

  def +(another: Matrix) = {
    if (n != another.n || m != another.m)
      throw new IllegalArgumentException("Sizes of arrays don't match.")
    else
      new Matrix(n, m, (i, j) => this(i)(j) + another(i)(j))
  }
  def *(another: Matrix) = {
    if (m != another.n)
      throw new IllegalArgumentException("Sizes of arrays don't match.")
    else
      new Matrix(n, another.m, (i,j) => { var sum = 0.0;
                                          for (k <- matrix(i).indices) sum += matrix(i)(k) * another(k)(j);
                                          sum }
      )
  }
  def *(k: Int) = new Matrix(n, m, k * matrix(_)(_) )

  def apply(i: Int)(j: Int) = matrix(i)(j)

  override def toString = {
    var result = ""
    for (row <- matrix) {
      for (value <- row)
        result += value + "  "

      result += "\n"
    }
    result
  }
}

Remove all var. Doesn't the book show for...yield by that point? For example:

(i,j) => {
  var sum = 0.0;
  for (k <- matrix(i).indices) sum += matrix(i)(k) * another(k)(j);
  sum
}

Can be replaced with:

(i,j) => (for (k <- matrix(i).indices) yield matrix(i)(k) * another(k)(j)).sum

The var on toString can be replaced by using mkString. I'll let you work out for yourself how to do that, now that I called your attention to that method. And, yes, it that method works on Array as well, though it doesn't appear on Scaladoc for Scala up to 2.9.2 because it is added implicitly. I suggest you use the nightly scaladoc to look things up -- the documentation there is better, though it may show things not available on release versions, and so is the tool itself.


Parsing a string with regex to build JSON I'm pretty sure the code is of mild quality but not entirely sure as I'm not much of a JavaScript programmer. Everything works as expected. A quick rundown of the code. It simply parses a string and builds some game data for saving into a database. One thing I'm not sure of is how I should handle data that doesn't conform to how it's expected.

// var diplo = `!report Game Type: Diplo Early Bird
// 1: <@12321321421412421>
// 2: <@23423052352342334>
// 3: <@45346346345343453>
// 4: <@23423423423523523>`

var diplo = `!report Game Type: Team Modded
Team: 1
    <@23452342359234534>
    <@34642342359232352>
Team: 2
    <@34534342359234879>
    <@58493457202345054>`

var report = {};

function gameType(game) {
    let data = {};
    let game_types = [{id: 1, name: 'Diplo'}, {id: 2, name: 'Always War'}, {id: 3, name: 'FFA'}
        , {id: 4, name: 'No Diplo'}, {id: 5, name: 'Team'}, {id: 6, name: 'Duel'}, {id: 7, name: 'No War'}];
    let o = {}
    let reType = /!report Game Type:[\s+]?(\d|\w+)\s?(\w+\s?(\w+)?)?\n/gi
    let match = reType.exec(game)
    if(match[1]){
        for (var j = 0; j < game_types.length; j++) {
            if ((game_types[j].name).toLowerCase() === (match[1]).toLowerCase()) {
                data.type = game_types[j].id;
                break;
            }
        }
    }
    if(match[2]){
        data.moddifier = match[2]
    }
    diplo = game.slice(reType.lastIndex);
    return (data)
}

function getPlayers(game) {
    let players = [];
    rePlayer = /(\d+):\s?<@(\d+)>[\s\S]?(\w+\s?\w+)?/gi;
        do {
            let o = {};
            player = rePlayer.exec(game);
            if (player) {
                o.position = player[1]
                o.discord_id = player[2]
                players.push(o);
            }
        } while (player);
        return players;
}

function getTeamPlayers(game) {
    let players = [];
    reTeam = /Team:\s(\d)[\s\S](\s+?<@(\d+)>)+/gi;
    rePlayer = /\s+?<@(\d+)>/gi;
    let i = 1;
    do {
        team = reTeam.exec(game);
        if(team){
            do {
                player = rePlayer.exec(team[0]);
                if(player){
                    let o = {};
                    o.team_id = i;
                    o.position = team[1];
                    o.discord_id = player[1];
                    players.push(o);
                }
            }while(player)
        }
        i++;
    }while(team)
    return players;
}

function main() {

    console.log("Reading game data...")
    try {
        report = gameType(diplo)
        if(report.type === 5){
            report.players = getTeamPlayers(diplo);
        }else{
            report.players = getPlayers(diplo);
        }
        console.log(report)
    } catch (err) {
        return 'Error Processing Report!';
    }
}

main();

I would manipulate RegExp.lastIndex with the sticky flag to avoid slicing the source string:

function parseGame(text) {
  const RX_HEADER = /!report Game Type:\s*(\d|\w+)(?:\s*(\w.*))?/y;
  const RX_TEAM = /Team:\s*(\d+)/y;
  const RX_DISCORD = /<@(\d+)>/y;
  const RX_POSITION_DISCORD = /(\d+):\s*<@(\d+)>\s*(?:\w+\s?\w+)?/y;
  const RX_WHITESPACE = /\s+/y;

  let pos = 0;
  const data = {players: []};

  function match(rx) {
    rx.lastIndex = pos;
    const matches = rx.exec(text);
    if (!matches) {
      return [];
    }
    pos = rx.lastIndex;
    if (pos < text.length) {
      RX_WHITESPACE.lastIndex = pos;
      RX_WHITESPACE.exec(text);
      pos = RX_WHITESPACE.lastIndex || pos;
    }
    return matches;
  }

  function readHeader() {
    const ID_NAME_MAP = {
      1: 'Diplo',
      2: 'Always War',
      3: 'FFA',
      4: 'No Diplo',
      5: 'Team',
      6: 'Duel',
      7: 'No War',
    };
    let [_, gameType, gameMod] = match(RX_HEADER);
    if (!gameType) {
      return;
    }
    gameType = gameType.toLowerCase();
    for (const type in ID_NAME_MAP) {
      if (type === gameType || ID_NAME_MAP[type].toLowerCase() === gameType) {
        data.type = Number(type);
        break;
      }
    }
    if (gameMod) {
      data.moddifier = gameMod;
    }
  }

  function readTeams() {
    let teamId;
    while ((teamId = Number(match(RX_TEAM)[1]))) {
      let position = 0;
      let discordId;
      while ((discordId = match(RX_DISCORD)[1])) {
        data.players.push({
          teamId,
          discordId,
          position: ++position,
        });
      }
    }
  }

  function readPlayers() {
    while (true) {
      const [_, position, discordId] = match(RX_POSITION_DISCORD);
      if (position) {
        data.players.push({
          discordId,
          position: Number(position),
        });
      } else {
        break;
      }
    }
  }

  readHeader();
  if (data.type === 5) {
    readTeams();
  } else {
    readPlayers();
  }
  return data;
}

And just to nitpick:

[\s+]? matches whitespace and a + which seems weird so I replaced it with  \s*
[\s\S]? in rePlayer is ambiguous as it allows any single character, which seems weird.
I would expect only whitespace and new line(s) allowed e.g. \s*
I usually specify a match group is unused via ?: e.g. (?:foo)?
Indent the code consistently
Consistently use spaces before opening braces e.g. if (foo) { instead of if(foo){
No need for var generally if you use ES6 anyway
Use const for variables you don't reassign (more info)
Use a consistent type for the id and position instead of mixing strings & numbers
Use a JS linter like eslint to catch the errors like a global player variable
Use camelCase for properties and variable names
Use object literals like arr.push({foo: 'bar'}) without an interim variable