Python/Sklearn code optimization (New at Python) I am trying to optimize the following code, as you may see I am quite new to python but I am trying to improve, any kind of tip or code changes is welcome. I am trying to optimise this module by keeping it as short as possible but still understandable for everyone.

from copy import deepcopy

import pandas as pd

from sklearn import ensemble
from sklearn.externals import joblib
from sklearn.metrics import mean_absolute_error
from sklearn.model_selection import train_test_split

from draw_benchmark_graphs import draw_benchmark_graphs

def preparate_dataset():
    # Load the dataset.
    dataframe = pd.read_csv('datasets/eggs_data_set.csv')
    features_df = deepcopy(dataframe)

    # Remove the wanted dataset output from the features-list.
    del features_df['days_remaning']

    # Create the Features(X) & Output(Y) arrays.
    features = features_df.as_matrix()
    output = dataframe['days_remaning'].as_matrix()

    # Split the data in a 70% to 30% ratio,the 30% will be kept hidden for testing porpuses.
    x_train, x_test, y_train, y_test = train_test_split(
        features, output, test_size=0.3)

    return [x_train, y_train, x_test, y_test]

def generate_new_model():
    dataset = preparate_dataset()
    x_train = dataset[0]
    y_train = dataset[1]
    x_test = dataset[2]
    y_test = dataset[3]

    # Create & fit a GB model using GridSearch optimised parameters.
    params = {
        'learning_rate': 0.01,
        'loss': 'ls',
        'max_depth': 4,
        'max_features': 0.3,
        'min_samples_leaf': 9,
        'n_estimators': 500
    }
    model = ensemble.GradientBoostingRegressor(**params)
    model.fit(x_train, y_train)

    # Find the error rate on the training set.
    mae_train = mean_absolute_error(y_train, model.predict(x_train))
    print('Training Set Mean Absolute Error: %.4f' % mae_train)

    # Find the error rate on the test set.
    mae_test = mean_absolute_error(y_test, model.predict(x_test))
    print('Test Set Mean Absolute Error: %.4f' % mae_test)

    # Save the trained model to a file for later use,
    # otherwise we will need to train a new model everytime we want to use it.
    joblib.dump(model, 'models/trained_eggs_classifier_model.pkl')

    draw_benchmark_graphs(model, params, x_test, y_test, mae_train, mae_test)
    return (model, (mae_train, mae_test))

generate_new_model()

I can't give you any advice on performance but here are some notes on coding style:

There is no need to add comments to obvious things. You can safely delete all of them and it won't hurt readability. But you could make a docstring from # Create & fit a GB model using GridSearch optimised parameters.
Instead of generate_new_model() in the end of your script write this:

if __name__ == '__main__':
    generate_new_model()

You can read about it for example here.
You have some hard-coded paths and magic numbers. How about taking them to function's signatures? You also could use type hints.
It is a common practice to name dataframes simply as df instead of dataframe. But it's not really critical.
Instead of deepcopy you could use a copy method, and a drop method instead of del.
Why using as_matrix()? As docs say, train_test_split accepts pandas dataframes.
Instead of writing:

x_train, x_test, y_train, y_test = train_test_split(
    features, output, test_size=0.3)
return [x_train, y_train, x_test, y_test]

You can write just:

return train_test_split(features, output, test_size=0.3)

and change the order in the place where you call this function.
Instead of:

dataset = prepare_dataset()
x_train = dataset[0]
x_test = dataset[1]
y_train = dataset[2]
y_test = dataset[3]

you can write:

x_train, x_test, y_train, y_test = prepare_dataset()

Personally, I prefer dict constructors:

params = dict(learning_rate=0.01,
              loss='ls',
              max_depth=4,
              max_features=0.3,
              min_samples_leaf=9,
              n_estimators=500)

It took me some time to understand the meaning of mae. Can we just name those variables as train_error and test_error?
I suggest adding keyword arguments to your call of draw_benchmark_graphs, so it would look something like this:

draw_benchmark_graphs(model=model,
                      parameters=params,
                      x_test=x_test,
                      y_test=y_test,
                      train_error=train_error,
                      test_error=test_error)

Also from draw_benchmark_graphs import draw_benchmark_graphs looks strange. Choose better names. How about import draw and call the function like draw.benchmark_graphs(...)?
You have redundant parentheses in return. Also, why do you pack errors in a tuple? Flat is better than nested, so make your return like this:

return model, train_error, test_error

Think of splitting your script to more functions. How you do it depends on your workflow. It's just your function is called generate_new_model but what you actually do is: reading csv file to dataframe, splitting it to samples(or how you call it), instantiating and fitting a model, calculating and printing errors, saving model in a file and drawing a graph. Also, you return data consists of different types which I think is not a good thing.

Here is how it can look like in the end. Except the fact that I didn't include docstrings and didn't try to split it to smaller functions.

from typing import (Tuple,
                    List)

import pandas as pd

from sklearn import ensemble
from sklearn.externals import joblib
from sklearn.metrics import mean_absolute_error
from sklearn.model_selection import train_test_split

import draw

def generate_new_model(
        path: str = 'datasets/eggs_data_set.csv',
        *,
        save_path: str = 'models/trained_eggs_classifier_model.pkl',
        output_column: str = 'days_remaining',
        test_size: float = 0.3,
        learning_rate: float = 0.01,
        loss: str = 'ls',
        max_depth: int = 4,
        max_features: float = 0.3,
        min_samples_leaf: int = 9,
        n_estimators: int = 500,
        **kwargs) -> Tuple[ensemble.GradientBoostingRegressor, float, float]:
    (x_train, x_test,
     y_train, y_test) = prepare_data(path,
                                     output_column=output_column,
                                     test_size=test_size)

    model_parameters = dict(learning_rate=learning_rate,
                            loss=loss,
                            max_depth=max_depth,
                            max_features=max_features,
                            min_samples_leaf=min_samples_leaf,
                            n_estimators=n_estimators,
                            **kwargs)
    model = ensemble.GradientBoostingRegressor(**model_parameters)

    model.fit(x_train, y_train)

    train_error = mean_absolute_error(y_train, model.predict(x_train))
    test_error = mean_absolute_error(y_test, model.predict(x_test))

    print(f'Training Set Mean Absolute Error: {train_error:.4f}')
    print(f'Test Set Mean Absolute Error: {test_error:.4f}')

    joblib.dump(model, save_path)

    draw.benchmark_graphs(model=model,
                          model_parameters=model_parameters,
                          x_test=x_test,
                          y_test=y_test,
                          train_error=train_error,
                          test_error=test_error)
    return model, train_error, test_error

def prepare_data(path: str,
                 *,
                 output_column: str,
                 test_size: float) -> List[pd.DataFrame]:
    df = pd.read_csv(path)

    features = df.drop(output_column, axis=1)
    output = df[output_column]

    return train_test_split(features,
                            output,
                            test_size=test_size)

if __name__ == '__main__':
    generate_new_model()


Read and sort an array of integers //Program To Input Values Into An Array And Sort Them In Ascending Order
#include<stdio.h>
//Function To Print The Array Values
void print(int tab[],int n)
{
    int i;
    printf("The Values Are : \n");
    for(i=0;i<n;i++)
    {
        printf("%d ",tab[i]);
    }
        printf("\n");//Line Jumping For Better Output
}
//Function To Swap The Values In The Array
void swap(int* x,int* y)
{
    int temp=*x;
    *x=*y;
    *y=temp;
}
//Function To Sort The Values In The Array By Using Previous Function
void sort(int tab[],int n)
{
    int i,j;
    for(i=0;i<n-1;i++)
    {
        for(j=i+1;j<n;j++)
        {
            if(tab[i]>tab[j])
            {
                swap(&tab[i],&tab[j]);//Swaps The Value Of Index i With The Value Of Index j In The Array
            }
        }
    }
}
//Main Code
int main()
{
    int i,n,tab[100];
    printf("Give The Number Of Elements In The Array:");
    scanf("%d",&n);
    while(n<=0)
    {
    printf("The Number Of Elements MUST Be Superior To 0!\n");
    printf("Give The Number Of Elements In The Array:");
    scanf("%d",&n);
    }
    printf("Insert %d Values :\n",n);
    for(i=0;i<n;i++)
    {
        scanf("%d",&tab[i]);
    }
    print(tab,n);//Prints The Values In The Array
    sort(tab,n);//Sorts The Values In The Array
    printf("SORTING....\n");
    print(tab,n);//Prints The Values In The Array
    return 0;
}

General Observations
Good job creating functions and the naming of the functions. The naming of the variables could probably be improved, for instance n could be renamed to item_count or array_element_count.
The indentation is inconsistent. This could lead issues during maintenance of the code.
Missing Function
As stated above you did a pretty good job of creating functions, but you missed one, the input of the array should be a function as well.
Logical Order of Printing
Put printf("SORTING...\n"); before the call to the sort() function.
Comments
Since you did a good job of naming the functions most of your comments are unnecessary. For instance //Swaps The Value Of Index i With The Value Of Index j In The Array really doesn't add anything.
Arguments for Swap Function
Rather than passing in 2 pointers to values and swapping the values I might pass in the array and the 2 indexes and use array notation in the swap function to relocate the values.
Missing Error Check
The code to input the array properly checks to see that the value of the size of the array is greater than 0, but it doesn't check that the size of the array is less than than the maximum size.


Unit Testing for a Complex Game I have known about unit testing for a while now, but I am just now finally understanding how to implement it.  I think that my initial implementation is a little rough so I could use some feedback.

I have implemented these tests in order to verify that a new feature of my strategy game is working properly. Previously, I would have output these values to the screen somewhere and simply watched to make sure that the expected behavior was happening. With these tests, I have concrete proof that the feature is working.  This is much better!

However there are some problems. I needed to implement a Testing category for the class that I am testing because some of the methods I needed to run the tests are private. By creating this category at the top of the testing class, I can use private methods from the class without having to permanently expose them.  This works, but I have heard that I should only be unit testing public methods, so I am wondering if what I have done is acceptable. I should note that I also had to add a couple getter methods to the class to get some information that was not public.

DTDwarfTests.m

#import <XCTest/XCTest.h>
#import "DTDwarf.h"
#import "DTJobUnit.h"

@interface DTDwarf (Testing)
    -(void) setMoodForTesting:(int)moodAmount;
    -(void) startJobCountdown:(JobType)jobType;
    -(BOOL) doJobCountdown;
    -(void) calculateMoodState;
    -(void) calculateMoodAmountForJob;
    -(JobType) getFavoriteJob;
    -(JobType) getHatedJob;
