Following Repository Pattern properly I am new to learning Repository Pattern. I am developing a site in MVC 4 (I'm new to it as well).

The way I am using Repository pattern as follows:

I created IRepository<T> interface as follows:

public interface IRepository<T>
{
    void Insert(T entity);
    void Delete(T entity);
    IQueryable<T> SearchFor(Expression<Func<T, bool>> predicate);
    IQueryable<T> GetAll();
    T GetById(int id);
}

Then I created a base class which implements the above interface as follows:

public class BaseRepository<TEntity> : IRepository<TEntity> where TEntity : class
{
    private readonly DbContext _dbContext;

    public BaseRepository(DbContext dbContext)
    {
        _dbContext = dbContext;
    }

    public void Insert(TEntity entity)
    {
        _dbContext.Set<TEntity>().Add(entity);
        _dbContext.SaveChanges();
    }

    public void Delete(TEntity entity)
    {
        _dbContext.Set<TEntity>().Remove(entity);
        _dbContext.SaveChanges();
    }

    public IQueryable<TEntity> SearchFor(Expression<Func<TEntity, bool>> predicate)
    {
        return _dbContext.Set<TEntity>().Where(predicate);
    }

    public IQueryable<TEntity> GetAll()
    {
        return _dbContext.Set<TEntity>();
    }

    public TEntity GetById(int id)
    {
        return _dbContext.Set<TEntity>().Find(id);
    }
}

Now for each repository I want to create, I create another interface which contains methods specific to that class. E.g if I want to create a CustomersRepository, I create an interface ICustomersRepository as follows:

public interface ICustomersRepository: IRepository<Customer>
{
    IEnumerable<Customer> GetRecentCustomers();
    Customer GetByName(string customerName);
}

Now I create CustomersRepository class, derive it from BaseRepository and  implement ICustomersRepository. as follows:

public class CustomersRepository : BaseRepository<Customer>, ICustomersRepository
{
    private readonly DbContext _dbContext;

    public CustomersRepository(DbContext dbContext) : base(dbContext)
    {
        _dbContext = dbContext;
    }

    public IEnumerable<Customer> GetRecentCustomers()
    {
        // implementation
    }

    public Customer GetByName(string customerName)
    {
        //
    }
}

Now to create another Repository, I will have to create an interface for it, then create that repository class which will implement BaseRepository<TEntity> class, implement the specific repository interface.

Issues:

This is not the proper way. But I use it as I get a BaseRepository<TEntity> which gives me many functions implemented.
I have to pass DbContext for each repository class.
not satisfied

Can you please guide me have this code corrected?  I will be using the same approach in almost all of my future projects where I use an ORM.

You can make DbContext protected property in your base implementation, so you don't have to declare it in every single derived class.
You might want to consider extracting Insert and Delete methods to separate interface, because often you want to make some of your repositories readonly.

Implementation may look like that:

interface IRepository<T>
{
    IQueryable<T> SearchFor(Expression<Func<T, bool>> predicate);
    IQueryable<T> GetAll();
    T GetById(int id);
}

interface IEditableRepository<T> : IRepository<T>
{
    void Insert(T entity);
    void Delete(T entity);
}

class ReadOnlyRepositoryBase<T> : IRepository<T>
{...}

class EditableRepositoryBase<T> : ReadOnlyRepositoryBase<T>, IEditableRepository<T>
{...}

this way you can distinct readonly repositories from non-readonly at compilation time.
Passing dbContext to every repository is not normally an issue, because usually in addition to your repositories, you also implement factory object, which creates your repositories for you, so you don't have to call constructor explicitly. This factory can be made a singleton, so you can access it at any point in your code. Alternatively, you can register your repositories in some container, or you can use both approaches.
If you are familiar with inversion of control and dependency injection (because there is definitely a learning curve), your best bet is to use a container.

Here is an example (using Castle Windsor):

public class RepositoryFactory : IDisposable
{
    private IWindsorContainer _container;

    public void Open()
    {
        _container = new WindsorContainer();

        //register dependencies
        _container.Register(Component.For<DbContext>().ImplementedBy<DbContext>());

        //register repositories
        _container.Register(Component.For<ICustomersRepository>().ImplementedBy<CustomersRepository>());
    }

    public TRepository GetRepository<TRepository>() where TRepository : class
    {
        return _container.Resolve<TRepository>();
    }

    public Dispose()
    {
        if (_container == null) return;
        _container.Dispose();
        _container = null;
    }

    public void Close()
    {
        Dispose();
    }
}

Then you can use it like this:

var f = new RepositoryFactory();
f.Open();
var rep = f.GetRepository<ICustomersRepository>();
f.Close();

Alternatively, you can implement your own container using reflection to instantiate your repositories, or you can use a dictionary. Or w/e, really. This implementation is obviously no singleton, but nothing stops you making one, if you feel like it.

Apart from that, your implementation looks fine to me. I didn't quite get your first issue though.


What is the most memory efficient way to load big arrays into PHP script? I have a few big arrays stored in separate files that are included into PHP as and when they are required:

Array File:

<?php
 $cty_arr["af"] = "Afghanistan";
 $cty_arr["ax"] = "Åland Islands";
 $cty_arr["al"] = "Albania";
 $cty_arr["dz"] = "Algeria";
 $cty_arr["as"] = "American Samoa";
...
?>

Main Script:

<?php
 require("/var/www/include/array_file.php");
...
?>

I am not very sure how PHP utilizes memory, but I have this thought that some RAM is allocated to store the big array for each instance that the script is run. Since these arrays are static, could there be a better way of loading such arrays to achieve some memory savings?

I thought about this again. My comments are still valid, so including it in a php file is the cheapest way to get the array into your system and without a shared memory you will not reduce the total memory usage.

But maybe you can optimize it at an higher level of abstraction. Assuming that you always only need some countries/settings/localization in your loaded page and never all, you could split this array into multiple files.

<?php
...
function countryCodeToName($key)
{
    if (isset($this->countries[$key])) return $this->countries[$key];
    $firstCharacter=$key[0]
    include "country_".$firstCharacter.".php"; //similar to your file above
    $this->countries=$this->countries + $cty_arr;
    //some handling if still missing
    //and return ...
}


Calculate duration until some date I use this code to generate time until some date:

 String dateStop = "25/11/17 09:00:00";

    // Custom date format
    SimpleDateFormat format = new SimpleDateFormat("yy/MM/dd HH:mm:ss");

    LocalDateTime d1 = LocalDateTime.now();
    Date d2 = null;
    try
    {
        d2 = format.parse(dateStop);
    }
    catch (ParseException e)
    {
        e.printStackTrace();
    }

    // Get msec from each, and subtract.
    long diff = d2.getTime() - d1.getTime();

    long days = TimeUnit.MILLISECONDS.toDays(diff);
    long remainingHoursInMillis = diff - TimeUnit.DAYS.toMillis(days);
    long hours = TimeUnit.MILLISECONDS.toHours(remainingHoursInMillis);
    long remainingMinutesInMillis = remainingHoursInMillis - TimeUnit.HOURS.toMillis(hours);
    long minutes = TimeUnit.MILLISECONDS.toMinutes(remainingMinutesInMillis);
    long remainingSecondsInMillis = remainingMinutesInMillis - TimeUnit.MINUTES.toMillis(minutes);
    long seconds = TimeUnit.MILLISECONDS.toSeconds(remainingSecondsInMillis);

    System.out.println("Days: " + days + ", hours: " + hours + ", minutes: " + minutes + ", seconds: " + seconds);

How I can get the same result but with much more less code?

You do not need to use Date, parse, Exception, .... You only need LocalDateTime :

LocalDateTime d1 = LocalDateTime.now();
LocalDateTime d2 = LocalDateTime.parse("25/11/17 09:00:00", DateTimeFormatter.ofPattern("yy/MM/dd HH:mm:ss"));

long days = d1.until(d2, ChronoUnit.DAYS);
d1 = d1.plusDays(days);
long hours = d1.until(d2, ChronoUnit.HOURS);
d1 = d1.plusHours(hours);
long minutes = d1.until(d2, ChronoUnit.MINUTES);
d1 = d1.plusMinutes(minutes);
long seconds = d1.until(d2, ChronoUnit.SECONDS);

System.out.println(days + " days " + hours + " hours " + minutes + " minutes " +
                                                         seconds + " seconds.");

Step by step, from large precision to small, it will look the difference with this precision and then add it to be nearer of the target


Use of metaclass python to override __init__ and instantiate the class later on I m writting some API which would use configuration-like arguments.

Here is an example configuration which will be used by my API

quail.run(
    quail.SolutionPacked(path='Allum1'),
    quail.Installer(
        name='Allum1',
        icon='icon.jpeg',
        binary='allum1',
        console=True
    ),
    quail.builder.Builder(
        quail.builder.CmdIcon('icon.ico'),
        quail.builder.CmdZip(solution_path, 'solution.zip')
    )
)

The only problem with that: we will always instantiate, even when we won't use the instance, (for example if we want to uninstall we won't use the install class)

One way around I have found is to use metaclass and override __call__ to get an "EggInstance" so I can instantiate the actual class later on.

Example implementation:

import six

class InstanceEgg:
    def __init__(self, cls, *args, **kwargs):
        self._cls = cls
        self._args = args
        self._kwargs = kwargs

    def __call__(self):
        return self._cls(_get_instance=True, *self._args, **self._kwargs)

class Egg(type):
    def __call__(cls, *args, **kwargs):
        get_instance = kwargs.pop("_get_instance", False)
        if get_instance:
            return super().__call__(*args, **kwargs)
        return InstanceEgg(cls, *args, **kwargs)

@six.add_metaclass(Egg)
class Test:
    def __init__(self, a, b):
        print("init %d %d" % (a, b))

Example use:

egg = Test(1, 2)
print("got egg")
f = egg()

Output:

got egg
init 1 2

Do you think it is acceptable to use metaclass for this purpose?

And what do you recommend?

From the looks of it you have objects that you only want to partially create, when you create them the first time.

To do this you can use functools.partial:

quail.run(
    partial(quail.SolutionPacked, path='Allum1'),
    partial(
        quail.Installer,
        name='Allum1',
        icon='icon.jpeg',
        binary='allum1',
        console=True
    ),
    partial(
        quail.builder.Builder,
        partial(quail.builder.CmdIcon, 'icon.ico'),
        partial(quail.builder.CmdZip, solution_path, 'solution.zip')
    )
)

Yeah, this isn't really that attractive. However it is what you want.

And so you can use a metaclass to perform this action automagically.
I'd remove the _get_instance stuff from your Egg, remove InstanceEgg and use a partial call.

from functools import partial

class PartialInit(type):
    def __call__(cls, *args, **kwargs):
        return partial(super().__call__, *args, **kwargs)

@six.add_metaclass(PartialInit)
class Test:
    def __init__(self, a, b):
        print('init {} {}'.format(a, b))

t = Test('a')
print('Made t: {}'.format(t))
t('b')


how to classify tweets in python based on handful of domains I am currently working on a project that classifies tweets based on certain categories of information they belong to (7 given categories for his project).

For example, a tweet with the keywords "I think NY should ban smoking" should be classified as a tweet in the "Pollution" category with negative sentiment.