@end

@interface DTDwarfTests : XCTestCase
@end

@implementation DTDwarfTests

- (void)setUp {
    [super setUp];
    // Put setup code here. This method is called before the invocation of each test method in the class.
}
- (void)tearDown {
    // Put teardown code here. This method is called after the invocation of each test method in the class.
    [super tearDown];
}

-(void) testMoodInitialization {
    DTDwarf *dwarf = [[DTDwarf alloc]initWithWorldSize:CGSizeZero];
    XCTAssert(dwarf.moodAmount > 0);
}
-(void) testFavoriteAndHatedJobsValues {
    DTDwarf *dwarf = [[DTDwarf alloc]initWithWorldSize:CGSizeZero];
    XCTAssert([dwarf getFavoriteJob] != JobTypeNumJobTypes);
    XCTAssert([dwarf getHatedJob] != JobTypeNumJobTypes);
}
-(void) testFavoriteJobMoodInfluence {
    DTDwarf *dwarf = [[DTDwarf alloc]initWithWorldSize:CGSizeZero];
    [dwarf setMoodForTesting:0];
    dwarf.dwarfJobUnit = [[DTJobUnit alloc]initWithJobType:[dwarf getFavoriteJob]];
    [dwarf calculateMoodAmountForJob];
    XCTAssert(dwarf.moodAmount > 0);
}
-(void) testHatedJobMoodInfluence {
    DTDwarf *dwarf = [[DTDwarf alloc]initWithWorldSize:CGSizeZero];
    [dwarf setMoodForTesting:1000];
    dwarf.dwarfJobUnit = [[DTJobUnit alloc]initWithJobType:[dwarf getHatedJob]];
    [dwarf calculateMoodAmountForJob];
    XCTAssert(dwarf.moodAmount < 1000);
}
-(void) testBadMoodIncreasesCountdown {

    DTDwarf *happyDwarf = [[DTDwarf alloc]initWithWorldSize:CGSizeZero];
    [happyDwarf setMoodForTesting:1000];
    [happyDwarf calculateMoodState];
    [happyDwarf startJobCountdown:JobTypeMining];
    int happyDwarfCountdown = 0;
    while (![happyDwarf doJobCountdown]) {
        happyDwarfCountdown++;
    }

    DTDwarf *sadDwarf = [[DTDwarf alloc]initWithWorldSize:CGSizeZero];
    [sadDwarf setMoodForTesting:0];
    [sadDwarf calculateMoodState];
    [sadDwarf startJobCountdown:JobTypeMining];
    int sadDwarfCountdown = 0;
    while (![sadDwarf doJobCountdown]) {
        sadDwarfCountdown++;
    }

    XCTAssert(happyDwarfCountdown < sadDwarfCountdown);
}

@end

Unit testing isn't something I do a whole lot of.  So as for a general review of what you've written, I can't attest much to that.

But as to the idea of creating a class category just to sorta-kinda exposed "private" methods for the sake of testing and testing only?  Well... let's talk about Objective-C, shall we?

Despite the way we talk about "methods", Objective-C doesn't really do... methods...  they're all messages which invoke underlying C functions.

In other languages, if you try to call a non-existent method, your code won't compile.  In Objective-C, you'll merely get a warning if the file you're in can't see the method you're trying to call, and if it really doesn't exist, you'll get a runtime crash.

But the fact remains that you can still compile the code and you can still pass the message to the object.  And if the object implements the method that matches the message, it will execute the code.

So even without the class category, we could still call the methods on the object--we'd just have a bunch of warnings.

And there's ways around the warnings as well.  This blog discusses this in more detail, but here's an example of doing this with one of your methods:

Given the method:

- (void)setMoodForTesting:(int)moodAmount;

We could invoke this method as such:

SEL mftSelector = NSSelectorForString(@"setMoodForTesting:");

((void (*) (id, SEL, float))objc_msgSend)(dwarf, mftSelector,1000);

It may also be worth looking into this blog entry for some information on cleaning this up and making it more reasonable to use.

In the end, this allows you to still test private details of the class, but these details aren't exposed anywhere to anyone.  If somehow you were to accidentally import this .m directly, you would then expose these private details and could accidentally use them somewhere.


Where should I put code which calculates stats which will be sent via mail I want to send mail which will have day to day report. I have setup action mailer like this

class ReportMailer < ApplicationMailer
  ...

  def send_stats(hash)
    @emails = ["my@email.com"]
    @stat = hash
    mail(:to => @emails.join(","), :subject => "Daily Count of tasks")
  end
end

The send_stats method will need parameter hash which should be passed. This hash will have different counts which are not limited to one model. So I came with an idea of report module.

module Report
  def self.email_stats(date)
    hash = {}
    hash["TaskCount"] = Task.where(adate: date).count
    hash["ProfileCount"] = Profile.where(ad_date: date).count
    ReportMailer.delay.send_stats(hash)
  end
end

Is this a right approach? Does anyone have better idea than this?

So far as I can tell without seeing the rest of your of your application structure this seems fine. I am making the assumption that this is small app and that performance is not a real concern as of yet. One suggestion though, you could take those where statements and internalize them in their respective models as scopes.

scope :task_count, -> (date) { where(:adate => date).count }
scope :profile_count, -> (date) { where(:ad_date => date).count }

This would at least neaten things up a bit and is a good habit to form, because it will insure consistency in behavior. Even though these are simple queries if you are reimplementing them (or any query) everywhere it creates the possibility to introduce errors; this is a benefit of code reuse.


Efficient ways of looking for start-key and processing data This question is a follow up from my previous question.

I am working with some wireless communication in Arduino.

979797 is the address of the transmitter. I have written the following code to check the address first before processing the data. My main concern is speed of computation.

void printBuffer(const uint8_t *serialBuffer)
{
  //print contents of buffer
  for (int i = 0; i<100; i++)
    Serial.println(serialBuffer[i]); //remove the print in binary option to decode
}

void processBytes(const uint8_t value)
{
  static uint8_t serialBuffer[bufferSize]; //buffer to hold values
  static unsigned int startPosition=0;

  if(startPosition < bufferSize)
  {
    serialBuffer[startPosition] = value;
    startPosition++;
  }

  else
  {
    printBuffer(serialBuffer);
    startPosition = 0;
  }

}//end of processBytes
void loop() {

  //check if data is available in serial buffer
  while(RX.available())
  {
    //check for start key
    address(RX.read())
    //process data
    for(int i = 0; i< 68; i++)
        processBytes(RX.read());
  }
}
void address(const uint8_t value)
{
  int counter = 0;
  int newVal = value;
  while(value == 97 && counter < 3)
  {
    counter++;
    newVal = RX.read();
  }
}

The data packet just has the following components: start key (979797) and 68 bytes of data(payload). There is no termination key. Also, the transmitter transmits one byte at a time. Thus, each value received is less than 256. Hence why I am saving the data into a uint8_t[] array.

Example of possible data:

97 97 97 (68 bytes of random positive integers all less than 256) 97 97 97

Other than the address() method, most of it was borrowed from Gammon. Here is the reference page for the arduino commands I am using. Are there ways I can optimize this computation?

Still problematic
The code still has the following issues:

newVal is set but not used:

    while(value == 97 && counter < 3)

should be:
    while(newVal == 97 && counter < 3)

Assuming the byte stream follows the protocol, address() will eat the first byte of the message.  In other words, the byte following the 97 97 97 header will be discarded.  This will in turn cause the message processor to eat one byte of the next address section.

There is no recovery mechanism for if the byte stream stops following the protocol.  The address() function doesn't look for a header, it just returns in the absence of a header.

Rewrite
I suggest this rewrite:
void loop(void)
{
    // Check if data is available in serial buffer
    while (RX.available()) {
        // Search for start key
        FindStartKey();
        // Process data
        for(int i = 0; i < 68; i++)
            processBytes(RX.read());
    }
}

void FindStartKey(void)
{
    int counter = 0;

    do {
        if (RX.read() == 97)
            counter++;
        else
            counter = 0;
    } while (counter < 3);
}


Image Database Service I created simply database service with get, insert, get all image names and verify exist method.

All images will be store in blob storage with unique filename + postfix size. Next one I have image model, with simple expression for mapping object select, methods for detection Webp format and existing.

All methods:

GetAllImageNames() - return enumerable list of imageNames
Insert(string filename) - insert filename
site GetImage(string filename) - return image entity, mapped to
ImageModel ImageExist(string filename) - return true/false
Delete(string filename) - delete file by filename

I wrote the Image object entity that represents a database object:

public class Image
{
    public int Id { get; set; }

    public string Filename { get; set; }
}

This is a very simple entity, no comments are needed I think.

ImageModel (object result structure):

public class ImageModel
{
    public string Filename { get; set; }

    public ImageSize Size { get; set; }

    public static Expression<Func<Image, ImageModel>> ToModel()
    {
        return image => new ImageModel
        {
            Filename = image.Filename,
            Size = image.Size
        };
    }

    public bool Exists()
    {
        return !string.IsNullOrEmpty(Filename);
    }

    public bool IsWebp()
    {
       //Now implement for now
    }
}

This model class is very simply again:

ToModel() is a function for mapping to results data.

And finally DatabaseImageService:

public interface IImageDatabaseService : IRepository<Image>
{
    Task<IEnumerable<string>> GetAllImageNames();

    Task Insert(string filename);

    Task Delete(string filename);

    Task<ImageModel> GetImage(string filename);

    Task<bool> ImageExist(string filename);
}

IRepository is design pattern Repository for manipulation with database operations. It's possible to use it as DI of course. But I like to use it as an inherit interface of the current entity.

public class ImageDatabaseService : Repository<Image>, IImageDatabaseService
{
    public ImageDatabaseService(DbContext dbContext, IUnityOfWork unityOfWork) : base(dbContext)
    {
        this.UnityOfWork = unityOfWork;
    }

    private IUnityOfWork UnityOfWork { get; set; }

    public async Task<IEnumerable<string>> GetAllImageNames()
    {
        return await this.Select(s => s.Filename).ToListAsync();
    }

    public async Task Insert(string filename)
    {
        this.InsertAsBatch(filename);
        await this.UnityOfWork.SaveAsync();
    }

    public async Task Delete(string filename)
    {
        var image = await this.FirstOrDefaultAsync(s =>
            s.Filename.Equals(filename, StringComparison.InvariantCultureIgnoreCase));
        if (image != null)
        {
            this.Remove(image);
            await this.UnityOfWork.SaveAsync();
        }
    }