I have been able to get the sentiment analysis to work somewhat, but need some help with getting the category part to work. I am also open to all solutions.

My code so far is the stream.py module. And the following command to get live twitter data into a text file: python stream.py > output.txt:

import oauth2 as oauth
import urllib2 as urllib

api_key = 'xx'
api_secret = 'xx'

access_token_key = 'x-x'
access_token_secret = 'x'

_debug = 0

oauth_token    = oauth.Token(key=access_token_key, secret=access_token_secret)
oauth_consumer = oauth.Consumer(key=api_key, secret=api_secret)

signature_method_hmac_sha1 = oauth.SignatureMethod_HMAC_SHA1()

http_method = "GET"

http_handler  = urllib.HTTPHandler(debuglevel=_debug)
https_handler = urllib.HTTPSHandler(debuglevel=_debug)

'''
Construct, sign, and open a twitter request
using the hard-coded credentials above.
'''
def twitterreq(url, method, parameters):
  req = oauth.Request.from_consumer_and_token(oauth_consumer,
                                             token=oauth_token,
                                             http_method=http_method,
                                             http_url=url,
                                             parameters=parameters)

  req.sign_request(signature_method_hmac_sha1, oauth_consumer, oauth_token)

  headers = req.to_header()

  if http_method == "POST":
    encoded_post_data = req.to_postdata()
  else:
    encoded_post_data = None
    url = req.to_url()

  opener = urllib.OpenerDirector()
  opener.add_handler(http_handler)
  opener.add_handler(https_handler)

  response = opener.open(url, encoded_post_data)

  return response
#locations=-74,40,-73,41
def fetchsamples():
  url = "https://stream.twitter.com/1.1/statuses/filter.json?track=money&locations=-74,40,-73,41"
  parameters = []
  response = twitterreq(url, "POST", parameters)
  for line in response:
    print(line.strip())

if __name__ == '__main__':
  fetchsamples()

The sentiment of a tweet is computed as the sum of the sentiment scores for each term in the tweet. Run: python tweet_sentiment.py AFINN-111.txt tweet_file to get the tweet sentiments.

Here is the link for my upload for AFINN-111.txt.http://s000.tinyupload.com/index.php?file_id=62473255612293859764

Here is the code for tweet_sentiment.py

import sys
import json
import ast
import re

def calcScoreFromTerm(termScoreFile):   # returns a dictionary with term-score values
    scores ={}
    for line in termScoreFile:
        term, score = line.split("\t")
        scores[term] = float(score)
    return scores

def getTweetText(tweet_file):   #returns a list of all tweets
    tweets = []
    for line in tweet_file:
        # print line
        jsondata = json.loads(line)
        if "text" in jsondata.keys():
            tweets.append(jsondata["text"])
    tweet_file.close()
    return tweets

def filterTweet(et):
    # Remove punctuations and non-alphanumeric chars from each tweet string
    pattern = re.compile('[^A-Za-z0-9]+')
    et = pattern.sub(' ', et)
    #print encoded_tweet

    words = et.split()

    # Filter unnecessary words
    for w in words:
        if w.startswith("RT") or w.startswith("www") or w.startswith("http"):
            words.remove(w)

    return words

I don't see your sentiment analysis code in your tweet_sentiment.py, so I will not comment on that.

And if I understand correctly, you are trying to build a classifier for text classification based on Twitter data.

So, depending on your data size, here are two things you could try.

Small data size (several tens of thousands or lower): Tdidf + Support Vector Machine, basically convert your text to a numerical vector form representing the existence of certain words along with their importance relative to the text and the whole dataset. Then feed the data into an SVM classifier, lots of tutorials are out there, and here is a good one.
Big data size (hundreds of thousands and more)(and ready to have fun): (word embedding Or one-hot encoding) + (CNN Or RNN)
, word embedding could represent the semantic similarity with others into a vector space model and have a dense representation, a good explanation could be found here. As for using CNN to do text classification you could refer to this and this Text understanding from scratch. and another one for RNN


Project Euler # 20 factorial digit sum in Python n! means n × (n − 1) × ... × 3 × 2 × 1
For example, 10! = 10 × 9 × ... × 3 × 2 × 1 = 3628800,
and the sum of the digits in the number 10! is 3 + 6 + 2 + 8 + 8 + 0 + 0 = 27. Find the sum of the digits in the number 100!

def fact(n):
    """returns factorial of n."""
    if n <= 1:
        return 1
    return n * fact(n - 1)

def count_digits(n):
    """Assumes n > 1.
    returns sum of digits of n's factorial."""
    factorial = fact(n)
    total = 0
    for digit in str(factorial):
        total += int(digit)
    return total

if __name__ == '__main__':
    print(count_digits(100))

The standardlibrary module math already contains a factorial function. On my machine it is about 20 times faster than your function using n = 100. It also does not suffer from stack size limitations as yours does (try computing fact(3000)).

Alternatively you could learn about memoizing, which will help you in many Project Euler problems. Here it would be useful if you had to evaluate the factorial of many numbers (and even better if the numbers are increasing).

from functools import wraps

def memoize(func):
    cache = func.__cache = {}

    @wraps(func)
    def wrapper(*args, **kwargs):
        key = args, frozenset(kwargs.items())
        if key in cache:
            ret = cache[key]
        else:
            ret = cache[key] = func(*args, **kwargs)
        return ret
    return wrapper

@memoize
def fact(n):
    ...

Note that this decorator only works if your arguments are hashable (so no lists for example).

Since getting the sum of the digits of a number is something you will regularly need for Project Euler problems, you should make it a function on its own, which you put in a utils module, to be reused later:

def digit_sum(n):
    """Return the sum of the digits of `n`"""
    return sum(map(int, str(n)))

This will also be faster than your manual for loop or a list comprehension. Not by much, but it is measurable for very large numbers:

def digit_sum_list_comprehension(n):
    return sum(int(x) for x in str(n))

def digit_sum_for(n):
    total = 0
    for digit in str(n):
        total += int(digit)
    return total


Retrieving database values through properties I have a Settings table which I make frequent DlookUp calls to retrieve values. So I thought instead of having to write Nz(DLookup("FieldName", "TableName", "Condition"), vbNullString) everytime I needed the value, why not wrap it in a property in a standard module.

Public Property Get ActiveKey() As String
    ActiveKey = Nz(DLookup("[_ActiveKey]", "WmSettings"), vbNullString)
End Property

Public Property Let ActiveKey(ByVal pValue As String)
    CurrentDb.Execute StringFormat("UPDATE WmSettings SET [_ActiveKey]='{0}';", pValue), dbFailOnError
End Property

Then I can call it like this:

key_ = Settings.ActiveKey       'I tend to write the module name for clarity

Settings.ActiveKey = "FAD22L"

The same approach could easily apply to an objectFrame/subform scenario.

Public Property Get PageFrame() As Object
    Set PageFrame = Forms.frmMain.SubForm.Form.InnerSubForm
End Property

Public Property Get Page() As Form
    Set Page = Forms.frmMain.SubForm.Form.InnerSubForm.Form
End Property

So my question is, is this approach I've taken considered to be a good practice and does it add a considerable amount of overhead to the application?

Properties in Standard Modules

Standard modules can have Properties, but as @Rubberduck pointed out, they're not necessarily intuitive (although they do force you to qualify a call with the module name, so there's that as a redeeming quality). You could write public methods instead, but you could also create a class, and if you wanted to avoid the need to New it up, you could give that class a Predeclared attribute.  A class would better reflect the stateful nature of the settings, and would give you the flexibility of abstracting the persistence of settings from the retrieval and usage of settings.

Right now, your approach looks up the value from the underlying table every time, and writes it to the table every time. That's time consuming disk access and page locking. It might be better to load the settings on demand, use/change the settings, and then save them on exit.

Variable Names

pValue is presumably short for parameterValue. Why not use the full name, or better still, something like keyName? pValue could be confused for the statistical term P-Value, or a user might infer that the p is Hungarian notation, but guess at what type that notation represents.

SQL Injection

Kudos for using the StringFormat (presumably from @Mat'sMug's CR question), but you're building a SQL statement on the fly, and a malicious user, or inadvertent user could cause an error, or worse, by providing a parameter with a single-quote in the pValue. What happens if the user supplied "', AdminMode = TRUE, AdminName = '"?

At the very least, sanitize your inputs. Better still, parameterize your queries and leave a sign up saying that Johnny DropTables isn't welcome.

LateBound `PageFrame` property

You're explicitly returning an Object, which forces usage of the property to be late-bound. You probably know the type of the return value, why not use it?

If you know the return-type will always be an instance of InnserSubForm, then return that type:

Public Property Get PageFrame() As Form_InnerSubForm
    Set PageFrame = Forms.frmMain.SubForm.Form.InnerSubForm
End Property

If the subform is set dynamically, and you only know the return type will be a Form, then return that base type:

Public Property Get PageFrame() As Access.Form
    Set PageFrame = Forms.frmMain.SubForm.Form.InnerSubForm
End Property

Clarifying the Form type

Access has built-in Form objects, but also allows the use of VBA UserForms. It might be helpful to qualify the type as Access.Form:

Public Property Get Page() As Access.Form

Be careful with properties matching built-in control names

Page is the name of a built-in type, but you're using it to return a Form. That might be confusing for users of the property.

Accessing the `Forms` object

The Forms object extends the names of open forms at run-time. That means you're using late-bound code and you're not getting any Intellisense. Worse still, if the form isn't open, or it's in the wrong state, the call will fail. At the very least, you should have some error handling.

Using the default instance of the form