    public async Task<bool> ImageExist(string filename)
    {
        var image = await GetImage(filename);
        return image.Exists();
    }

    public async Task<ImageModel> GetImage(string filename)
    {
        return await this.Where(s => s.Filename.Equals(filename, StringComparison.InvariantCultureIgnoreCase))
            .Select(ImageModel.ToModel()).FirstOrDefaultAsync();
    }

    private void InsertAsBatch(string filename)
    {
        var image = this.Create();
        image.Filename = filename;
        //possibly another image attributes

        this.Add(image);
    }
}

UnityOfWork is a design pattern for saving data to my database, used in combination with the Repository pattern.

First of all the controversial point: there are very good chances that you do not need a Repository. Why?

Entity Framework already implements Unif of Work. No need to manually handle anything outside very very special cases.
There are infinitesimal chances that you'll need to switch to another ORM (and if you will do then you will need to rewrite your service layer because EF-specific code pervades those classes).

Honestly: how many times you actually did it? I think I did it three times (in 20 years) and if I have a service layer (and it is well-written) then it's probably the only part you need to rewrite.

Drop all that code and make your code easy to read, programming is already complicate and error-prone without adding useless complexity.

Model

Do you actually need read/write properties? Details vary according to the EF version you're using but at least Image.Id might be read-only.

Validate your inputs! Is an empty file name valid?

public sealed class Image
{
    public int Id { get; }
    public string FileName
    {
        get => _fileName;
        set
        {
            if (String.IsNullOrWhiteSpace(value))
                throw new ArgumentException("...");

            _fileName = value;
        }
    }

    public Image(int id, string fileName)
    {
        _id = id;
        FileName = fileName;
    }

    private readonly int _id;
    private string _fileName;
}

Please do better than this (for example I hate to throw ArgumentException when input is null, ArgumentNullException should be used instead). If file name can be null (in your DB!) then just ignore this. Also note that if not required by your ORM then class should be sealed.

View Model

Same as above, do you need read/write properties? If not then make the relevant ones read-only.

I'm perplex about the ToModel() static method. Is there a good reason you're returning an Expression instead of the object itself? If for any reason you need some kind of lazy creation (and for such simple model without any heavy initialisation I can't imagine the reason) then why you're using an expression instead of a simple Func<T>?

Exists() and IsWebp() are named as properties but declared as methods. If the operation might be expensive then you're right to use methods but then you should name them as verb+noun (for example CheckIfFileExists()). If they're trivial then just use properties:

public bool Exists => !String.IsNullOrWhiteSpace(FileName);

ImageSize is declared here but it's not in the model. Do not post partial code  (or, at least, be sure it compiles).

Service

Asynchronous methods should have the Async suffix. It's not required but it's a well-accepted convention and you should follow it.

InsertAsBatch() is misleading. Which batch? I see just one parameter. It might be InsertCore(), InsertImpl() or simply dropped all together because it does not add any value.

In the Delete() method you're doing something extremely dangerous: you're swallowing an error. If trying to delete a non existing image is a soft error you may, at least, return a flag (it's what, for example, .NET collection classes do). I'd prefer a good old exception but that's just my opinion. Also do not repeat your code, to search an image you already have GetImage() method:

public async Task<bool> DeleteAsync(string fileName)
{
    var image = await GetImage(fileName);
    if (image == null)
        return false;

    this.Remove(image);
    await this.UnityOfWork.SaveAsync();

    return true;
}

Whatever you pick be consistent: some methods silently ignore this issue (like DeleteAsync() while others will throw at run-time (like ImageExists()).

public async Task<bool> ImageExistsAsync(string fileName)
    => (await GetImageAsync(fileName))?.Exists() ?? false;

Validate your parameters! I can always pass null for fileName and I don't get any error. Even more serious: decide if null is valid or not because in GetImageAsync() you're calling ImageModel.FileName.Equals() and it'll undoubtedly fail with a tough NullReferenceException if that property is null.

I'm not sure if GetAllImages() should be asynchronous or not. You're reading values from DB and sometimes it might be useful to filter them ("...all images that contain text ABC...") but forcing materialization (even if asynchronous) precludes this possibility. It's also true that you should introduce a separate service method for this then I'd keep it as-is (just adding async in the name).

Are ImageModel.Exists() and ImageModel.IsWebp() used only inside the service? If yes then they should probably be internal instead of public.

Error handling? I do not see a single line to handle DB errors (for example concurrent updates). Is it everything inside Repository base class?


Script that retrieves trending topics from Twitter The following code fetches trending topics from Twitter's API using a WOEID. It caches the response, loading from this cache the next time if its relatively fresh (generated within the last 15 minutes) to avoid returning a "Rate limit exceeded" message from Twitter.

A 'force-refresh' flag can be passed to skip the cache.

Please could someone have a look at the code and suggest ways it can be improved?

<?php
use Abraham\TwitterOAuth\TwitterOAuth;
$cache_file = '/home/domstapleton/public_html/home/profiles/default/cache/twitter-trends-' . $_GET['location-code'] . '.data';
if (file_exists($cache_file) && !$_GET['force-refresh']) {
    $data = unserialize(file_get_contents($cache_file));
    if (($data['timestamp'] > time() - 15 * 60)) {
        $trends = $data['trends'];
        echo $trends;
        exit;
    }
}
if (!$trends) {
    require '/home/domstapleton/lib/twitteroauth-master/autoload.php';
    $consumer_key = 'REDACTED';
    $consumer_secret = 'REDACTED';
    $access_token = 'REDACTED';
    $access_token_secret = 'REDACTED';
    $connection = new TwitterOAuth($consumer_key, $consumer_secret, $access_token, $access_token_secret);
    $trends = $connection->get('trends/place', array('id' => $_GET['location-code']));
    if (strpos($trends, 'Rate limit exceeded') !== false) {
        if (file_exists($cache_file)) {
            $data = unserialize(file_get_contents($cache_file));
            $trends = $data['trends'];
            echo $trends;
        }
    } else {
        $data = array('trends' => json_encode($trends), 'timestamp' => time());
        file_put_contents($cache_file, serialize($data));
        echo json_encode($trends);
    }
}

Structure: Cache Class

You already noted it yourself, you have some duplication. You could get rid of it by introducing some loadTweetFromCache function, but ideally, you would extract the whole cache handling code to its own class (and maybe have an additional twitter specific cache function or class).

Right now, if you want to change how the cache works, you would need to do that all over the place. And if you want to cache other things, you would again duplicate code.

So instead, create a Cache class that can load and store cache entries.

Structure: Logic

  Wouldn't it be better if the code tried to get the trending topics directly from Twitter, resorting to the cache if it can't right off the bat?

It really depends. You would still spam twitter with requests if you handled it like that, so it's probably not the best approach.

Still, it takes a while to see when what is happening in your code, because of the nested ifs, the exit, and the check for the trends variable.

Because of this nesting, it's also easy to miss errors. For example: If your cache file doesn't exit, and the rate limit is exceeded, nothing happens. There is no feedback for this at all.

I would probably go with a structure like this:

// the used variables are either class variables or just passed as arguments
function getTwitterTrends() { // throws some kind of exception if trends could not be loaded from cache
    if (!$twitterCache->isStale()) {
        return $twitterCache->getTrends();
    }
    try {
        $trends = $twitterAPI->getTrends();
    } catch (RateLimitException $rle) {
        return $twitterCache->getTrends(); // couldn't retrieve new trends, use stale cache
    }
    $twitterCache->storeTrends($trends);
}

Now it's pretty clear when what is happening (of course, you have a lot more function calls, but the code clarity should be worth it).

Security

You are open to directory traversal via location-code.

In your case, that isn't all that bad, but it should still be fixed. What can happen is:

An attacker can read parts of files that have a .data extension and properly deserialize (not likely).
An attacker can check for the existence of any .data files (which may gain them some system knowledge or may aid in further attacks, but which isn't that serious).
If you have a file upload that allows the uploading of files with a .data extension an attacker can gain object injection (which may be serious, depending on the rest of your code).

Note that the .data extension limitation does not apply to old PHP versions that are vulnerable to null byte poisoning or path truncation.

Also, your echo is vulnerable to XSS.


Group a list of `dict` by all keys except one I have a function that I use to convert a list of dictionaries into a list of tuples, similar to itertools.groupby() would do in an ideal world.  The goal is to make a list of {unique-dict} => [list of values].  I'm having a hard time explaining it, so I hope the example makes it clear.

I couldn't find a thing on google, so I wrote it pretty quickly, and I am wondering if there is a better way to do this (or a standard library even)

Naming things is hard... what is a better one?
This is a task I performed fairly often when receiving user input
The keys in each dict may not be consistent (some may be missing)
It is OK to destroy or mangle the original input list & items
Validation is done before calling, so 'key_field' is always present
It is expected that values can be duplicated in the list.

{ status: 1 } => [1,2,3,4,1,2] is ok, if the values are actually present twice.

Simple little python function:

def group_by_excluding_key(list_of_dicts, key_field):
    """
    Takes a list of `dict` items and groups by ALL KEYS in the dict EXCEPT the key_field.
    :param list_of_dicts: List of dicts to group
    :param key_field: key field in dict which should be excluded from the grouping
    """

    output = []

    for item in list_of_dicts:
        found = False
        item_key = item.pop(key_field)

        for existing_group, found_keys in output:
            if existing_group.viewitems() == item.viewitems():
                found_keys.append(item_key)
                found = True
                break

        if not found:
            output.append((item, [item_key]))

    return output

Example Input/Output

from pprint import pprint

data = [
    {'id': 1, 'status': 1, 'product': 1},
    {'id': 2, 'status': 1, 'product': 1},
    {'id': 7, 'status': 1, 'product': 2},
    {'id': 9, 'status': 1, 'product': 2},
    {'id': 3, 'status': 1, 'product': 1},
    {'id': 4, 'status': 1, 'product': 1},
    {'id': 8, 'status': 1, 'product': 2},
    {'id': 1, 'status': 1, 'product': 1},
]

results = group_by_excluding_key(data, 'id')

pprint(results)

# [({u'product': 1, u'status': 1}, [1, 2, 3, 4, 1]),
#  ({u'product': 2, u'status': 1}, [7, 9, 8])]

Your code is pretty good, there is one thing that I would add,
the for-else keyword, as this gets rid of the found variable.
Which honestly is just noise.
This is as if a for loop runs completely without breaking then the else will run too. But if it breaks then it won't run the else.
This can leave you with:

def group_by_excluding_key(list_of_dicts, key_field):
    """
    Takes a list of `dict` items and groups by ALL KEYS in the dict EXCEPT the key_field.
    :param list_of_dicts: List of dicts to group
    :param key_field: key field in dict which should be excluded from the grouping
    """
    output = []
    for item in list_of_dicts:
        item_key = item.pop(key_field)
        for existing_group, found_keys in output:
            if existing_group.viewitems() == item.viewitems():
                found_keys.append(item_key)
                break
        else:
            output.append((item, [item_key]))
    return output

Other than that your code is good.

But if I were to were to write this, I'd prefer a very small solution.
Lets say dicts are hash able, what you want is a dictionary that has the modified item as the key, and the popped item_key as the value.
This obviously has two down-sides, it's not ordered, and dicts aren't hash able.
Both easily solved with collections.OrderedDict and tuple(dict.items()).
And so can result in:

from collections import OrderedDict

def group_by_excluding_key(list_of_dicts, key_field):
    """
    Takes a list of `dict` items and groups by ALL KEYS in the dict EXCEPT the key_field.
    :param list_of_dicts: List of dicts to group
    :param key_field: key field in dict which should be excluded from the grouping
    """
    output = OrderedDict()
    for item in list_of_dicts:
        key = item.pop(key_field)
        output.setdefault(tuple(item.items()), []).append(key)
    return [(dict(key), value) for key, value in output.items()]

This has the benefit of moving the for loop into the OrderedDict, and possibly getting \$O(1)\$ key lookup, but requires you to change the type of all the keys, twice.

I know you didn't ask for a performance review, but the performance difference between my code and your code can be tested with the following. The comments are my functions run time over yours as a percentage, so if mine took 0.8s and yours 3.3s then it'll be 24%, followed by how long it took my function to run.

from timeit import timeit
from itertools import count

# 240%, 0.1s
c = count(1)
l = [{'status': 1, 'product': i, 'id': next(c)} for i in range(10)]
print(timeit('fn({!r}, "id")'.format(l), 'from __main__ import group_by_excluding_key_dict as fn', number=1000))
print(timeit('fn({!r}, "id")'.format(l), 'from __main__ import group_by_excluding_key as fn', number=1000))

# 25%, 0.8s
c = count(1)
l = [{'status': 1, 'product': i, 'id': next(c)} for i in range(100)]
print(timeit('fn({!r}, "id")'.format(l), 'from __main__ import group_by_excluding_key_dict as fn', number=1000))
print(timeit('fn({!r}, "id")'.format(l), 'from __main__ import group_by_excluding_key as fn', number=1000))

# 28%, 0.8s
c = count(1)
l = [{'status': i, 'product': j, 'id': next(c)} for i in range(10) for j in range(10)]
print(timeit('fn({!r}, "id")'.format(l), 'from __main__ import group_by_excluding_key_dict as fn', number=1000))
print(timeit('fn({!r}, "id")'.format(l), 'from __main__ import group_by_excluding_key as fn', number=1000))

# 0.4%, 0.9s
c = count(1)
l = [{'status': i, 'product': j, 'id': next(c)} for i in range(100) for j in range(100)]
print(timeit('fn({!r}, "id")'.format(l), 'from __main__ import group_by_excluding_key_dict as fn', number=10))
print(timeit('fn({!r}, "id")'.format(l), 'from __main__ import group_by_excluding_key as fn', number=10))


Annotate a class's important string value via an attribute I have some Field objects that need to be serialized. Their primary string identifier is called a Tag and starts with a number, so it's not suitable for use as a class name. Thus, for a field bearing tag 32A, I am using the name Field32A. However, during serialization, I need access to the literal string value "32A".

I don't think it's a good idea to convert the name of the class to a string and substring the 32A off the end (though I'm open to hearing argument). And, instead of hard-coding this string in the serialization logic somewhere deep in the class, I thought of putting the string in an attribute that can decorate the class itself, in close proximity to the class name in the source code, so future developers who (say) copy this class to a new one won't miss the need to change the class name and the string representing the tag value.

Note: the FieldTag attribute on the FieldBase class is just there to prove that we’re getting the right fieldtag (and in some cases I did incorrectly get the base class field tag, such as when it was omitted on the derived class, until I put the inherit: false in there).

The magic here is occurring inside the Field32A.ToString method where it refers to FieldTag, which goes to the property on the base class and gets the value from the attribute. Do you think this is a good idea?

Run the following code live in a DotNetFiddle

[FieldTag("Base")]
public abstract class FieldBase {
    public string FieldTag {
        get {
            FieldTagAttribute fieldTagAttribute = (FieldTagAttribute) (
                GetType().GetCustomAttribute(typeof(FieldTagAttribute), inherit: false)
            );

            if (fieldTagAttribute == null) {
            throw new InvalidOperationException(String.Format(
               "Class {0} has no attribute [FieldTag]. "
               + "Please add the attribute to this class.", GetType()));
            }

            return fieldTagAttribute.FieldTag;
        }
    }
}

public class FieldTagAttribute : Attribute {
   public FieldTagAttribute(string fieldTag) {
       FieldTag = fieldTag;
   }

   public string FieldTag { get; set; }
}

[FieldTag("32A")]
public class Field32A : FieldBase {
    public string Value{ get; set; }
    public override string ToString() {
        return ":" + FieldTag + ":" + Value;
    }
}

void Main() {
    Field32A field32A = new Field32A { Value = "12345" };
    field32A.ToString().Dump(); // :32A:12345
}

It's considered bad form for a property getter to throw an exception. I would do so in a protected constructor of your FieldBase class. At that point, you can cache the retrieved value in a class member and not have to do any reflection in the property getter at all.

I'm also a fan of immutable semantics where available and appropriate and have modified the attribute itself to be such:

[FieldTag("Base")]
public abstract class FieldBase {
    protected FieldBase() {
        FieldTagAttribute fieldTagAttribute = (FieldTagAttribute) (GetType().GetCustomAttribute(typeof(FieldTagAttribute), false));

        if (fieldTagAttribute == null) {
            throw new InvalidOperationException(String.Format("Class {0} has no attribute [FieldTag]. Please add the attribute to this class.", GetType()));
        }

        s_FieldTag = fieldTagAttribute.FieldTag;
    }

    private readonly string s_FieldTag;

    public string FieldTag { get { return s_FieldTag; } }
}

[System.AttributeUsageAttribute(System.AttributeTargets.Class, Inherited = false)]
public sealed class FieldTagAttribute : Attribute {
   public FieldTagAttribute(string fieldTag) {
       s_FieldTag = fieldTag;
   }

   private readonly string s_FieldTag;

   public string FieldTag { get { return s_FieldTag; } }
}

[FieldTag("32A")]
public class Field32A : FieldBase {
    public string Value { get; set; }
    public override string ToString() {
        return ":" + FieldTag + ":" + Value;
    }
}

public class Program {
    public static void Main() {
        Field32A field32A = new Field32A { Value = "12345" };
        field32A.ToString().Dump();
    }
}


Creating new PHP pages (pagination) I have a MySQL database, with a table called articles. There is a column in articles for its id, name, date_created, etc.

I want to create a page for each article, for example:

  example.com/article.php?id=1

  example.com/article.php?id=2

  ...

My code works right now, but is there anything I should do differently? I'm worried about the safety of the code. Is there a better way to validate/sanitize the id?

<?php
        require_once('../mysqli_connect.php');

        if(isset($_GET['id'])){

            // Replace
            $idv = preg_replace("#[^0-9]#","", $_GET['id']);

            $query = "SELECT name, description, date, id, date_created
            FROM articles WHERE id = $idv LIMIT 1";

            $result = mysqli_query($dbc, $query);

            if ($result){

                $row = mysqli_fetch_assoc($result);

                if(!$row)
                {
                    //Row doesnt exist
                    //Redirect user somewhere else
                }
                else
                {
                    // echo blog post, with html
                }
            }

            else
            {
                echo "Couldn't issue database query
";
                echo '
' . mysqli_error($dbc) . '
';
                // Redirect user
            }

        }

        else
        {
            //Redirect user
        }

        mysqli_close($dbc);

    ?>

In this case you just want your input to be an integer (or a long).
So using the regex is one idea.

But you should also consider malicious users. What if you try

  example.com/article.php?id=1000000000000000000000000000

Which is too big for int/bigint. When the exception is not caught well and your error pages expose information. Worst case this may expose information about the underlying directory structure. Which can be used by hackers while doing other attacks. And even when this is not the case, passing through exceptions are not a best practice.

I would rather use a built-in php method for that like intval.

As an addition to this, you can also use prepared statements which is another very good way to protect against sql injections.

$idv = intval($_GET['id']);
if($idv > 0)
{
    $stmt = $mysqli->prepare("SELECT name, description, date, id, date_created FROM articles WHERE id = ? LIMIT 1");
    $stmt->bind_param("i", $idv);
    $stmt->execute();
    $stmt->bind_result($result);
    $stmt->fetch();
    $stmt->close();
}

ref:

http://php.net/manual/en/function.intval.php
http://www.w3schools.com/php/php_mysql_prepared_statements.asp
https://phpbestpractices.org/


Simple Brainfuck Interpreter in C This is a simple interpreter for the esoteric brainfuck programming language, written in C. It is one of my personal projects and I would love to receive expert advice.
Behavior

All characters except the eight commands ><+-.,[] are ignored and
treated as comments.

The array has 30,000 cells and each cell in the array contains a byte
(most commonly 8 bits).

Moving the pointer outside the bounds of the array will result in
undefined behavior.

Running
After successfully installing the interpreter, you can run it:
brain <file>

This is an example of running brain:
brain example.bf

You can visit the Git Repository at gitlab.com/andy-sb/brain for building/compiling instructions and further information.
Code
In brain.c I have:
#include <stdio.h>
#include <stdlib.h>

void goto_open(char **codep)
{
    int search = 1;
    while (search > 0) {
        --*codep;
        switch (**codep) {
        case '[':
            --search;
            break;
        case ']':
            ++search;
            break;
        default:
            break;
        }
    }
}

void goto_close(char **codep)
{
    int search = 1;
    while (search > 0) {
        ++*codep;
        switch (**codep) {
        case '[':
            ++search;
            break;
        case ']':
            --search;
            break;
        default:
            break;
        }
    }
}

void exec_command(char **codep, char **memp)
{
    switch (**codep) {
    case '>':
        ++*memp;
        break;
    case '<':
        --*memp;
        break;
    case '+':
        ++**memp;
        break;
    case '-':
        --**memp;
        break;
    case '.':
        fputc(**memp, stdout);
        break;
    case ',':
        **memp = fgetc(stdin);
        break;
    case '[':
        if (!**memp)
            goto_close(codep);
        break;
    case ']':
        if (**memp)
            goto_open(codep);
        break;
    default:
        break;
    }
}

void interpret(FILE *fp)
{
    fseek(fp, 0, SEEK_END);
    long fsize = ftell(fp);
    fseek(fp, 0, SEEK_SET);

    char code[fsize];
    char *codep = code;
    fread(code, 1, fsize, fp);

    char mem[30000] = {0};
    char *memp = mem;

    while (*codep != '\0') {
        exec_command(&codep, &memp);
        ++codep;
    }
}

int main(int argc, char *argv[])
{
    if (argc != 2) {
        fprintf(stderr, "Usage: %s <file>\n", argv[0]);
        exit(EXIT_FAILURE);
    }

    FILE *fp = fopen(argv[1], "r");
    if (!fp) {
        fprintf(stderr, "Error: failed to open file '%s'\n", argv[1]);
        exit(EXIT_FAILURE);
    }

    interpret(fp);

    fclose(fp);
    return EXIT_SUCCESS;
}

The code is easy to read and to follow. There isn't a global in sight, the indentation is consistent, you use EXIT_SUCCESS and EXIT_FAILURE, and the control flow is simple. Therefore I have no comments about superficial aspects of style.

However, there is a bug in interpret: You are reading uninitialized memory. The code assumes that code will end in a null character; this is not usually the case. Keep in mind that ftell, fseek, fread do not account for a terminating \0.
Most of the time, fsize will be the number of characters in the file. If the file happens to end in \0, all is well. But if it ends in some other fashion, then code[fsize - 1] (the last valid index of code) will not be \0. Hence codep will begin wandering into memory outside of code until it chances upon a 0.
In C, it is valid to have a pointer to one element past the end of an array. In other words, there is no issue having codep > code + fsize. The problem occurs when codep is dereferenced, since the contents of *codep is undefined.
You can fix this by reserving space for a null character and adding it yourself.

After fixing the problem, there are some other things to be careful about. You check the the return value of fopen, but you don't do the same for other input/output functions. For example, fgetc's result is assigned to a cell blindly, even though fgetc does not return a char. It returns an int, which might be EOF; you should check for and handle this case, as long as the type of your cell is char. (See also the remarks on parameterization below.)

Function name
Error return

fputc
EOF

fgetc
EOF

fseek
a nonzero value

ftell
-1L

fread
a value less than the third argument in the call

fprintf
a negative value

fclose
EOF

My (somewhat controversial) stance on error checking is to do it for any function that can fail. Most people would probably not check fputc and fprintf, even if they check the others. I certainly don't object to this, but it isn't my personal choice.
In each failure case, you should exit(EXIT_FAILURE), probably after printing an indication of what happened. The perror function might be useful.

Why not perform bounds checking to avoid undefined behavior if the pointer moves outside the memory? You can print an error message and leave the pointer unchanged. A similar case to look out for is when a [ or a ] is missing. One way to handle bounds checking is to use offsets (probably of type size_t) instead of real pointers.

I would also suggest parameterizing the program a bit more. Your specification does say that the memory is 30 000 cells in size, but why not give the 30000 a symbolic name? But I think the most interesting aspect is the types you use.
For the most part, char works great as a cell type. However, there are two properties to note: (1) char might be wider than eight bits; (2) char might be signed or unsigned. The former is much less important than the latter. Because the signedness of char is uncertain, I would suggest using a fixed-width and fixed-signedness type.
Regardless of the underlying type, you should probably give a meaningful name to it with typedef—perhaps cell_type. I recommend doing this even if you stick with char.
Most brainfuck interpreters use signed, eight-bit cells. One choice of type is naturally int8_t, but such a type may not be provided by an implementation. You can test for its presence by checking whether INT8_WIDTH is defined. Alternatively, you could use int_least8_t or int_fast8_t, both of which are guaranteed to be provided by a conforming C implementation.
You should pay attention to overflow in memory cells, since overflow of signed numbers in C is undefined behavior. The simplest thing to do is to use unsigned types (uint8_t, uint_least8_t, uint_fast8_t), which wrap on overflow.

Finally, I should address your method of reading the input file. It is simple to read it all in at once, but ftell and fseek might fail, and you're wasting memory. An alternative is to store only the text associated with currently active loops. This is much more complicated to implement, however, especially if you want to allow loops to be arbitrarily sized.


Unreliable shared memory circular buffer with read-only consumer This code is slightly platform dependent but should be pretty easy to port. The goal with this code was to create a circular buffer where the consumer could be limited to read-only access of the buffer (the test uses threads but it could be ported to processes fairly easily.)

I think I probably made at least one mistake in this code and I'm not even sure it is technically possible to implement in a standards compliant way.

#define _GNU_SOURCE

#include <sched.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
#include <pthread.h>

#include <xmmintrin.h>

#define BUFFER_SIZE 64U
#define PACKET_SIZE 128U

#define CUTOFF 2147483648U

typedef uint32_t seqno;

struct packet_header {
        seqno production_count;
};

struct packet_payload {
        char bytes[PACKET_SIZE - sizeof (struct packet_header)];
};

union packet {
        struct {
                struct packet_header header;
                struct packet_payload payload;
        } format;
        char bytes[PACKET_SIZE];
};

static union packet volatile buffer[BUFFER_SIZE] = {{.format.header.production_count = -1}};

static void *producer(void *foo)
{
        seqno producer_count = 0U;

        for (;;) {
                uint64_t payload_number = random();

                struct packet_payload payload = {0};
                memcpy(payload.bytes, &payload_number, sizeof payload_number);

                __atomic_store_n(&buffer[producer_count % BUFFER_SIZE].format.header.production_count, -1, __ATOMIC_SEQ_CST);

                /* This store needs to be SEQ_CST to prevent the write moving before it (whether via the compiler or the CPU */

                buffer[producer_count % BUFFER_SIZE].format.payload = payload;

                __atomic_thread_fence(__ATOMIC_RELEASE);

                __atomic_store_n(&buffer[producer_count % BUFFER_SIZE].format.header.production_count, producer_count, __ATOMIC_RELEASE);

#if defined LOG
                fprintf(stdout, "production count: %u, payload: %lu, produced\n", producer_count, payload_number);
#endif

                producer_count = (producer_count + 1U) % CUTOFF;
        }
}

static void *consumer(void *foo)
{
        seqno consumer_count = 0U;

        for (;;) {
                seqno count = __atomic_load_n(&buffer[consumer_count % BUFFER_SIZE].format.header.production_count, __ATOMIC_ACQUIRE);

                if ((seqno)-1 == count) {
                        /* the message hasn't been completed yet */
                        _mm_pause();
                        continue;
                }

                __atomic_thread_fence(__ATOMIC_ACQUIRE);

                struct packet_payload payload = buffer[consumer_count % BUFFER_SIZE].format.payload;

                /* Note that the double check needs to be SEQ_CST to prevent the read moving after it (whether via the compiler or the CPU */

                if (count != __atomic_load_n(&buffer[consumer_count % BUFFER_SIZE].format.header.production_count, __ATOMIC_SEQ_CST)) {
                        /* torn read, skip packet */
                        consumer_count = (count + 1U) % CUTOFF;
                        continue;
                }

                if (consumer_count > count) {
                        /* we have incremented the consumer count enough to catch up to the producer */
                        _mm_pause();
                        continue;
                }

                uint64_t payload_number;
                memcpy(&payload_number, payload.bytes, sizeof payload_number);

#if defined LOG
                fprintf(stderr, "production count: %u, payload: %lu, consumed\n", count, payload_number);
#endif

                consumer_count = (count + 1U) % CUTOFF;
        }
}

int main(void)
{
        pthread_t xx;
        pthread_create(&xx, 0, producer, 0);
        pthread_create(&xx, 0, consumer, 0);

        pthread_exit(0);
}

Merge cases
In the consumer, you have two separate checks:

            if ((seqno)-1 == count) {
                    /* the message hasn't been completed yet */
                    _mm_pause();
                    continue;
            }

and later:

            if (consumer_count > count) {
                    /* we have incremented the consumer count enough to catch up to the producer */
                    _mm_pause();
                    continue;
            }

Since consumer_count doesn't change in between those two blocks of code, you could move the second check up and merge it with the first one:
            if ((seqno)-1 == count || consumer_count > count) {
                    /* the message hasn't been completed yet, or we have */
                    /* incremented the consumer count enough to catch up to the producer */
                    _mm_pause();
                    continue;
            }

Moving the second check up could actually help you avoid dropping a packet that didn't need to be dropped.


How to make this csv-reading PHP code more efficient? I have the following code that reads a .csv file that than outputs the result in the form of a HTML table:

$fhdrop = fopen("ac.csv", "r");

while (!feof($fhdrop) ) {

$at[] = fgetcsv($fhdrop, 1024);

}

$fhdrop2 = fopen("rc.csv", "r");

while (!feof($fhdrop2) ) {

$at2[] = fgetcsv($fhdrop2, 1024);
}
?>

a
b
c
d

<?php echo $at[0][0] ?>
<?php echo $at[0][1] ?>
<?php echo number_format($at[0][1]*$at2[0][1],2) ?>
<?php echo number_format($at[0][1]*$at2[1][1],2) ?>

<?php echo $at[1][0] ?>
<?php echo $at[1][1] ?>
<?php echo number_format($at[1][1]*$at2[0][1],2) ?>
<?php echo number_format($at[1][1]*$at2[1][1],2) ?>

<?php echo $at[2][0] ?>
<?php echo $at[2][1] ?>
<?php echo number_format($at[2][1]*$at2[0][1],2) ?>
<?php echo number_format($at[2][1]*$at2[1][1],2) ?>

<?php echo $at[3][0] ?>
<?php echo $at[3][1] ?>
<?php echo number_format($at[3][1]*$at2[0][1],2) ?>
<?php echo number_format($at[3][1]*$at2[1][1],2) ?>

The contents of ac.csv are:

a, 5,
b, 10,
c, 24,
d, 21

The contents of rc.csv are:

not, 1.87,
notatall, 1.78

As you can guess from the original code, I need to do multiplication in the second column.  In the second row and third column, for instance, I need 5*1.87 and 5*1.78. So basically a * not and a * notatall.

My code is not very efficient.  How can I make it more efficient?

Looping on feof is wrong. feof doesn't return return true until after you've hit the end of file. If the read pointer is sitting past the last character and you try to read, feof will return false, but the read will fail. At that point feof will return true.

Example:

1, a, b, c
2, a, b, c
3, a, b, c

Then reading it:

$rows = array();
while (!feof($fh)) {
    $rows[] = fgetcsv($fh);
}
print_r($rows);

A much more natural (and correct) loop structure is as follows:

while (($row = fgetcsv($fh)) !== false) {
    $rows[] = $row;
}

(You can of course use fgetcsv($fh, 1024) instead of fgetcsv($fh). Just an example.)

As far performance goes, you're doing what you're doing as fast as it can be done. In other words, to get a speed increase, the approach has to change, not the implementation.

There are three main approaches that you can take (that can all be combined as desired): use caching, put the HTML output straight into the first loop so you only do one loop instead of two, and try using a fixed field-size format rather than CSV so parsing overhead can be eliminated.

Putting the HTML output straight into the read loop is very gross from a separation of concerns perspective, and the gains would be minimal compared to the code quality hit.

Likewise, changing to a fixed size format would be a fairly minimal performance gain for quite a bit of headache.

Caching is what you need to do. A primitive approach is to just create an on-disk version of the array and read that rather than the CSV. A slightly more complex but much faster approach is to cache the CSV in memory using apc or even something like memcached (memcached is way overkill, as is arguably apc). Yet another approach is cache the output HTML rather than the array. This allows you to serve a static page rather than regenerate it each request. You can't get any faster than that.

Since the idea remains largely the same, I'll show an example of the simplest one to implement: caching the array.

$csvFilePath = "...";
$cacheFilePath = "...";

if (!file_exists($cacheFilePath) || filemtime($csvFilePath) > $cacheFilePath) {
    $fh = fopen($cacheFilePath, 'r');
    $rows = array();
    while (($row = fgetcsv($fh)) !== false) {
        $rows[] = $row;
    }
    file_put_contents($cacheFilePath, serialize($rows));
} else {
    $rows = unserialize(file_get_contents($cacheFilePath));
}

Though this one is very simple, it's also the least effective. I would cache the entire page output. I would probably use something like Zend_Cache or Symfony's caching component to avoid lots of nasty manual output buffer management. Alternatively, you could write your own simple page cache.


Duplicate File Finder in python I just started learning python yesterday, I have prior experience of C++. So I think I am able to get most of it pretty fast. I wrote a duplicate file finder for testing and practicing.

Can you guys look on this and give me some opinion if I am doing this right?

import argparse
import os
import fnmatch
import hashlib

class MyFile:
    partHashSize = 0
    def __init__(self, name):
        self.name = name
        self.size = os.path.getsize(self.name)
        self._processed = False
        self._partHash = None
        self._fullHash = None

    def __str__(self):
        return '{} is {}'.format(self.name, self.size)
    def __repr__(self):
        return '<MyFile {}:{}>'.format(self.name, self.size)

    def _getPartHash(self):
        if self._partHash is None:
            h = hashlib.sha1()
            with open(self.name, 'rb') as f:
                h.update(f.read(MyFile.partHashSize))
            self._partHash = h.hexdigest()
        return self._partHash
    partHash = property(_getPartHash)

    def _getFullHash(self):
        if self._fullHash is None:
            h = hashlib.sha256()
            with open(self.name, 'rb') as f:
                while True:
                    buf = f.read(MyFile.partHashSize)
                    if not buf:
                        break
                    h.update(buf)
                self._fullHash = h.hexdigest()
        return self._fullHash
    fullHash = property(_getFullHash)

    def findDuplicates(self, files):
        if self._processed:
            return list()
        dup = list()
        for f in files:
            if f.size == self.size and not os.path.samefile(f.name, self.name):
                if f.partHash == self.partHash and f.fullHash == self.fullHash:
                    f._processed = True
                    dup.append(f)
        if len(dup) > 0:
            dup.append(self)
        return dup

def sizeParse(szstr):
    suffix = ['GB', 'MB', 'KB', 'B']
    for i, s in enumerate(suffix):
        if szstr.endswith(s):
            return int(szstr[0: len(szstr) - len(s)]) * (1024 ** (len(suffix) - i - 1))
    return int(szstr)

def recursiveDir(dir, pattern, filter):
    l = list()
    for root, dirs, files in os.walk(dir):
        for file in files:
            if fnmatch.fnmatch(file, pattern):
                f = MyFile(os.path.join(root, file))
                if filter(f):
                    l.append(f)
    return l

def getFiles(arg, filter):
    if os.path.isfile(arg):
        return [MyFile(arg)]
    if os.path.isdir(arg):
        return recursiveDir(arg, '*', filter)
    [path, file] = os.path.split(arg)
    if len(path) == 0:
        path = '.'
    return recursiveDir(path, file, filter)

def main():
    parser = argparse.ArgumentParser(description='Duplicate File Finder')
    parser.add_argument('--min-size', dest= 'minsize', default='1KB', help='minimum file size, supports suffixes GB, MB, KB, B')
    parser.add_argument('--max-size', dest= 'maxsize', default='1024GB', help='maximum file size, supports suffixes GB, MB, KB, B')
    parser.add_argument('--hash-size', dest= 'hashsize', default='64KB', help='file hash size used for preliminary checking')
    parser.add_argument('dirs', metavar='dirs', type=str,
                        nargs='*', help='dirs or globs or files, supports glob patterns')
    args = parser.parse_args()
    MyFile.partHashSize = sizeParse(args.hashsize)
    MinimumSize = sizeParse(args.minsize)
    MaximumSize = sizeParse(args.maxsize)

    Files = list()
    [Files.extend(getFiles(x, lambda x: x.size > MinimumSize and x.size < MaximumSize)) for x in args.dirs]

    p = False

    for f in Files:
        dup = f.findDuplicates(Files)
        if len(dup) > 0:
            if not p:
                print("Following are the duplicates:")
                p = True
            print([x.name for x in dup])

if __name__ == '__main__':
    main()

Python has some neat features, some of which might seem familiar from C++ and some not. The Python standard library is also very powerful. These comments are meant to be complementary to the answer by @Reinderlein, I will not repeat the useful advice given there.

You can compare multiple things: MINIMUM_SIZE < x.size < MAXIMUM_SIZE.
The itertools module:

from itertools import chain
files = list(chain.from_iterable(
    get_files(x, lambda x: MINIMUM_SIZE < x.size < MAXIMUM_SIZE)) for x in args.dirs))

Decorators, which makes code involving getters easy with property, which you are already using, but not to it's full potential:

@property
def part_hash(self):
    if self._part_hash is None:
        h = hashlib.sha1()
        with open(self.name, 'rb') as f:
            h.update(f.read(self.part_hash_size))
        self._part_hash = h.hexdigest()
    return self._part_hash

Note that self.part_hash_size is the same as MyFile.part_hash_size, unless you overwrite it in the instance (and be careful of mutating mutable objects). This gives you additional flexibility.
(Python 3.6+) Format strings, which utilize the format syntax and make it even better:

def __str__(self):
    return f"{self.name} is {self.size}"

def __repr__(self):
    return f"<{self.__class__.__name__} {self.name}:{self.size}>"

(Python 3.8+) Assignment expressions, which allow you to shorten some while loops:

@property
def full_hash(self):
    if self._full_hash is None:
        h = hashlib.sha256()
        with open(self.name, 'rb') as f:
            while buf := f.read(self.part_hash_size):
                h.update(buf)
            self._full_hash = h.hexdigest()
    return self._full_hash

Truthiness of non-empty containers: if l is the same as if len(l) > 0 for any container in the standard library (and should also be the same for any custom classes you create).
Modules are automatically in their own namespace. No need to come up with names like MyFile, just call it File. You should avoid overwriting built-in names, but everything else is fair game, since you can always import them from another module and prefix them with the module name.
List comprehensions are nice, but sometimes the functions in operator give you more readability. Not really in this case, but as a demonstration:

from operator import attrgetter
a = [x.name for x in dup]              # in your code
b = [getattr(x, "name") for x in dup]  # if the attribute name is a variable
c = map(attrgetter("name"), dup)       # using operator instead
assert a == b == list(c)

Generators allow you to get rid of storing the full result in a list only to then operate on every item of the list. Instead, generate the next item whenever you are done processing the previous one:

def recursive_dir(dir, pattern, filter):
    for root, dirs, files in os.walk(dir):
        for file in files:
            if fnmatch.fnmatch(file, pattern):
                if filter(f := File(os.path.join(root, file))):
                    yield f

Or, with a generator expression added and using the yield from keyword:

def recursive_dir(dir, pattern, filter):
    for root, dirs, files in os.walk(dir):
        yield from (f for file in files
                    if fnmatch.fnmatch(file, pattern)
                    and filter(f := File(os.path.join(root, file))))

As to your actual algorithm, you want to group together "equal" items, for some measure of equality. Currently you are comparing each file against each other file, so your algorithm is \$\mathcal{O}(n^2)\$. Instead, just define how two files compare and what the hash should be:

def __eq__(self, other):
    if not isinstance(other, self.__class__):
        return False
    return self.part_hash == other.part_hash and self.full_hash == other.full_hash

def __hash__(self):
    return int(self.part_hash, base=16)

Then you can just put them into a dictionary, which is \$\mathcal{O}(n)\$, because you only need to iterate over the files once:

from collections import defaultdict

def groupby_hash(files):
    duplicates = defaultdict(list)
    for f in files:
        duplicates[f].append(f)
    return duplicates

def files_with_duplicates(files):
    groups = groupby_hash(files).values()
    return list(filter(lambda x: len(x) > 1, groups))

This uses the fact that when putting a hashable object into a dictionary, it is not just put into a slot according to its hash. If two objects have the same hash, they are also compared by equality. This way only the part_hash should be used if there are no duplicates of it and the full_hash is used to make sure they are actually full duplicates. Sometimes the full_hash will still be calculated anyway because of regular collisions. You can test that this is the case for example like this:

files = list(recursive_dir(".", "*", lambda f: os.path.isfile(f.name)))
len(files)
# 1448
d = groupby_hash(files)

# There are files which are unique and their `full_hash` has never been computed
sum(map(lambda f: f._full_hash is None and len(d[f]) == 1, files)))
# 759

# The `full_hash` has been computed for all files with duplicates
sum(map(lambda f: f._full_hash is not None and len(d[f]) > 1, files))
# 93
sum(map(lambda f: f._full_hash is None and len(d[f]) > 1, files))
# 0

# But there are some files without duplicates, whose `full_hash` has been computed
sum(map(lambda f: f._full_hash is not None and len(d[f]) == 1, files))
# 596


Calculate the crc32 of the contents of a file using boost I'm computing the crc32 of a given file using boost:

uint32_t crc32(const std::string &fp){
    boost::crc_32_type result;
    std::ifstream is{fp, std::ios::in | std::ios::binary};
    size_t file_size = is.tellg();
    is.seekg(0, std::ios::beg);
    std::vector<char> buf(file_size);
    is.read(&buf[0], file_size);

    result.process_bytes(&buf[0], file_size);
    return result.checksum();
}

I believe this code is correct, but it seems unecssarily verbose, and does 2 passes over the data.

Ideally I'd like to write something like:

uint32_t crc32(const std::string &fp){
    boost::crc_32_type result;
    std::ifstream is{fp, std::ios::in | std::ios::binary};
    size_t file_size = is.tellg();
    result.process_block(std::istreambuf_iterator<char>{is}, {}); //invalid
    return result.checksum();
}

But I'm not sure there's a valid way to convert the iterator to char *.

The final alternative I tried was:

for(uint8_t b : std::istreambuf_iterator<char>{is}){ //invalid
    result.process_byte(b);
}
return result.checksum();

But I'm unclear how to get a range from a istreambuf_iterator.

You seem to be missing a seek to the end.

This will always be zero.

    std::ifstream is{fp, std::ios::in | std::ios::binary};
    size_t file_size = is.tellg();

You need to add a seek to end:

    std::ifstream is{fp, std::ios::in | std::ios::binary};
    is.seekg(0, std::ios::end);
    size_t file_size = is.tellg();

There are other functions that query the file system to get the size. But You should note this does NOT do a scan of the file it just moves the descriptor to the end. So its not like reading the file.

You could just wrap it.

std::size_t fileSize(std::ifstream& file)
{
    std::streampos current = file.tellg();
    file.seek(0, std::ios::end);
    std::size_t result = file.tellg();
    file.seek(current, std::ios::beg);
    return result;
}

Now your function becomes:

std::uint32_t crc32(const std::string &fp)
{
    std::ifstream is{fp, std::ios::in | std::ios::binary};
    std::size_t file_size = fileSize(is);

    std::vector<char> buf(file_size);
    is.read(&buf[0], file_size);

    boost::crc_32_type result;
    result.process_bytes(&buf[0], file_size);

    return result.checksum();
}

The problem with this is that some files are huge. You don't want to read the whole thing into memory like that. So you could define a reasonable size buffer and read chunks at a time.

 std::vector<char>   buffer(4096);

 while(is.read(&buffer[0], buffer.size()))
 {
     std::size_t count = is.gcount();
     result.process_bytes(&buffer[0], count);
 }

Or you could use a memory mapped file:

Its been a while since I did this. Here are some instructions.

https://www.safaribooksonline.com/library/view/linux-system-programming/0596009585/ch04s03.html


Checking if a string is a pangram This checker works perfectly but I have a strong feeling that I could make this script much more compliant with the "Zen of Python", especially having read a book about it recently. There is no doubt that I made this script much longer than needed, so if you see something I can shorten, please say so.

#!/usr/bin/env python

myPhrase = "The quick brown fox jumps over the lazy dog"

def is_pangram(phrase):
    c = 0
    alphabet = "abcdefghijklmnopqrstuvwxyz"
    phraseLetters = ""
    for char in phrase:
        for letter in alphabet:
            if char == letter and char not in phraseLetters:
                phraseLetters += char
    for char in phraseLetters:
        for letter in alphabet:
            if char == letter:
                c += 1
    if c == 26:
        return True
    else:
        print phraseLetters, alphabet
        return False

print is_pangram(myPhrase)

This can be improved and made more Pythonic.  First, there is the in and not in keyword, which you already know of as you used it.  Why not utilize that instead of doing two loops?
You will have to change the logic slightly:
def is_pangram(phrase):
    alphabet = "abcdefghijklmnopqrstuvwxyz"
    phraseLetters = ""
    for char in phrase:
       if char in alphabet:
            phraseLetters += char
    for char in alphabet:
        if char not in phrase:
            print phraseLetters, alphabet
            return False

    return True

I iterate over the letters in phrase, checking if they are in the alphabet, and add them to the phraseLetters string.  Then, I iterate over the alphabet and check whether each character is in the phrase.
This can be simplified even further if you are not interested in printing the letters that are in the phrase.  I would recommend cutting it down even further to this:
def is_pangram(phrase):
    alphabet = "abcdefghijklmnopqrstuvwxyz"
    for char in alphabet:
        if char not in phrase:
            return False

    return True

This way, you can check whether the phrase is a pangram without always printing to the screen, which is good for re-using the code later; you can then write another method to print the strings in the phrase and the alphabet.  Additionally, I would recommend printing the letters in phrase alphabetically so it is easy to see which letter is not in phrase, which will also make it easier for any potential debugging you have to do.
Edit:
Thanks to matsjoyce in the comments, you can simplify it even further to this:
def is_pangram(phrase):
    alphabet = "abcdefghijklmnopqrstuvwxyz"
    return not (set(alphabet) - set(phrase))

This way, you can even return the letters not in phrase for ease of reference by just removing the not keyword, but you would also have to change the name of the method as is_something implies a boolean value being returned.
Additionally, you should probably name alphabet in all caps to signify it is a constant variable.


loop inside loop over one array I have the following code:

const arr = Array.from({length: 5}, (_, i) => i + 1);
const finalArray = [];
for (let i = 0; i < arr.length; i++) {
   for (let j = 0; j < arr.length; j++) {
       finalArray.push(arr[i] + '-' + arr[j])
   }
}

console.log(finalArray);

Which creates an array of range 1 => 5, and returns an array with a combination of all elements in the original array.
This code runs in O(n²), is it possible to make it more efficient?

No
You cannot output \$\Theta(n^2)\$ items in \$o(n^2)\$ time. Output size is a trivial lower bound of any algorithms.


Egg drop problem solution in template meta-programming I have implemented a dynamic programming solution of the egg-dropping problem (this problem, but generalized for any number of eggs and floors):

#include <iostream>
#include <limits.h>

//Optimal sub-structure
//We have K floor and N eggs
//Case1: If egg break of X floor then we have x - 1 floor and N - 1 egg
//Case2: If egg doesn't break at X floor then we have K - X floor and N eggs
//Base Case1: we have 1 egg then answer is K floor
//Base Case2: we have 0 or 1 floor then 0 or 1 is answer

int cache[1234][1234];

int solve(int egg, int floor) {

    if(floor == 0 || floor == 1 || egg == 1)
        return floor;

    if(cache[egg][floor])
        return cache[egg][floor];

    int result = INT_MAX;
    for(int current_floor = 1; current_floor <= floor; ++current_floor) {
        int local = std::max(solve(egg - 1, current_floor - 1), solve(egg, floor - current_floor));
        if(local < result)
            result = local;
    }

    cache[egg][floor] = 1 + result;
    return cache[egg][floor];
}

int main(void) {

    int egg = 0, floor = 0;
    //egg = 2, floor = 10 result = 4
    std::cin >> egg >> floor;
    std::cout << "result is: " << solve(egg, floor) << '\n';

    return 0;
}

Now I want to implement this solution in C++ template meta-programming, but I am really not getting how I can use the result variable from the solve function to the loop function.

#include <iostream>
#include <climits>

template<int egg, int floor>
struct solve;

template<int current_floor, int egg, int floor>
struct loop {
    static const int r1 = solve<egg - 1, current_floor - 1>::result;
    static const int r2 = solve<egg, floor - current_floor>::result;
    static const int local = r1 > r2 ? r1 : r2;
    //Here I have to compare this local variable to result variable of the
    //solve class and store?
    //In other words I have to implement
    //if(local < result)
    //  result = local
    loop<current_floor + 1, egg, floor> l;
};

template<int current_floor, int egg>
struct loop<current_floor, current_floor, egg> {
    static const int r1 = solve<egg - 1, current_floor - 1>::result;
    static const int r2 = solve<egg, floor - current_floor>::result;
    static const int local = r1 > r2 ? r1 : r2;
};

template<int egg, int floor>
struct solve {
    static const int result = loop<1, egg, floor>::result + 1;
};

template<int egg>
struct solve<egg, 0> {
    static const int result = 0;
};

template<int floor>
struct solve<1, floor> {
    static const int result = floor;
};

int main(int argc, char** argv) {
    //main function to check program
    return 0;
}

Template metaprogramming is all about rewriting your program to be functional instead of imperative, and then not worrying about caching since the compiler takes care of that for you. The other important guideline is that the return of a metafunction is a type named type. Template metaprogramming is about types - anything that isn't a type is just really annoying to have to deal with, so stay in the type world for as long as possible.

So how do we write this functionally? Let's look at the loop - which is the only really interesting part of the code:

int result = INT_MAX;
for(int current_floor = 1; current_floor <= floor; ++current_floor) {
    int local = std::max(solve(egg - 1, current_floor - 1), solve(egg, floor - current_floor));
    if(local < result)
        result = local;
}

Rewriting this in Python as a generator comprehension (even if you don't know Python, hopefully this is clear):

result = 1 + min(
           max(solve(egg-1, current-1), solve(egg, floor-current))
           for current in range(1, floor+1)
           )

Going from 1 to floor is a little more awkward than going from 0 to floor-1, so let's just subtract 1 from current everywhere:

result = 1 + min(
           max(solve(egg-1, current), solve(egg, floor-current-1))
           for current in range(floor)
           )

Now, this is the right form for template metaprogramming! So what tools do we need to build this up?

A way to iterate from 0 to floor-1
A way to take the min and max of lots of things at compile time
A way to add one to a type

For the former, we have std::make_index_sequence and the latter two I will leave as an exercise.

Once we have these things though, we directly translate the generator comprehension into actual template code thanks to the power of unpacking variadic templates:

template <int Egg, int Floor, typename = std::make_index_sequence<Floor>>
struct SolveImpl;

template <int Egg, int Floor, size_t... Current>
struct SolveImpl<Egg, Floor, std::index_sequence<Current...>>
{
    using type = add1<                            // result = 1 +
        min<                                      //   min(
            max<                                  //     max(
                Solve<Egg-1, Current>,            //        solve(egg-1, current),
                Solve<Egg, Floor - Current - 1>   //        solve(egg, floor-current-1)
            >...                                  //     ) for current in range(floor)
        >                                         //  )
    >;
};

Dealing with the base cases is just a matter of wrapping with std::conditional:

template <int Egg, int Floor>
struct Solve;

/* the SolveImpl code goes here */

template <int Egg, int Floor>
struct Solve
    : std::conditional_t<
            (Floor == 0 || Floor == 1 || Egg == 1),
            int_<Floor>,
            SolveImpl<Egg, Floor>
            >::type
{ };

Again, leaving out the base case details where Floor == 0, Floor == 1, or Egg == 1 since those are less interesting. As always, with template metaprograms, static_assert is your friend:

static_assert(Solve<2, 10>::value == 4, "!");

Check!


Sieve of Eratosthenes in C++ I'm looking for some feedback on my implementation of the algorithm. How can I improve it? I ran into problems when calculating the larger prime numbers > 46349 due to integer overflow, but fixed that by using sqrt instead of pow.

#include<iostream>
#include<math.h>
using namespace std;

int main(){
    int number;
    cin >> number;
    const int CAP = number;
    bool * prime = new bool[CAP];

    for(int i = 0; i < CAP; i++){ //sets all to true for the marking
        prime[i] = true;
    }

    for(int i = 2; i < number; i++){
        if(i <= sqrt(number) && prime[i] == true){
            for(int j = i*i; j < number; j++){ //if %i == 0 mark false
                if(j % i == 0){               //haven't tried another way
                    prime[j] = false;
                }
            }
        }
    }

    for(int i = 2; i < number; i++){
        if(prime[i] == true){
            cout << i << endl;
        }
    }

    return 0;
}

As 200_success pointed out, you have not implemented a sieve. See Wikipedia's pseudo code for the Sieve of Eratosthenes.

You need to check if reading in a number was actually successful:

if (!(std::cin >> number)) {
    std::cerr << "Invalid number provided" << std::endl;
    return EXIT_FAILURE;
}

(Note that you would need cstdlib for EXIT_FAILURE.)

number is a bad name. It's an int--of course it's a number. What is it's meaning? Get rid of CAP, and name number something more meaningful like maxNumber (or ideally something better -- I'm having a naming mental block).

You should only ever use bare dynamic arrays if you are implementing a container or some other low level structure. They are non-exception safe, and you have to remember to clean up the memory. Just use a pre-allocated vector, and you can retain the same performance (or maybe even better in this relatively rare circumstance).

std::vector<bool> prime(CAP, true);

Note that std::vector<bool> has a specialization that uses packing to save space at the cost of a few extra cycles and some rather odd semantics (you actually get a proxy object from the vector rather than a direct reference to the element).

As Jerry Coffin noted, this task is almost certainly memory constrained rather than CPU, so the higher throughput of bit-based bools should actually provide faster performance.

Also, note that as a bonus on top of automatic memory management, you get to remove your initializing loop.

Though in toy programs it doesn't matter, using namespace std; is considered harmful, and it's a bad habit to form. Instead, use using std::cout;, using std::endl;, etc to only import certain symbols (and do it inside of a function, not at the global level).

It's not technically wrong, but it's much, much more common to use a space between #include and the file:

#include <iostream>

//if %i == 0 mark false this comment says the exact same thing the code does. Either make it much more meaningful (// mark the number non-prime if it is divisible by i), or--better in this situation--just remove it.

Interactive programs should be avoided if at all possible. They cannot be chained with other commands in a scripted fashion, and they are prone to user error. Instead, when you're only accepting one or a few simple user inputs, just use arguments to the program (i.e. use argv to get the maximum number rather than using std::cin).

In C++, if a return value is not specified in main, it is assumed to be EXIT_SUCCESS (0). Because of this, I like to omit return values in main when it's not possible for the program to result in a non successful return. Seeing a return in main makes me immediately wonder if it can fail. (Your program actually should be able to fail, so I would keep the return, you just should also have some error checking on the input reading).


find the longest sequence of consecutive numbers in decreasing order I have a list [5,2,7,4,3,2,0,8, 9,100,99,98,97,93,92]
i need out [100,99,98,97]
but i am getting 4, 3, 2, 100, 99, 98, 97, 93, 92

`arr=[5,2,7,4,3,2,0,8, 9,100,99,98,97,93,92]
 z=[]
 l=[]
 for i in range(len(arr)-1):

      if(arr[i]==arr[i+1]+1):
            if(arr[i] not in z):
                z.append(arr[i])
            if(arr[i+1] not in z):
                z.append(arr[i+1])
      print(z)`

The problem with the algorithm is:
1. there is no logic that recognizes the ending of the on-going consecutive decreasing sequence.
2. there is no logic that compares candidate sequences and tracks the longer one.

If we walk through the code, we can see it encounters a sequence of 4,3,2 starting from 4th iteration and then it encounters a sequence of 100,99,98,97. So the output is expected.

An example of working code:

arr=[5,2,7,4,3,2,0,8, 9,100,99,98,97,93,92]
longest = []
current = []
i = 0
if len(arr < 2):
    print(arr)
    return

while i < len(arr)-1:
   current.append(arr[i])
   j = i + 1
   while j < len(arr) and arr[i] == arr[j] + j-i:
      current.append(arr[j])
      j += 1

   i = j # start a new sequence at index j

   if len(current) > len(longest):
       longest = current
   current = []

print(longest)

There are definitely better ways to implement this in idiomatic python but hopefully, the code above illustrates the algorithm.


Managing configs to use scripts on different computers I have some python scripts to run on different computers. To avoid struggles with different paths and filenames I made specific config-files (DConfig, AConfig and MConfig) and another script that decides which config to use. It works, but I guess, there are better ways to do this. Any ideas?
import os
import socket

hostname_K = 'DESKTOP-K'
hostname_A = 'A'
hostname_M = ''                    # hostname not known for sure

print("start config")
print("cwd: ", os.getcwd())
print("os: ", os.name)
# print(os.getlogin())                # Doesn't work on non-posix

hostname = socket.gethostname()
if hostname == hostname_K:
    import DConfig as Conf
elif hostname == hostname_A:
    import AConfig as Conf
else:
    print("Hostname: {}, guess we are on M".format(hostname))
    import MConfig as Conf

dirData = Conf.dirData
dirScript = Conf.dirScript
pathDB = Conf.pathDB

print("config done.")

How it is used by other scripts:
import config
XMLFILE = config.dirData + 'Tags.xml'
DATABASE = config.pathDB

Here is an example script for one of the config-files.
dirData = '/home/username/path/to/data/'
dirScript = '/home/username/path/to/scripts/'
pathDB = '/home/username/path/to/database.db'

The other scripts are similar.

You have a value (hostname in this case) that you want to use to select among
various data choices (config modules in this case). When you have situations
like this, use a data structure. Sensible
data is always simpler than logic. For example, see Rule 5 from Rob
Pike.
An illustration based on your code:
import socket
import DConfig
import AConfig
import MConfig

configs = {
    'host-D': DConfig,
    'host-A': AConfig,
    'host-M': MConfig,
}

hostname = socket.gethostname()
Conf = configs.get(hostname, MConfig)


User Access Authorization for Web Resources Im refactoring a large internal legacy application and part of the stuff I want to get rid is the  widespread code repetition.

I'm starting with the way the website checks if a User can access a route e.g., '/admin` and which parts should be rendered or not.

I made this little function and works pretty well but wanted to see if you could take a look into it and suggest improvements or point out problems with the way I built it.

Anyway, here's the code:

export default async function isAdmin(alias, policyOverride = null) {
    try {
        var authResult = false;
        const url = 'https://xxx.execute-api.us-west-2.amazonaws.com/local/xxxx/';

        var adminRolePolicy = ['Admin', 'Dev', 'BaseUser']; //Default Admin Policy.

        var rolesBoundToAlias = (await axios.get(url + alias)).data //Fetch roles that the alias holds.

        if(policyOverride){
            adminRolePolicy = policyOverride;
        } //If a new policy is, override the default adminRolePolicy.

        authResult =
            rolesBoundToAlias.some(role => adminRolePolicy.includes(role)); //If alias has any of the roles in the policy grant access.

        return authResult;
    } catch (error) {
        console.error("Error when attempting to authorize user " + alias
            + ". Access for this user has been denied, please try again."
            + "\nError: " + error);
        return false;
    }
}

I added the policyOverride as an optional param so the function can accept an array with different roles... allowing for more flexibility.

One of the most simple applications for this function would be as follows:

import isAdmin from '../utils/authorization.js';
//Other stuff happens here.
//...
isAdmin(alias)
.then((authResult) => {
    if(authResult) {
        //Authorized to view content render this.
    }
    else {
        //Else render a 403 or something...
    }
}

If you're going to use const for some variables, then use that (or let) for all variables. Avoid var unless you have a good reason - e.g. you really need a global variable and/or hoisting.

The variable authResult can be eliminated from function isAdmin since it only gets a default value assigned, which then gets overwritten before it is returned. The function could merely return the value from the call to rolesBoundToAlias.some(). The arrow function passed to .some() could be declared on the previous line if you want, in an effort to decrease line length. Actually you might be able to just use Array.includes() bound to the array of roles instead of an extra arrow function.

The name adminRolePolicy might be better named as DEFAULT_ROLES. I use all caps since it is more like a real constant that wouldn't change much - and could be declared outside the function if deemed appropriate. And while there is likely some overlap, Dev and BaseUser seem separate from the concept of admin.

Instead of calling the second parameter policyOverride, you could call it roles with the default of DEFAULT_ROLES - then there would be no need to assign the parameter to a different variable.


Removing the massive amount of for-loops in this code Are there ways to avoid this triply nested for-loop?

def add_random_fields():
from numpy.random import rand
server = couchdb.Server()
databases = [database for database in server if not database.startswith('_')]
for database in databases:
    for document in couchdb_pager(server[database]):
        if 'results' in server[database][document]:
            for tweet in server[database][document]['results']:
                if tweet and 'rand_num' not in tweet:
                    print document
                    tweet['rand_num'] = rand()
                    server[database].save(tweet)

Update

People suggesting that I use a generator reminded me that I forgot to mention that couchdb_pager is a generator over the list server[database].

There are a few tweaks that can improve this code:

def add_random_fields():
    from numpy.random import rand
    server = couchdb.Server()
    databases = (server[database] for database in server if not database.startswith('_'))
    # immediately fetch the Server object rather then the key,
    # changes to a generator to avoid instantiating all the database objects at once
    for database in databases:
        for document in couchdb_pager(database):
            document = database[document]
            # same here, avoid holding keys when you can have the objects

            for tweet in document.get('results', []):
            # rather then checking for the key, have it return a default that does the same thing
                if tweet and 'rand_num' not in tweet:
                    print document
                    tweet['rand_num'] = rand()
                    database.save(tweet)