Access creates a default predeclared instance of a form (much like VBA creates a default predeclared instance of a UserForm), but Access only does this if the form is open. However, it is also possible to create instances of the Access form (and, I'd argue, is the preferable way to deal with Forms and UserForms).

Your code is intimately tied to the default instance, and should perhaps be made capable of handling any instance of the form. See this example to see how a form with a default caption of "foo" is handled by VBA:

  Dim frm As Form_frmMain
  Set frm = New Form_frmMain

  frm.Caption = "Bar"

  'Print the current instance's caption - Bar
  Debug.Print frm.Caption

  'Print the early-bound default instance's caption - Foo
  Debug.Print Form_frmMain.Caption

  'Print the late-bound default instance's caption - Foo
  Debug.Print Forms.frmmain.Caption

`CurrentDb` is a function used like an object

CurrentDB is a function that returns a pointer to a Database. It's a member of the Application class, but note this text from an old KB article:

  The following example attempts to use the CurrentDb function to return a
  pointer to the database that is currently open in Microsoft Access. Because
  the code does not assign that database to an object variable, the pointer
  returned by the CurrentDb function is temporary and becomes invalid after
  the TableDef object is set. Consequently, any later references in your code
  to the TableDef object variable will result in an error.

That is, usage of Database members against CurrentDB, like CurrentDB.Execute or, more correctly, CurrentDB().Execute can be unreliable and result in error Object Invalid or Not Set. You'll have more reliable code if you first assign the result of CurrentDB() to a local variable, and then access the Execute method from that variable.

Dim db As Database
Set db = Application.CurrentDb()
db.Execute StringFormat("UPDATE WmSettings SET [_ActiveKey]='{0}';", pValue), dbFailOnError


Nested state with different many keys I have a nested state which is needed to set its state by a key's value:

state = {
    income_source: {
        type: 'text', label_name: 'Income Source', value: '', helper: 'dropdown',
    },
    employment_status: {
        type: 'text', label_name: 'Employment Status', value: '', helper: 'dropdown',
    },

...

I'm getting a huge list from a get_financial_assessment object and the code becomes messy when I tried to setState in componentDidMount():

async componentDidMount() {
    let { get_financial_assessment } = await DAO.getFinancialAssessment()
    if( get_financial_assessment ) {
        const {
            account_turnover,
            cfd_score,
            education_level,
            employment_industry,
            employment_status,
            estimated_worth,
            financial_information_score,
            income_source,
            net_income,
            occupation,
            source_of_wealth,
            total_score,
            trading_score,
        } = get_financial_assessment;

        this.setState(prevState => ({
            anticipated_account_turnover: {...prevState.anticipated_account_turnover, value: account_turnover},
            occupation: {...prevState.occupation, value: cfd_score},
            level_of_education: {...prevState.level_of_education, value: education_level},
            source_of_wealth: {...prevState.source_of_wealth, value: employment_industry},
            net_annual_income: {...prevState.net_annual_income, value: employment_status},
            estimated_net_worth: {...prevState.estimated_net_worth, value: estimated_worth},
            source_of_wealth: {...prevState.source_of_wealth, value: financial_information_score},
        }));
    } else {
        console.log('nope');
    }
}

Any better way? One possible approach might be, once we get the data, we can make an object and setState the object to the state. Would it be the best possible answer?

First things first, I am against using different names in things that are inherently the same. If you do not have any other option, you could the data coming from the DAO as soon as they are fetched from the server.

If that is not an option, since this code is indeed messy, I would move this logic into a function while indicating the mapping from one state to the other (this is similar to what Redux does with mapStateToProps).

const mapFinancialAssesmentToState = (fa, state) => {
  const mapping = {
    anticipated_account_turnover: fa.account_turnover,
    occupation: fa.cfd_score
    //...
  };
  const nextState = {};
  Object.keys(mapping).forEach(key => {
    nextState[key] = { ...state[key], value: mapping[key] };
  })
  return nextState;
}

async componentDidMount() {
  let { get_financial_assessment } = await DAO.getFinancialAssessment();
  if (get_financial_assessment) {
      this.setState(mapFinancialAssesmentToState(get_financial_assessment, this.state))
  } else {
    console.log('nope');
  }
}


Building a flexible upsert statement for SQL This is a single function in C#. It works fine. My concern is that although I've made the code as short and as multi-functional as I can, what it's doing seems very unclear. It looks like the sort of thing I'm going to come back to in six months and hate myself for writing.

Its purpose is to build and execute generic "upsert" sql statements for a variety of scenarios. Each record that it comes across has a name, an address and a flag to say whether the operation on that record is a delete and whether that record represents someone who's gone away. The former tells us whether we're setting a bit value to true or false, while the latter determines the column name.

How can I make this code clearer without turning it into a mess of flow control?

private void Sprail(FileRow fr, string columnName)
{
    List<Suppression> addressMatches = GetMatchingRecordsFromDb(SanitiseForMySql(fr.CleanedAddress), SanitiseForMySql(fr.CleanedName));

    using (MySqlConnection con = new MySqlConnection(_connectionString))
    {
        string sqlTargetFlag = columnName;
        string sqlTargetValue = "1";
        string sql = "";

        if (fr.IsDeleted)
        {
            sqlTargetValue = "0";
        }

        if (fr.IsGoneaway)
        {
            sqlTargetFlag = sqlTargetFlag + "Goneaway";
        }

        if (string.IsNullOrWhiteSpace(fr.CleanedAddress))
        {
            sql = BuildInsertSql(sqlTargetFlag, fr, sqlTargetValue);
        }

        foreach (Suppression s in addressMatches)
        {
            if (fr.CleanedName.ToString().ToLower() == s.NameKey.ToLower())
            {
                sql = string.Format("UPDATE MyTable set {0} = {1} where SuppressionId = {2}", sqlTargetFlag, sqlTargetValue, s.SuppressionId);
                break;
            }
        }

        if(sql != "")
        {
            con.Open();
            MySqlCommand cmd = new MySqlCommand(sql, con);
            cmd.ExecuteNonQuery();
        }
    }
}

private string BuildInsertSql(string sqlTargetFlag, FileRow fr, string sqlTargetValue)
{
    return string.Format(@"INSERT INTO MyTable (`AddressKey`, `NameKey`, `{0}`) VALUES ('{1}', '{2}', '{3}')",
                                sqlTargetFlag, SanitiseForMySql(fr.CleanedAddress), SanitiseForMySql(fr.CleanedName), sqlTargetValue);
}

Code like this is bound to be a bit hairy.

However

You aren't using parameters

This is a big security vulnerability. What's that SanitiseForMySql method doing? Is it doing everything by the book? Does it handle every possible obscure corner case? Let ADO the MySQL connector manipulate your query, especially if there's a chance you're handling values supplied by the user.

MySQL can perform the so-called MERGE (also called UPSERT)

You can do it via INSERT ... ON DUPLICATE KEY UPDATE statement. In case you are not aware what that is, it's basically a way to either insert or update in case the INSERT fails because of a duplicate key error. It's for sure faster than making a separate query to retrieve the records that are currently present.

By the way, code like this leaves your code open to race conditions in case another user inserts a record exactly between when you read the current records and the moment you execute your INSERT. This is bad, and you're not currently guarding against it. MySQL can do it for you, take advantage of it.

EDIT

Also

This:

        if (fr.IsGoneaway)
        {
            sqlTargetFlag = sqlTargetFlag + "Goneaway";
        }

are you sure this is correct? Concatenating a column name to the other like that? Are you assuming the column name will be passed in followed by a comma? And anyway, isn't it going to blow up with a syntax error when you do the `UPDATE`? Or will the column name get passed as an empty string? Might be a good idea to check what garbage you are receiving through your parameters, assuming said checks aren't already in place at an upper layer. How can you be sure all callers will remember to pass your arguments exactly the way you expect them? Plus, just my two cents, but if my assumptions are correct, this is not a very fun API to work with.


Transform a string into an array based on grouping rules I've got a simple spec for defining character sets, ranges, or wildcards. I'd like to take the string and appropriately create an array based on these rules. I have a solution but I'm sure there is a better way!

Single characters may be used
Sets and ranges are enclosed in brackets []
Sets are comma separated, [1,3,S]
Ranges are hyphenated, [1-3]
Sets and ranges can be combined, [1-3,7,9-10,S]
Wildcard character is - and not enclosed in brackets
Strings can contain the following characters are A-Z 0-9 - .

Example inputs and expected outputs

'ABC' outputs ['A', 'B', 'C']
'S[1-2]-' outputs ['S', '1-2', '-']
'S[1-2]2.0[0-9][1,3,7]S' outputs ['S', '1-2', '2', '.', '0', '0-9', '1,3,7', 'S']
'S[1-2,7]' outputs ['S', '1-2,7']

My implementation:

function stringToArray(code) {
    var exploded = code.split(''),
            charGroups = [],
            tempGroup = '',
            inGroup = false;
    while (exploded.length > 0) {
        var cur = exploded.shift();
        if (cur === '[') {
            inGroup = true;
            continue;
        }
        if (inGroup) {
            if (cur === ']') {
                inGroup = false;
                charGroups.push(tempGroup);
                tempGroup = '';
            } else {
                tempGroup += cur;
            }
        }
        if (!inGroup) {
            if (cur !== ',' && cur !== '[' && cur !== ']') {
                charGroups.push(cur);
            }
        }
    }
    return charGroups;
}

Demo fiddle here: http://jsfiddle.net/3q25eps3/

You could perhaps use regular expressions, provided the spec stays simple. For instance

function stringToArray(string) {
  var segments = [];
  string.replace(/(\[([^\]]+)\]|[^,])/g, function (m0, m1, m2) {
    segments.push(m2 || m1); // push bracket content (m2) or single char (m1)
  });
  return segments;
}

The code uses replace to map/scan the string (it doesn't actually bother with replacing anything), since it accepts a callback function.

The pattern used will match either:

The content between [ and ] as a contiguous string: \[([^\]]+)\]
Single, non-comma character outside brackets: [^,]

And due to the g (global) flag in the pattern, it'll repeat matching until the end of the string.

This depends on bracket not being nested or unbalanced (it can't handle that). It's also assumes anything that's not a comma should be captured. I.e. even if the character @ isn't part of the spec, it'll still get treated as a "valid" character and included in the output. Similarly it'll skip over multiple commas in a row, even if such a string is invalid.

So it's kinda rough, and I can't guarantee it'll cover all cases, but it gives the correct output for your test cases.


Displaying subcategories of subcategories I wanted to know if this code is safe against SQL injection or not?
Also do I need to mysqli_close if I already did the mysqli_stmt_close?  Other suggestions are always welcome.

if ($stmt = mysqli_prepare($connect, "SELECT subcategories.subcat_name, subsubcategories.subsubcat_name, subcategories.subcat_ID FROM subcategories INNER JOIN subsubcategories ON subcategories.subcat_ID=subsubcategories.subcat_ID WHERE subcategories.cat_ID = ? OR subcategories.extra_cat_ID = ? ORDER BY subcategories.subcat_name, subsubcategories.subsubcat_name ASC")){
mysqli_stmt_bind_param($stmt, "ii", $cat_ID, $cat_ID);
mysqli_stmt_execute($stmt);
mysqli_stmt_bind_result($stmt, $subcat_name, $subsubcat_name, $subcat_ID);
$lastcat = 0;
while (mysqli_stmt_fetch($stmt)){
    if($lastcat != $subcat_ID){
        $lastcat = $subcat_ID;
        echo "
";
        echo $subcat_name;
        echo "
";
        echo "
";
        }

    echo $subsubcat_name;
    echo "
";
    }
}
mysqli_stmt_close($stmt);
mysqli_close($connect);

SQL Injection

Yes, your code is safe against SQL Injection. Make sure to use prepared statements when fetching things from the database as well, even in internal application services without user input.

Others

Well, I'm guessing this is a subset of the code in Displaying categories and subcategories in php having different tables, so my review there still stands.


PyYAML - Saving data to .yaml files So I've picked up PyYAML over the past few days but I feel like some of the documentation is vague or I'm misinterpreting it. Here is a function for writing data to a .yaml which can write the values to each element I have in the list:

def writeSave(filename, **kwargs):
    with open(filename, 'r') as yamlFile:
        yamlDict = yaml.load(yamlFile)
        if not yamlDict: yamlDict = {}

    with open(filename, 'w') as write:
        z = yamlDict.copy()
        z.update(kwargs)
        yaml.dump(z, write)

Which will then overwrite the values below when called:

saveData:
        {mission_state: 7, playername: 'steve'}

Using this function seems simple:

writeSave('../path/savedata.yaml', mission_state=2)

Which will then change the mission_state in the list.

I don't know if I've done this smoothly or if there is a better practice to performing these kind of functions. If there are, it'd be nice to have some kind of pointers.

Is there a way to make this more readable? Maybe more readable vars or more commenting?

First, Python has an official style-guide, PEP8, which you should follow. It recommends using lower_case for functions and variables.
I would also rename your function, since write_save does not say what this function is doing. I think overwrite_setting or update_value or something similar would be better.
You might want to add a check if any keyword arguments are passed to the function. Either raise an exception (a simple AssertionError might be enough), or at least don't waste time reading the file and writing the same content back.
While open opens a file in read-mode by default, here I think it is actually fine to open it once explicitly with 'r' and then with 'w'.
Your z = yamDict.copy() should be completely unnecessary. Mutating the original dictionary does not do any harm. The updating does not need to be within the with block either.
Finally, you could use or for the check of an empty dictionary.

With all of the above changes, your code would become:

import yaml

def update_value(filename, **kwargs):
    assert kwargs
    with open(filename, 'r') as f:
        yaml_dict = yaml.load(f) or {}
    yaml_dict.update(kwargs)
    with open(filename, 'w') as f:
        yaml.dump(yaml_dict, f)

If you have only one thread, you could skip the reading of the config everytime you want to change a value and instead keep the dictionary in memory (after having it read once at the beginning). You will still want the write to save state, though. For this you might want to have a Config class, derived from dict:

import os

class Config(dict):
    def __init__(self, filename):
        self.filename = filename
        if os.path.isfile(filename):
            with open(filename) as f:
                # use super here to avoid unnecessary write
                super(Config, self).update(yaml.load(f) or {})

    def __setitem__(self, key, value):
        super(Config, self).__setitem__(key, value)
        with open(self.filename, "w") as f:
            yaml.dump(self, f)

    def __delitem__(self, key):
        super(Config, self).__delitem__(key)
        with open(self.filename, "w") as f:
            yaml.dump(self, f)

    def update(self, kwargs):
        super(Config, self).update(kwargs)
        with open(self.filename, "w") as f:
            yaml.dump(self, f)

You might want to define a decorator to add the dumping part to the methods:

import functools

def dumps(func):
    @functools.wraps(func)
    def wrapper(self, *args, **kwargs):
        ret = func(self, *args, **kwargs)
        with open(self.filename, "w") as f:
            yaml.dump(self, f)
        return ret
    return wrapper

class Config(dict):
    def __init__(self, filename):
        self.filename = filename
        if os.path.isfile(filename):
            with open(filename) as f:
                # use super here to avoid unnecessary write
                super(Config, self).update(yaml.load(f) or {})

    __setitem__ = dumps(dict.__setitem__)
    __delitem__ = dumps(dict.__delitem__)
    update = dumps(dict.update)

You can use this class like a dict, but every change to the dictionary is also dumped to the yaml file. The file is only read once (when the config is initialized), so changes to the file will only be visible when the script is re-started.

cfg = Config("test.yaml")
print(cfg)
cfg['a'] = 3
print(cfg)
cfg.update({"b": 4})
cfg.update(c=5)
del cfg['a']
print(cfg)


Find all palindromes in the danish language I made a program to attempt to find all palindromes in the danish language. Words.txt contains about 50.000 lines of danish words. Palindromes.txt is empty initially.

Im running the program on a Intel® Core™ m3-7Y30 Processor - which has a base frequency of 1.00 GHz. Seeing as the program - as it currently stands - has to check 50.000 * 50.000 = 2.500.000.000 words, it is really slow. Ive been running it for about 25 minutes now without a result (unsuprisingly). I am fairly new to C++, and so I have no idea how to optimize this.

Thats why im posting here - any tips on how to optimize the speed of the program would be greatly appreciated!

#include <string>
#include <fstream>
#include <iostream>
#include <vector>
#include <sstream>
#include <algorithm>
#include <chrono>

std::string read_file(const std::string& path)
{
    std::ifstream file(path);
    if (!file.is_open())
    {
        std::cerr << "Couldent open file!" << std::endl;
    }
    std::string content = std::string(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>());
    if (content.empty())
    {
        std::cerr << "Couldent read file content" << std::endl;
    }
    return content;
}

std::vector<std::string> split_words(const std::string& words_string)
{
    std::vector<std::string> words;
    words.reserve(50000);
    std::stringstream ss_word(words_string);
    std::string word;
    while (!ss_word.eof())
    {
        ss_word >> word;
        words.push_back(word);
    }
    return words;
}

std::vector<std::string> find_palinedromes(const std::vector<std::string>& words)
{
    std::vector<std::string> palindromes;
    std::string first_word = std::string();
    std::string second_word = std::string();

    for (int i = 0; i < words.size(); ++i)
    {
        for (int j = 0; j < words.size(); ++j)
        {
            first_word = words[i];
            second_word = words[j];
            std::reverse(first_word.begin(), first_word.end());

            if (first_word == second_word)
            {
                if (std::find(palindromes.begin(), palindromes.end(), first_word) == palindromes.end())
                {
                    palindromes.push_back(first_word);
                }
            }
        }
    }

    return palindromes;
}

void write_to_file(const std::string& path, const std::vector<std::string>& words)
{
    std::ofstream file(path);
    if (!file.is_open())
    {
        std::cerr << "Couldent open file!" << std::endl;
    }
    for (const std::string& word : words)
    {
        file << word + "\n";
    }
}

int main()
{
    auto start = std::chrono::high_resolution_clock::now();
    std::string words_string = read_file("C:/Users/A/source/repos/Bresenhams/Bresenhams/src/Words.txt");
    std::vector<std::string> words = split_words(words_string);
    std::vector<std::string> palindromes = find_palinedromes(words);
    write_to_file("C:/Users/A/source/repos/Bresenhams/Bresenhams/src/Palindromes.txt", palindromes);
    auto stop = std::chrono::high_resolution_clock::now();
    auto duration = std::chrono::duration_cast<std::chrono::seconds>(stop - start);
    std::cout << "Found all palindromes in: " << duration.count() << std::endl;
    return 0;
}

You are doing a double loop checking against all other words to determine if something is a palindrome. This is unnecessary. You can look at a single word and determine if it is a palindrome, by reversing it. This should reduce the number of checks by a factor of n.

You should also prefer using a range for to a raw for-loop.

find_palindromes can then become something like this:

std::vector<std::string> find_palindromes(const std::vector<std::string>& words)
{
    for (const std::string& word : words )
    {
        std::string reverseWord(word);
        std::reverse(reverseWord.begin(), reverseWord.end());

        if (word == reverseWord)
        {
            if (std::find(palindromes.begin(), palindromes.end(), first_word) == palindromes.end())
            {
                palindromes.push_back(first_word);
            }
        }
     }

    return palindromes;
}


Python implementation of quicksort My teacher often tells me that I don't take full advantage of Python capabilities, I would like to know if my implementation of Quicksort is good enough and how could I improve it, aside from doing it in-place

def quickSort(l):
    if len(l) > 1:
        l,iPiv = partition(l)
        return quickSort(l[:iPiv]) + [l[iPiv]] + quickSort(l[iPiv+1:])
    else:
        return l

def partition(l):
    i = 1
    iPiv = 0
    for j in range(1,len(l)):
        if l[j] <= l[iPiv]:
            l[i],l[j] = l[j],l[i]
            i += 1
    l[i-1],l[iPiv] = l[iPiv],l[i-1]
    return l,i-1

My teacher often tells me that I don't take full advantage of Python capabilities

Try partitioning with list comprehensions.

import random

def quicksort(s):
    len_s = len(s)
    if len_s < 2:
        return s

    pivot = s[random.randrange(0, len_s)]

    L = [x for x in s if x < pivot]
    E = [x for x in s if x == pivot]
    G = [x for x in s if x > pivot]

    return quicksort(L) + E + quicksort(G)

This is Pythonic, compact and faster than your partition function.

Furthermore, notice the pivot is random. Your original code always uses zero for the pivot. This results in worst-case O(n^2) complexity for sorted inputs. A random pivot mitigates this risk.

As for style, the answer from @janos offers solid guidance also.


Read date from a file and rename two files using the date Problem
I have image files that are RAW data and have the extension .NEF Whatever edits I make to these files are stored in what is called a sidecar file with the extension .xmp
Given there are many photos and the photo numbering in the camera can only go up to 9999, there is a possibility that I may get duplicate filenames in the same folder over time.
Objective
Given the image files and the sidecar files must have the same base name to remain associated, I'd like to append the date the photo was taken to the base name for both files.
The original date can be found within the .xmp file on a line beginning with a specific string.
I want to

find all image files that have a sidecar file
Get the image date from within the sidecar file
Rename both the image file and sidecar file with the date appended at the end of the base name

Code
I'm pretty sure I'm not importing any namespace except System.IO, so this should all be the .NET way, not any VB6.
Option Explicit On
Option Strict On
Option Infer On
Option Compare Text
Imports System.IO
Module PhotoRename

    Sub Main()
        Const DIR_PATH As String = "C:\Temp\"
        Const IMAGE_EXTENSION As String = ".NEF"
        Const SIDECAR_EXTENSION As String = ".xmp"
        GetFilesInDirectory(DIR_PATH, IMAGE_EXTENSION, SIDECAR_EXTENSION)
    End Sub

    Private Sub GetFilesInDirectory(ByVal path As String, ByVal photoExtension As String, ByVal sidecarExtension As String)
        Const EXTENSION_LENGTH As Integer = 4
        Dim arrayOfFiles() As String = Directory.GetFiles(path, "*" & photoExtension)
        Dim baseName As String
        Dim photoFileName As String
        Dim sidecarFileName As String
        Dim originalDate As String
        For arrayIndex As Integer = 0 To arrayOfFiles.Length - 1
            photoFileName = arrayOfFiles(arrayIndex)
            baseName = photoFileName.Substring(0, photoFileName.Length - EXTENSION_LENGTH)
            sidecarFileName = baseName & sidecarExtension
            If File.Exists(sidecarFileName) Then
                originalDate = GetFileDate(sidecarFileName)
                RenamePhotoFiles(baseName, originalDate, photoExtension, sidecarExtension)
            End If
        Next
    End Sub

    Private Function GetFileDate(ByVal sidecarFile As String) As String
        Const LINE_START As String = "   exif:DateTimeOriginal="
        Const DATE_STRING_LENGTH As Integer = 10
        Const CHAR_TO_REMOVE As String = "-"
        Dim lines = File.ReadAllLines(sidecarFile)
        Dim targetData As String = Nothing
        For i As Integer = 0 To lines.Length - 1
            If lines(i).StartsWith(LINE_START) Then
                targetData = lines(i)
                Exit For
            End If
        Next
        targetData = targetData.Substring(LINE_START.Length + 1, DATE_STRING_LENGTH)
        GetFileDate = targetData.Replace(CHAR_TO_REMOVE, "")
    End Function

    Private Sub RenamePhotoFiles(ByVal oldName As String, ByVal fileDate As String, ByVal photoExtension As String, ByVal sidecarExtension As String)
        Const FILE_EXTENDER As String = "_"
        Dim newName As String = oldName & FILE_EXTENDER & fileDate
        File.Move(oldName & photoExtension, newName & photoExtension)
        File.Move(oldName & sidecarExtension, newName & sidecarExtension)
    End Sub

End Module

Important change

I would suggest you look at the Path class. A lot of your logic is about manipulating filename and this class does it all for you.

Less important change

Depending on how big the file is, doing File.ReadAllLines might take more memory than needed. Reading one line at a time would be better.

GetFilesInDirectory function name isn't true. It also renames the file.

You could use a bit of error checking. For example, what happen if the string "exif:DateTimeOriginal=" can't be found in the file. What if there's no xmp file to the nef file.


Custom threading macro To help myself learn macros, I made a custom version of the threading macro that lets you choose which argument the "thread" gets put into.

My main concern is simplifying it, and making it more idiomatic, but anything notable is welcome. If anyone knows of a better implementation for insert-at, I'd love to know that as well!

; Helpers. Other macros use these, or else I'd just inline them
(defn factor? [n m]
  (= (rem n m) 0))

(defn mult-forms? [n forms]
  (factor? (count forms) n))

(defn insert-at
  "Inserts an element at the given index. Returns a list.
  Indices out of range of the collection are treated as either 0 or (dec (count xs))
  Likely very expensive."
  [xs x index]
  (let [[f-half s-half] (split-at index xs)]
    (concat f-half `(~x) s-half)))

(defmacro n->
  "Expects a value to be threaded, and pairs of indices and forms.
  The indices indicate which index the value should be inserted at in the following form.
  Example:
      (n-> 1
          1 (+ 2)
          0 (+ 3))
  Expands to: (+ (+ 2 1) 3)"
  [val & forms]
  (if (and (not (empty? forms))
           (mult-forms? 2 forms))
    (let [[thread-n form & rest-forms] forms
          threaded  (insert-at form val (inc thread-n))]
      `(n-> ~threaded ~@rest-forms))
    val))

Note, I'm not claiming this is a useful macro; it will likely lead to confusing to read code. This was just an exercise.

Example:

(n-> "Hello"
  0 (str "0" "1" "2" "3")
  1 (str "0" "1" "2" "3")
  2 (str "0" "1" "2" "3"))

Expands to:

(clojure.core/str "0" "1"
    (clojure.core/str "0"
        (clojure.core/str "Hello" "0" "1" "2" "3") "1" "2" "3") "2" "3")

Which evaluates to:

"010Hello012312323"

I'd make a few modest changes.

Don't use the helpers: use even? and count instead.
Reorder the arguments to insert-at to comply with those of assoc.
Make the even-ness condition in n-> an assertion: don't try to deal
with invalid cases.

Thus

(defn insert-at [xs n x]
  (let [[f-half s-half] (split-at n xs)]
    (concat f-half (cons x s-half))))

(Note the use of cons)

For example,

(insert-at [1 2 3 4 5] 3 :a)
;(1 2 3 :a 4 5)

And

(defmacro n-> [val & forms]
  (assert (even? (count forms)))
  (if (seq forms)
    (let [[n form & rest-forms] forms
          threaded  (insert-at form (inc n) val)]
      `(n-> ~threaded ~@rest-forms))
    val))

For example,

(macroexpand '(n-> 1, 1 (+ 2), 0 (+ 3)))
;(+ (+ 2 1) 3)

Note that the macro now works (as an identity) with one argument:

(macroexpand '(n-> (whatever you like)))
;(whatever you like)

Another approach is to do the work of the macro inside a function:

(defn n->fn [val & forms]
  (assert (even? (count forms)))
  (let [pairs (partition 2 forms)]
    (reduce
     (fn [acc [n form]]
       (insert-at form (inc n) acc))
     val
     pairs)))

For example,

(apply n->fn '(1, 1 (+ 2), 0 (+ 3)))
;(+ (+ 2 1) 3)

Once this function gets hold of the forms as data, it can do what the macro did without implicit recursion.

So we can define the macro in terms of the function:

(defmacro n-> [& stuff]
  (apply n->fn stuff))

... with identical results.


Rotate the sub-list of a linked list from position M to N to the right by K places Given a linked list and two positions m and n. The task is to rotate the sub-list from position m to n, to the right by k places. (link to GeeksforGeeks)
Examples
Input:
list 1->2->3->4->5->6, m = 2, n = 5, k = 2
Output: 1->4->5->2->3->6
Explanation: Rotate the sub-list 2 3 4 5 towards right 2 times
then the modified list is: 1 4 5 2 3 6
Input: list = 20->45->32->34->22->28, m = 3, n = 6, k = 3
Output: 20->45->34->22->28->32
Explanation: Rotate the sub-list 32 34 22 28 towards right 3 times
then the modified list is: 20 45 34 22 28 32
// Rotate the sub-list of a linked list from position M to N to the right by K places

#include <iostream>
using namespace std;

//  structure of the node in the linked list
struct Node {
    int data;
    Node* next;
};

// inserting at the beggining
void push( Node **head, int data ) {
    Node *newNode = new Node();
    newNode->data = data;

    if((*head) == NULL) {
        (*head) = newNode;
        return;
     }

    Node *temp = (*head);
    while(temp->next != NULL)
        temp = temp->next;
    temp->next = newNode;
    newNode->next = NULL;
  }

// display
void display(Node *head) {
    cout << "The list : \t";
    while (head != NULL) {
        cout << head->data << " ";
        head = head->next;
    }
}

// Rotate the linked list by k
void rotate(Node *head, int m, int n, int k) {
    Node *temp1 = head ;
    Node *temp3 = NULL, *ref = NULL, *last = NULL;

    for (int i = 0; i < m - 1 ; i++)
        temp1 = temp1->next;
    ref = temp1->next;
    temp3 = ref ;

    for (int i = 0; i < n - m; i++)
        temp3 = temp3->next;
    last = temp3->next;
    temp3->next = ref ;

    k = k%(n - m + 1);

    for(int i = 0; i < k; i++) {
        temp3 = ref;
        ref = ref->next;
    }
    temp3->next = last;

    // if the starting point is not 1st element
    if (m == 0)
        head = ref;
    else
        temp1->next = ref;

    display(head);
}

// Driver function
int main() {
    Node *head = NULL;

    push(&head, 1);
    push(&head, 2);
    push(&head, 3);
    push(&head, 4);
    push(&head, 5);
    push(&head, 6);
    push(&head, 7);

    display(head);

    cout << "\n\n";
    int m, n, k;
    cout << "m, n, k = \t";
    cin >> m >> n >> k ;

    rotate(head, m - 1, n - 1, k);
    return 0;
}

I see a number of things that may help you improve your program.

Don't abuse using namespace std

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

Omit return 0

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

Make sure the comments don't mislead

The code currently includes this comment and function:

// inserting at the beggining
void push( Node **head, int data ) {

However, that comment is false (and spelled incorrectly).  In fact, the data is appended to the end of the linked list.  For that reason, I'd suggest changing the name to append.

Use nullptr rather than NULL

Modern C++ uses nullptr rather than NULL.  See this answer for why and how it's useful.

Return something useful from functions

It is not very useful to have every function return void.  Instead, I'd suggest that push (now renamed to append per previous suggestion) could return a pointer to the head of the list.  Here's one way to write that:

// append data to end of linked list
Node* append(Node *head, int data) {
    auto newNode = new Node{data, nullptr};
    if (head == nullptr) {
        return newNode;
    }
    auto temp{head};
    while (temp->next) {
         temp = temp->next;
    }
    temp->next = newNode;
    return head;
}

Don't leak memory

This code calls new but never delete.  This means that the routines are leaking memory. It would be much better to get into the habit of using delete for each call to new and then assuring that you don't leak memory.

Use objects

The Node object is a decent start, but I'd recommend and actual linked list object to better take care of memory management and node initialization.

Sanitize user input

If the user enters a negative number or a number that's larger than the array, or numbers that are not in the right order, bad things happen in the current program.  In general, it's better to be very wary of user input and test for and handle any bad input.

Fix the bug

If the subsequence includes the first node, the output is incorrect.  That's a bug.

Rethink the problem

There are two essential parts to the algorithm.  First, we identify the subsequence and then we do the rotation.  With some thought, you can figure out either 0 or 3 next links will need to be changed.  We can immediately understand that if \$n - m + 1 = k \mod (n-m+1)\$ then no links need to change and we're done.  Otherwise 3 links need to change.  Let's number each of the nodes, starting from 1.  So the first node is \$N[1]\$, and the second is \$N[2]\$, etc.

Now the first node that needs to be altered is \$N[m-1]\$.  It needs to point to \$N[m + k]\$.
Next \$N[n]\$ (the last node of the subset) points to \$N[m]\$ (the first of the subset).
Then \$N[m + k - 1]\$ must point to \$N[n+1]\$

With that in mind, it should be apparent that only a single pass through the data structure is needed with only two temporary pointers.  I'll leave it to you to write the code for that.


Project Euler #55 - Lychrel numbers If we take 47, reverse and add, 47 + 74 = 121, which is palindromic.
Not all numbers produce palindromes so quickly. For example,
349 + 943 = 1292,
1292 + 2921 = 4213
4213 + 3124 = 7337
That is, 349 took three iterations to arrive at a palindrome.
Although no one has proved it yet, it is thought that some numbers, like 196, never produce a palindrome. A number that never forms a palindrome through the reverse and add process is called a Lychrel number. Due to the theoretical nature of these numbers, and for the purpose of this problem, we shall assume that a number is Lychrel until proven otherwise. In addition you are given that for every number below ten-thousand, it will either (i) become a palindrome in less than fifty iterations, or, (ii) no one, with all the computing power that exists, has managed so far to map it to a palindrome. In fact, 10677 is the first number to be shown to require over fifty iterations before producing a palindrome: 4668731596684224866951378664 (53 iterations, 28-digits).
Surprisingly, there are palindromic numbers that are themselves Lychrel numbers; the first example is 4994.
How many Lychrel numbers are there below ten-thousand?

This code works with NodeJS. It is also using ES6 features.
So no SpiderMonkey nonsense now.
$ node -v
v0.12.7
$ node --harmony p55.js
249

Any and all feedback is appreciated.
// Like Python's range.
function* range(start, stop, step){
    if (arguments.length == 0){
        start = 0;
        stop = null;
    }
    if (arguments.length == 1){
       stop = start;
       start = 0;
    }
    if (arguments.length < 3)
        step = 1;

    start = parseInt(start);
    if (Number.isNaN(start))
        start = 0;
    stop = parseInt(stop);
    if (Number.isNaN(stop))
        stop = null;
    step = parseInt(step);
    if (Number.isNaN(step))
        step = 1;

    if (stop === null){
        while (true){
            yield start;
            start += step;
        }
    }else if (step == 0){
        while (true){
            yield start;
        }
    }else{
        if (step > 0){
            for (var number = start; number < stop; number += step){
                yield number;
            }
        }else{
            for (var number = start - 1; number >= stop; number += step){
                yield number;
            }
        }
    }
}

function is_palindrome(string){
    var str_len = string.length - 1;
    for (var index of range(string.length / 2)){
        if (string[index] != string[str_len - index]){
            return false;
        }
    }
    return true;
}

function reverse(string){
    var new_string = '';
    for (var index of range(string.length, 0, -1)){
        new_string += string[index];
    }
    return new_string;
}

function is_lychrel(number, recursions){
    if (recursions == 0)
        return false;
    if (arguments.length == 1)
        recursions = 50;
    number += parseInt(reverse(number.toString()));
    if (is_palindrome(number.toString()))
        return true;
    return is_lychrel(number, recursions - 1);
}

function main(){
    var total = 0;
    for (var number of range(10000)){
        if (! is_lychrel(number)){
            total += 1;
        }
    }
    return total;
}

console.log(main());

Typically in JavaScript, when calling isNaN, it is just typed:

isNaN(...);

Not as a call from the Number class:

Number.isNaN(...);

Also, by calling it the way that you did, you are being inconsistent. If you are going to call isNaN through Number, then why don't you also call parseInt through Number?

The function parseInt takes a second parameter radix which is the base of the first parameter.

According to the MDN, you should always specify the radix parameter.

Why do you call parseInt on the arguments of range? In every single point of your code where you are calling this function, you are passing numerical arguments.

Calling this functions is just a waste.

In places like these:

if (arguments.length == 0){

and

if (arguments.length == 1){

You should be using the === comparison operator rather than the == comparison operator.

It is good JavaScript practice.
It may be a little faster.

See this SO post for more information.

As shown in this SO post, a much, much simpler way to reverse a string would be to do this:

return string.split("").reverse().join("");

This could potentially be faster, too, as this is using JavaScript's built in functions/methods/features.

Your is_palindrome function confuses me a little. A palindrome is defined as a word that is spelled the exact same as it's reverse. Therefore, why aren't you using the reverse function that you already made?

Now, your is_palindrome function becomes this:

return string == reverse(string);

This is much more simple than whatever you were doing.

The naming case for JavaScript is camelCase, not snake_case. You need to change the name of these functions:

is_palindrome --> isPalindrome

and

is_lychrel --> isLychrel

And, make sure you change the name of some of your variables to camelCase too.

You are putting too much work on your isLychrel function.

Here, you check to see that if there is no recursions specified, set it to 50:

if (arguments.length == 1)
    recursions = 50;

However, this check is going to happen every single iteration (since the function is recursive).

Is is really that difficult to just pass 50 to it in the initial time you call it in main? If you did this and removed the check from the function, this could greatly improve your performance.


Match formula for loop in VBA I am using a match formula inside of a for loop, but it is taking too much time. Is there any way to optimize this code to make it go faster?

 Lastrow = ActiveSheet.UsedRange.SpecialCells(xlCellTypeLastCell).Row
For i = 3 To Lastrow

       Sheets("sample").Range("AM1000000").End(xlUp).Offset(1, 0).Select
        Selection.FormulaArray = _
        "=IF(ISNUMBER(MATCH(1," & Chr(10) & "  (order!R2C15:R1000000C15=RC[-24])*" & Chr(10) & "  (order!R2C7:R1000000C7=RC[-32])*" & Chr(10) & "  (order!R2C24:R1000000C24=RC[-15])," & Chr(10) & "  0)), ""pass"",""review"")"

    Next i
      Columns("AM:AM").Select
    Selection.Copy
    Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks _
        :=False, Transpose:=False

The key optimisation can be found in this part:

Dim orderColumnOArray as Variant, orderColumnGArray as Variant, orderColumnXArray as Variant
orderColumnOArray = ThisWorkbook.Worksheets("order").Range("O2:O1000000").Value
orderColumnGArray = ThisWorkbook.Worksheets("order").Range("G2:G1000000").Value
orderColumnXArray = ThisWorkbook.Worksheets("order").Range("X2:X1000000").Value

There above, we are using arrays to store the order data, and we can traverse them more efficiently, rather than repeat a full array formula calculation for each cell.

Note that even then we could cut down on the million rows, and do a lastRow calculation for the order sheet. Perhaps that will bring great improvement too. However if we say for now that you will genuinely have that number of data rows in the order Worksheet, we can still go about it a bit better.

With ThisWorkbook.WorkSheets("sample")
    'I will assume that the column needs to filled from AM3 to AMLastrow
    'I also assume the tested values are strings or integers, not dates or floats
    Dim resultArray() as String
    ReDim resultArray(3 to Lastrow)
    Dim j as Long

    For i = 3 To Lastrow
        resultArray(i) = "review"
        For j = 1 To Ubound(orderColumnOArray)
            'The below nested Ifs can be done in many different ways
            'e.g. And with the 3 equality tests
            'or even by initially making two concatenated arrays!
            If .Range("O" & i).Value = orderColumnOArray(j, 1) Then
                If .Range("G" & i).Value = orderColumnGArray(j, 1) Then
                    If .Range("X" & i).Value = orderColumnXArray(j, 1) Then
                        resultArray(i) = "pass"
                        Exit For
                    End if
                End if
            End if
        Next j
    Next i

    'Now we can write back the array to the worksheet faster
    .Range("AM3").Resize(Lastrow - 3 + 1, 1).Value = resultArray
End With


Trait for specifying a type parameter for Into::into There are occasionally times when it would be nice to be able to specify a type parameter for Into::into.

This doesn't happen very often—mostly just when you have a long-ish chain of method calls where the object type is changing. In that case, wrapping one part of the chain with T::from, or splitting it into multiple statements so you can specify the type in the variable binding would be kind of awkward.

I put together the following trait:

pub trait TypedInto {
    fn typed_into<T>(self) -> T where Self: Into<T> { self.into() }
}

impl<T> TypedInto for T {}

And I can now specify the type for into:

let clip = clips::table.find(id).first::<db::Clip>(&*conn)?
    .typed_into::<models::Clip>()
    .with_associations(&*conn)?;

Am I going to regret using this? This is in the context of an application rather than a library, so I don't necessarily need to worry about anyone other than myself reading it. Is this a bad habit? Should I just be more verbose?

The most common way to handle type-hinting Into::into in stable Rust today is via temporaries. So for the use case you have here, I'd probably write something like:

let clip = {
    let clip: models::Clip = clips::table_find(id).first::<db::Clip>(&*conn)?.into();
    clip.with_associations(&*conn)?
};

But honestly, I don't think anyone will get up in arms about you using TypedInto. I know I've typed .into::<Type>() a couple times, and the common place you'd see the turbofish, .collect::<Vec<_>>(), is exactly this.

For this specific case, why don't you make a common db::T -> model::T conversion? It seems to be a common thing that happens, so making it explicit may be beneficial.

But that said, why don't we take a look at nightly feature gates? Because since 2015 the RFC for Type Ascription has been merged. It introduces an expr : ty syntax to the language meant to address cases like this. So rewriting your example to use type ascription,

let clip = ((clip::table.find(id)
    .first(&*conn)? : db::Clip)
    .into() : models::Clip)
    .with_associations(*&conn)?;

Unfortunately, the binding of type ascription, while the same as as, is lower than method call syntax (.), so the parenthesis are necessary (and rustfmt would probably not approve of the formatting I used here). There is a patch written which would cause type ascription to bind more tightly than method calls, but it was closed in favor of waiting to later RFC the change (which would be backwards compatible (probably)).

There has been some recent noise around the feature, since it would make working with Futures a lot easier (though impl Trait may also help as well). Stabilization (since it has been approved through RFC) is blocked on ensuring soundness and allowing &[1, 2, 3] : &[u8] to work, so if you'd love to see type ascription stabilize, you can help with one of those.


A function to split an string of alphabets and numbers into arrays and sort them respectively if I have a string AYUKB17053UI903TBC. I want a function to return ABKUY01357IU039BCT. So every alphabetical part of the string is sorted, as well as the numerical part. But they retain their original orders in the string...

We can assume that the input only contains number and English letters

I came up with a solution but I don't think it is elegant.

    const string1 = 'AYUKB17053UI903TBC'
    const string2  = `ABKUY01357IU039BCT`

    function fn1(string1) {
      let tempArray = [[]]
      for (const char of string1) {
        let lastCharIsNumber
        let currentCharIsNumber
        const lastArray = tempArray[tempArray.length - 1]
        if(!lastArray.length){
          lastArray.push(char)
          continue
        }
        currentCharIsNumber = !Number.isNaN(Number(char))
        lastCharIsNumber = !Number.isNaN(Number(lastArray[lastArray.length - 1]))
        if (currentCharIsNumber && lastCharIsNumber)  lastArray.push(char)
        else if (!currentCharIsNumber && !lastCharIsNumber) lastArray.push(char)
        else tempArray.push([char])
      }

      tempArray.forEach(item => item.sort())

      return tempArray.map(array => array.join('')).join('')
    }

    console.log(fn1(string1) === string2); // true

Can anyone help to improve my solution?

You can use RegEx to extract the sequence of alphabets or numbers.
To match alphabets/number, use character class in Regular expression as below

/[A-Z]+|[0-9]+/

to match all occurrences, use g(global) flag.

After extracting sequences, use Array#reduce with Array#sort & Array#join to get the sorted string.

const str1 = 'AYUKB17053UI903TBC';
const str2 = 'ABKUY01357IU039BCT';

console.log(str1
  .match(/[A-Z]+|[0-9]+/g)
  .reduce((a, c) => a + [...c].sort().join(''), '')
    === str2
);


A Simple Rock-Paper-Scissors Game I have just finished learning basic-intermediate Python subjects and wanted to test myself. So here is the my basic Rock-Paper-Scissors game.

def play():
    import random

    player_score = 0
    ai_score = 0

    while True:
        try:
            user_choice = int(input("\nRock: 1"
                                    "\nPaper: 2"
                                    "\nScissor: 3"
                                    "\nWrite the number of the object you choose: "))
        except NameError:
            print("You can only enter number.")
            continue
        except ValueError:
            print("You can only enter number.")
            continue

        if user_choice not in range(1, 4):
            print("Invalid number, you have to write numbers between 1-3")
            continue

        rps = {1: "Rock", 2: "Paper", 3: "Scissor"}
        ai_choice = random.randint(1, 3)

        print(f"\nPlayer's choice: {rps[user_choice]} | AI's choice: {rps[ai_choice]}")

        if user_choice == ai_choice:
            print("Draw!")
        elif user_choice == 1:
            if ai_choice == 2:
                print("AI's paper wrapped Player's rock, AI won this round!")
                ai_score += 1
            elif ai_choice == 3:
                print("Player's rock crushed AI's scissor, Player won this round!")
                player_score += 1
        elif user_choice == 2:
            if ai_choice == 1:
                print("Players's paper wrapped AI's rock, Player won this round!")
                player_score += 1
            elif ai_choice == 3:
                print("AI's scissor cut Player's paper, AI won this round!")
                ai_score += 1
        elif user_choice == 3:
            if ai_choice == 1:
                print("AI's rock crushed Player's scissor, AI won this round!")
                ai_score += 1
            elif ai_choice == 2:
                print("Player's scissor cut AI's paper, Player won this round!")
                player_score += 1

        print(f"\nPlayer's score: {player_score} | AI's score: {ai_score}"
              f"\nGame ends when player or AI's score reach 3!")

        if player_score == 3:
            print("Game over, Player won!")
            quit()
        elif ai_score == 3:
            print("Game over, AI won!")
            proceed = input("Wanna try again and show the AI your unmatched and great wisdom?"
                            "\nWrite Y or N >: ").lower()
            if proceed == "y":
                play()
            elif proceed == "n":
                quit()
            else:
                print("Invalid command, exiting program...")
                quit()

if __name__ == "__main__":
    play()

So, what do you think? How can i make it better, what are my mistakes?

I'd say there is two major issues in your code.

First one is that main loop which is very unclear, many if statements, filled with continue and quit() , this is spagetthi code and it is makes your code harder to read, so harder to debug, because that jumps everywhere.

The second one is the lack of OOP design, which could be very adapted to this case. Games are usually developped with strongly object oriented languages (Unreal engine uses C++ and Unity uses C# among others) because of the strong interactions between components, and a very favorable context to abstraction and modularity. To be fair, that's not really needed for short games like rock paper scissors but it prevents you to scale the game further.

There is few design improvements I suggest you:

Split your code into functions, first one could be the game winning condition that should be used in your game loop to avoid unwanted infinite loops, common with constant conditions like while True:. Replace this statement instead by while has_game_ended():, with has_game_ended looking like:

    def has_game_ended(player_score: int, ia_score: int) -> bool:
        if player_score == 3 or ia_score == 3:
            print("Game over: {} won!".format("player" if player_score == 3 else "AI"))
            return True
        return False

Then use an enum to list you moves to make them easier to reuse in your code (PAPER makes more sense than 1) and enable comparison using operator overload, note that this is a tricky comparison specific to game rules.

from enum import Enum

class Move(Enum):
    PAPER = 1
    ROCK = 2
    SCISSORS = 3

    def __lt__(self, other):
        return (self is Move.ROCK and other is Move.PAPER) or (self is Move.Paper and other is Move.SCISSORS) or (self is Move.SCISSORS and other is Move.ROCK)

Now you can write:

>>> player_choice = Move.PAPER
>>> ia_choice = Move.ROCK
>>> player_choice < ia_choice
True

and something like

player_score = player_score + int(player_choice > ia_choice)
ia_score = ia_score + int(ia_choice > player_choice)

This works well because it's not comparing two integers anymore, it's comparing two Move, which makes all the difference thanks to operator overloading.

Avoid quit() in your scripts, it totally cuts the code flow and doesn't allow nice error handling, prefer instead loop conditions or try except statements with customized Exception for unexpected behaviors (for example when you use invalid command, as in the example below).

def WrongCommand(Exception):
    pass

if __name__ == "__main__":
    try:
        play()
    except WrongCommand:
        print("Invalid command, exiting program...")


Store all entries in a single array from Wufoo api What I'm doing

Using the v3 of the Wufoo API to retrieve all entries and store in a single array ($all_results). This is necessary because of the limit 100 entries limit that Wufoo has when retrieving results from their api. Also store the results to file we don't get slow responses from the api.

My question

I'm wondering how I could have done this better. This code is working for me, but I want to get better.

<?php

function wufoo_entries() {
    // Setup
    $account = "ACCOUNT_NAME";
    $form = "FORM_NAME";
    $api_key = "YOUR-API-KEY";
    $offset = 0;
    $limit = 100;
    $check_int = 15 * 60; // 15 minutes
    $file_time = 'time_stamp.txt';
    $file_results = 'results.txt';

    // Check for the last update
    $time = time();
    $time_stamp = file_get_contents($file_time);
    $refresh = ($check_int < ($time - $time_stamp)) ? TRUE : FALSE;

    // if true run functions and save the results and time to file else pull the results from file
    if ($refresh) {
        // Initialize our results array
        $all_results = array();

        // How many entries are there?
        $api_uri = "https://$account.wufoo.com/api/v3/forms/$form/entries/count.json";
        $count = wufoo_api($api_key, $account, $form, $offset, $limit, $api_uri);
        $counted = $count['EntryCount'];

        // Call the api as many times as needed
        for ($offset = 0; $offset <= $counted - 1; $offset = $offset + $limit) {
            $results = wufoo_api($api_key, $account, $form, $offset, $limit);
            // Limit the number of times we put results into the $all_results array to make sure we don't end up with blanks
            $limit = (($offset + $limit) > $counted) ? $limit - (($offset + $limit) - $counted) : $limit;

            // Iterate through the Entries array to push each entry into a new array
            for ($result = 0; $result <= $limit - 1; $result++) {
                array_push($all_results, $results['Entries'][$result]);
            }
        }

        // Save the new results (as json) and time to file
        $all_results_file = json_encode($all_results);
        file_put_contents($file_results, $all_results_file);
        file_put_contents($file_time, $time);
        return $all_results;
    }

    else {
        // Pull the results array from file
        $all_results = file_get_contents($file_results);
        $all_results = json_decode($all_results, true);
        return $all_results;
    }
}

function wufoo_api($api_key, $account, $form, $offset, $limit, $api_uri = NULL) {
    // Check to see if it's asking for the the entries or the total count
    $api_url = ($api_uri) ? $api_uri : 'https://' . $account . '.wufoo.com/api/v3/forms/' . $form . '/entries.json?pageStart=' . $offset . '&pageSize=' . $limit;

    $curl = curl_init($api_url);
    curl_setopt($curl, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($curl, CURLOPT_USERPWD, $api_key . ':footastic');
    curl_setopt($curl, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
    curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, false); curl_setopt($curl, CURLOPT_FOLLOWLOCATION, true); curl_setopt($curl, CURLOPT_USERAGENT, 'Wufoo Sample Code');
    $response = curl_exec($curl);
    $resultStatus = curl_getinfo($curl);

    if ($resultStatus['http_code'] == 200) {
        $results = json_decode($response, true);
        return $results;
    } else {
        $results = 'Call Failed '.print_r($resultStatus, true);
        return $results;
    }
}

// Show me some results and make sure it works
echo "<pre>";
print_r(wufoo_entries());
echo "</pre>";

?>

Just some quick note:

1, I would separate the configuration settings to a distinct php file (and use a WufooConfig class/struct) and include it at the beginning of the code. It makes the configuration easier to change. If you use a WufooConfig class/struct and pass it to the wufoo_entries() function you will be able to use the same function to fetch data from more than one accounts. Furthermore, it would encapsulate configuration, so you just have to pass around one parameter instead of $account, $form, $api_key, etc.

class WufooConfig {
    private $account;
    private $form;
    private $api_key;
    ...

    public function __construct($account, ...) {
        $this->account = $account;
        ...
    }
}

2, Extract the then and else branches of the if in the wufoo_entries() function:

if ($refresh) {
    $results = fetchContent(...);
    saveToCache($results, ...); // write file here
    return $results
} else {
    return getCachedContent();
}

It makes your code easier to read, you'll see the big picture immediately, it won't be distracted with low-level API calls.

For the same reason I'd create some new functions, for example:

functio entryCount(...)
    $count = wufoo_api($api_key, $account, $form, $offset, $limit, $api_uri);
    $counted = $count['EntryCount'];
    return $counted;
}

The name of the function says what you want to get and the body shows how you do it.

3, Change

for ($offset = 0; $offset <= $counted - 1; ...

to

for ($offset = 0; $offset < $counted;

It's more common.

4, In the inner for loop the $result loop variable is a little bit confusing, it's easy to mix up with $results. $resultIndex would be better.

for ($resultIndex = 0; $resultIndex < $limit; $resultIndex++) {
    array_push($all_results, $results['Entries'][$resultIndex]);
}

Finally, I agree with @Yannis Rizos, the whole logic would be better as a class.


ASP.NET MVC models that can be validated without using attributes The model looks like this, with no attributes:

public class PersonalModel : Validatable
{
    public string Name { get; set; }
    public string Email { get; set; }

    public override void Validate(dynamic item)
    {
        if (this.ValidatesPresenceOf(item.Name, "Name is required"))
        {
            if (((String)item.Name).Length > 5)
            {
                Errors.Add("name is too long less than 5 chars please!");
            }
        }
    }
}

and the action looks like this:

    [HttpPost]
    [ValidateAntiForgeryToken]
    public ActionResult PersonalInfo(PersonalModel model)
    {
        if (model.IsValid(model))
        {
            this.FlashInfo("Your Personal info was successfully received");
            return RedirectToAction("ThankYou");
        }
        else
        {
            ModelState.AddModelError(string.Empty, String.Join("; ", model.Errors.ToArray())) ;
            return View(model);
        }
    }

This is the baseclass Validatable:

public class Validatable : DynamicObject
{
    //Hooks
    public virtual void Validate(dynamic item) { }

    //Temporary holder for error messages
    public IList<string> Errors = new List<string>();

    public bool IsValid(dynamic item)
    {
        Errors.Clear();
        Validate(item);
        return Errors.Count == 0;
    }

    //validation methods
    public virtual bool ValidatesPresenceOf(object value, string message = "Required")
    {
        if (value == null || String.IsNullOrEmpty(value.ToString()))
        {
            Errors.Add(message);
            return false;
        }
        return true;
    }
}

I wholeheartedly agree that the attributes are evil.
But in my opinion having to derive from a base class is just as bad as having to add attributes. Both makes you depend on framework infrastructure you don't need to depend on. (Models should be model logic, and model logic only.)

In MVC (3 at least) you can implement a ModelValidatorProvider and a ModelMetadataProvider. The validator provider can be added in Application_Start using ModelValidatorProviders.Providers.Add(...), and the metdata provider can be injected using the standard IoC container.

The guys who wrote Professional ASP.NET MVC 3 also created fluent implementations of those, so you can have configuration like this:

validationProvider.ForModel<PersonModel>()
    .ForProperty(p => p.Name).Required()
    .ForProperty(p => p.Email).Required().Email();

This will also be picked up by the unobtrusive validation jQuery plugin, and everything else in MVC that would've used the attributes for.

As a sidenote, since you also derive from DynamicObject in your base class, the compiler won't complain if someone inadvertedly attempts to set person.name instead of person.Name.


Calling different bundles in parallel using ExecutorService I am working on a project in which I will be having different Bundles. Let's take an example, Suppose I have 5 Bundles and each of those bundles will have a method name process.

Below are the things, I am supposed to do-

I need to call all those 5 Bundles process method in parallel using multithreaded code and then write to the database. Meaning each bundle will return me back Map of String and String and then I am supposed to write that map into the database. I am not sure what is the right way to do that? Should I have five thread? One thread for each bundle? But what will happen in that scenario, suppose if I have 50 bundles, then I will have 50 threads?
And also, I want to have timeout feature as well. If any bundles is taking lot of time than the threshold setup by us, then it should get timeout and log as an error that this bundle has taken lot of time.

The following attempt that I have done is most probably flawed and error handling is by no means complete. And I am looking for best and efficient way of doing this problem.

Below is the solution I have. And below is my method which will call process method of all the bundles in a multithreaded way.

public void processingEvents(final Map<String, Object> eventData) throws InterruptedException {
    ExecutorService pool = Executors.newFixedThreadPool(5);
    List<ProcessBundleHolderEntry> entries = new ArrayList<ProcessBundleHolderEntry>();

    Map<String, String> outputs = (Map<String, String>)eventData.get(BConstants.EVENT_HOLDER);

    for (BundleRegistration.BundlesHolderEntry entry : BundleRegistration.getInstance()) {
        ProcessBundleHolderEntry processBundleHolderEntry = new ProcessBundleHolderEntry(entry, outputs);
        entries.add(processBundleHolderEntry);
    }

    try {
        List<Future<Map<String, String>>> futures = pool.invokeAll(entries, 30L, TimeUnit.SECONDS);
        for (int i = 0; i < futures.size(); i++) {
            // This works since the list of future objects are in the
            // same sequential order as the list of entries
            Future<Map<String, String>> future = futures.get(i);
            ProcessBundleHolderEntry entry = entries.get(i);
            if (!future.isDone()) {
                // log error for this entry
            }
        }
    } catch (InterruptedException e) {
        pool.shutdownNow(); //cancels the tasks
        //restore interrupted flag and exit
        Thread.currentThread().interrupt();
        //or rethrow the exception
        throw e;
    }
}

Secondly, an implementation of Callable for your threads:

public class ProcessBundleHolderEntry implements Callable<Map<String, String>> {
    private BundleRegistration.BundlesHolderEntry entry;
    private Map<String, String> outputs;

    public ProcessBundleHolderEntry(BundleRegistration.BundlesHolderEntry entry, Map<String, String> outputs) {
        this.entry = entry;
        this.outputs = outputs;
    }

    public Map<String, String> call() throws Exception {
        final Map<String, String> response = entry.getPlugin().process(outputs);
        // write to the database.
        System.out.println(response);
        return response;
    }
}

Can anyone tell me whether there is any problem with the above approach or is there any better and efficient way of doing the same thing? I am not sure whether there is any thread safety issue as well.

Any help will be appreciated on this.

I need to call all those 5 Bundles process method in parallel using
  multithreaded code and then write to the database. I am not sure what
  is the right way to do that? Should I have five thread? One thread for
  each bundle? But what will happen in that scenario, suppose if I have
  50 bundles, then I will have 50 threads?

It is recommended not to have more threads than the number of physical cpu's on that machine in order to be efficient. So if you have 50 Bundles and 8 cores for instances than an ExecutorService with 8 threads should be enough. The bundles will be executed 8 at a time and at most there will be 50/8 rounds of execution. (In real world some of the bundles will execute faster, etc.)

  And also, I want to have timeout feature as well. If any bundles is
  taking lot of time than the threshold setup by us, then it should get
  timeout and log as an error that this bundle has taken lot of time.

You could use a Timer (ScheduledExecutor) on which you register the action to take place in case of a timeout.
On timeout you should iterate the list of futures and cancel the ones that are not done yet.

Otherwise the code looks good enough. I can't see why should not be optimal.


Euler Project 14 (Longest Collatz Sequence) Project Euler 14 asks: Which starting number, under one million, produces the longest Collatz sequence?

I've been trying to optimize this solution to execute in less then 1s. Agenda for first 50 eulers. Currently it executes in 1,2s for me, so I guess that I want it to be 20% faster. Any tips or other methods?

from time import time

def rec_col(n, cache):
    if n in cache:
        return cache[n]
    x = (rec_col(n//2, cache) if n % 2 == 0 else rec_col(3*n+1, cache))+1
    cache[n] = x
    return x

def main():
    start = time()
    highest = 0
    ans = 1
    cache = {1: 1}
    for i in range(1, 10**6):
        temp = rec_col(i, cache)
        if temp > highest:
            ans = i
            highest = temp
    print("{0}\nTime {1:.4f} s".format(ans, time() - start))

if __name__ == '__main__':
    main()

This has the same preformance:

def rec_col(n, cache):
    cache[n] = (rec_col(n//2, cache) if n % 2 == 0 else rec_col(3*n+1, cache)) + 1 \
        if n not in cache else cache[n]
    return cache[n]

Using a 3.5 interpreter, things yielding "negative speed-up" included:

@functools.lru_cache(None) (mainly due to explicit base-case handling?)
substituting cache.get() and is not None for in and []
divmod(n, 2) instead of %2 and //2
"inlining the even case"

Negligible speed-up:

Binary operators instead of %2 and //2
Global cache instead of an explicit parameter

Slight speed-up:

rec_col((3*n+1) >> 1)+2

Of 1588206 cache entries, almost half are not used at all, not even one eighth twice, none more often.


A function to remove characters from a string I wanted to write a function to remove a set of characters from a string in JavaScript.

I came up with this.

How can it be improved in terms of time/space complexity?

function remove(str, chars) {
    var set = new Set(chars); // looking up presence of char is O(1)
    var arr = [...str]; // operate on an arrray rather than a string to avoid unneccessary string copying
    // What follows is O(N) I think...
    return arr.reduce((p,c) => {
        if(set.has(c)) {
            return p;
        }
        p.push(c);
        return p;
    }, []).join('');
}

remove('hello world', 'el') === 'ho word';

Don't abuse reduce. Reduce should be used when you want to reduce the list. Take using reduce to find the sum of a list: [1, 2, 3].reduce((a, b) => a + b, 0).
Use filter if you want to filter a list. arr.filter(i => !set.has(i)).
You may want to instead use regex.replace. However, it'd require checking your chars for any regex escape stuff.

However if you ignore that, it could be as easy as:

function remove(str, chars) {
  return str.replace(new RegExp(`[${chars}]`, 'g'), '');
}

Otherwise using the above, for \$O(n)\$ time and space complexity, could get you:

function remove(str, chars) {
    var set = new Set(chars);
    return [...str].filter(i => !set.has(i)).join('');
}


Functional Conway's Game of Life in JavaScript In this code, I tried to integrate my little knowledge of (and great fascination towards) functional programming. I'd like to make this code more functional, and faster, if that's possible.

'use strict';
var insert = require('./insert');

const STEP = 50;
const WIDTH = 200, HEIGHT = 100;
var ptr  = [[0, 1, 0, 0, 0, 0, 0],
            [0, 0, 0, 1, 0, 0, 0],
            [1, 1, 0, 0, 1, 1, 1]];

function init(scr) {
    var cwObj = {
        canvas: document.getElementById(scr),//set up the graphics department
        gMap: new Array(HEIGHT).fill(new Array(WIDTH).fill(0))
    };
    draw(cwObj, ptr, 30, 60);
    step(cwObj.gMap);
    return cwObj;
}

function draw(cwObj, pattern, x = 0, y = 0) {
    if(pattern){//draw pattern to map
        pattern.map(function (cur, i){
            cwObj.gMap[x + i] = insert(cwObj.gMap[x + i], cur, y);
            //insert() is a helper, just inserts at an index
        });
    }
    var out = '';
    cwObj.gMap.map(function (cur) {     //create a string of active and
        cur.map(function (cell) {       //inactive cells
            if(cell) {out += "&#9632;";} else {out += "&#9632;";}
        });
        out += "
";
    });
    cwObj.canvas.innerHTML = out;  //put it in the HTML
}

function step (gm){
    return gm.map(function (cur, y) {
        return cur.map(function (cell, x){
            var n = getNb(gm, x, y);
            n = cell === 1 ? n - 1 : n;//getNb includes the cell itself
            return n === 3 ? 1 : ((n === 2 && cell === 1) ? 1 : 0);//rules
        });
    });
}

function getNb (gm, x, y){
    var w = WIDTH, h = HEIGHT,
        xmin = (x <= 0) ? 0 : x-1, ymin = (y <= 0) ? 0 : y-1,//handle borders
        xmax = (x+2 > w) ? w : x+2, ymax = (y+2 > h) ? h : y+2;

    var env = gm.slice(ymin, ymax).map(function (row){//get environment array
        return row.slice(xmin, xmax);
    });

    env = [].concat.apply([], env);//flatten array

    return env.reduce(function (prev, cell) {//sum the neighbors
        return prev += cell;
    });
}

var Game = init("screen");

setInterval(function (){//repeat the steps every 50ms
    Game.gMap = step(Game.gMap);
    draw(Game);
}, STEP);

const STEP = 50;
const WIDTH = 200, HEIGHT = 100;

var w = WIDTH, h = HEIGHT,
    xmin = (x <= 0) ? 0 : x-1, ymin = (y <= 0) ? 0 : y-1,//handle borders
    xmax = (x+2 > w) ? w : x+2, ymax = (y+2 > h) ? h : y+2;

Be consistent. I usually recommend a const/let/var per variable and a line each to to make it easy to add and remove variables as needed. No surgical removal of variables in between commas and it's easy to spot the variables.

var Game = init("screen");

setInterval(function (){//repeat the steps every 50ms
    Game.gMap = step(Game.gMap);
    draw(Game);
}, STEP);

The problem with above is that it's not kosher in terms of functional programming. Each step is mutating Game, specifically Game.gmap. If I remember correctly, the functional way to create a game loop is to use recursion and construct a new game state on each step.

function gameLoop(gameState){
  draw(gameState);
  const newState = update(gameState);
  return gameLoop(newState);
}

return gameLoop(createInitialState());

The big difference is that you don't have Game or Game.gmap that mutates on each step. Each step takes in an entirely different state object than the previous. This is inefficient given existing JS data structures, but can be optimized using persistent data structures.

But JS has no TCO yet and you'll blow away the stack if you do it in existing engines (Node.js has it under a flag). The closest you can get to maintaining a similar code structure is by scheduling the next step asynchronously using setTimeout.

function gameLoop(gameState){
  draw(gameState);
  const newState = update(gameState);
  setTimeout(() => gameLoop(newState), STEP);
}

gameLoop(createInitialState());

function init(scr) {
    var cwObj = {
        canvas: document.getElementById(scr),//set up the graphics department
        gMap: new Array(HEIGHT).fill(new Array(WIDTH).fill(0))
    };
    draw(cwObj, ptr, 30, 60);
    step(cwObj.gMap);
    return cwObj;
}

I would suggest this init function only construct state. Drawing should just be a part of the game loop.

function createInitialState(){
  return { /* initial game state */ }
}

pattern.map(function (cur, i){
    cwObj.gMap[x + i] = insert(cwObj.gMap[x + i], cur, y);
    //insert() is a helper, just inserts at an index
});

var out = '';
cwObj.gMap.map(function (cur) {     //create a string of active and
    cur.map(function (cell) {       //inactive cells
        if(cell) {out += "&#9632;";} else {out += "&#9632;";}
    });
    out += "
";
});

You are misusing array.map. array.map is meant to be a 1:1 transform of each item in an array into another. What you're doing here is a job for array.forEach. And since array.forEach does nothing but loop and return undefined, it has to be mutating something outside the callback, making it not functional.

Your first operation could be done with an array.map as it's essentially updating each value with a new value. You will have to replace the array, not mutate the existing one.

cwObj.gMap = pattern.map((current, i) => updatedValueForValueAt(i));

The second one appears to be a job for array.reduce as it is reducing the array into a single string.

const out = cwObj.gMap.reduce((prev, cur) => prev + cur.map(c => c? '&#9632;' : '&#9632;').join(''), '');

JS isn't entirely functional and you will often break rules where necessary. At best, you keep mutation to a minimal.