ActivityLifecycleHelper implementation Description

The ActivityLifecycleHelper uses wrap(Context)to create the class and save a global Application field. It then uses with(ActivityCallbacks) to register android.app.Application.ActivityLifecycleCallbacks on the Application field. Whenever onActivityResumed(Activity) is triggered the class calls a null  OnSaved(Bundle). Whenever onActivitySaveInstanceState(Activity, Bundle) is triggered the class calls a saved/null OnSaved(Bundle). It then unregisters the android.app.Application.ActivityLifecycleCallbacks when onActivityDestroyed(Activity) is triggered. Finally, it returns the base context of the Application field.

ActivityLifecycleHelper.class

public class ActivityLifecycleHelper {

    public interface ActivityCallbacks {
        void OnSaved(Bundle savedInstance);
    }

    private final Application application;

    private ActivityLifecycleHelper(Application application) {
        this.application = application;
    }

    public static ActivityLifecycleHelper wrap(Context context) {
        return new ActivityLifecycleHelper(assertApplication(context));
    }

    public Context with(final ActivityCallbacks callbacks) {
        application.registerActivityLifecycleCallbacks(new ActivityLifecycleCallbacks() {
            @Override
            public void onActivityCreated(Activity a, Bundle b) {}

            @Override
            public void onActivityStarted(Activity a) {}

            @Override
            public void onActivityResumed(Activity a) {
                callbacks.OnSaved(null);
            }

            @Override
            public void onActivityPaused(Activity a) {}

            @Override
            public void onActivityStopped(Activity a) {}

            @Override
            public void onActivitySaveInstanceState(Activity a, Bundle b) {
                callbacks.OnSaved(b);
            }

            @Override
            public void onActivityDestroyed(Activity a) {
                application.unregisterActivityLifecycleCallbacks(this);
            }
        });
        return application.getBaseContext();
    }

    private static Application assertApplication(Context context) {
        final Context application = context.getApplicationContext();
        if (application instanceof Application) return (Application) application;
        throw new NullPointerException("Context must be instance of Application");
    }
}

Usage

public class Usage implements ActivityLifecycleHelper.ActivityCallbacks {

    private final Context context;

    Usage(Context context) {
        this.context = ActivityLifecycleHelper.wrap(context).with(this);
    }

    @Override
    public void OnSaved(Bundle savedInstance) {
        // doingit
    }
}

Reasoning

This is used within a library that when initialized receives a context that will extend from an Application and hooks into any Activity LifeCycle events.

OnResume() will allow the library to reset some things and onSaveInstanceState() is so the library can rebuild.

Needed

Will this approach register/unregister appropriately? Is anything bad design?
 Will this cause any context leaks?

Yes. It seems like a bad design or I would say not thought through given that you're building a library project which will be used by other apps.

First of all,
in your library initialization, you should ask for Application reference rather than Context reference.

For example,

if your library has an init method like this,

MyLibrary.init(Context applicationContext);

then it should be converted to accept Application object like this,

MyLibrary.init(Application application);

Then,

inside that init method, you should just register your class which is implementing Application.ActivityLifecycleCallbacks.

application.registerActivityLifecycleCallbacks(ActivityLifecycleHelper.getInstance());

Make sure your class is a singleton. In your example, your ActivityLifecycleHelper class is not a singleton which is definitely a bad design pattern given that you're dealing with global level callbacks and want to initialize only once.

It might look like this,

public class ActivityLifecycleHelper implements Application.ActivityLifecycleCallbacks {
private static ActivityLifecycleHelper ourInstance = new ActivityLifecycleHelper();

private ActivityLifecycleHelper() {
}

public static ActivityLifecycleHelper getInstance() {
    return ourInstance;
}

@Override
public void onActivityCreated(Activity activity, Bundle bundle) {
}

@Override
public void onActivityStarted(Activity activity) {
}

@Override
public void onActivityResumed(Activity activity) {
}

@Override
public void onActivityPaused(Activity activity) {
}

@Override
public void onActivityStopped(Activity activity) {
}

@Override
public void onActivitySaveInstanceState(Activity activity, Bundle bundle) {

}

@Override
public void onActivityDestroyed(Activity activity) {

}
}

Also, I don't understand why you have two classes here. What's the use case of Usage class ? Something seems wrong in this two class design.

I hope this helps. Let me know if you want me to elaborate anything.


Mapping DataTable to a concrete type I have an extension that adds values to a class and I was wondering if there was a faster way to do this.

Does a for loop execute faster or would a LINQ query do better?

public static List<T> DataTableToList<T>(this DataTable dt) where T : class, new()
{
    List<T> dataTableList = new List<T>();
    var rowsCount = dt.Rows.Count;
    var columnCount = dt.Columns.Count;

    for (int i = 0; i < rowsCount; i++)
    {
        T obj = new T();
        int j = -1;
        Type type = obj.GetType();
        PropertyInfo[] properties = type.GetProperties();

        foreach (PropertyInfo property in properties)
        {
            if (j == -1)
            {
                property.SetValue(obj, Guid.NewGuid());
            }
            else if (j < columnCount)
            {
                if (dt.Rows[i][j] == null) continue;
                property.SetValue(obj, dt.Rows[i][j].ToString());
            }
            j++;
        }
        dataTableList.Add(obj);
    }
    return dataTableList;
}

I wouldn't say, your major concern should be performance. I find the method rather error prone and it has a very limited use:

1) All columns have to be of type string - except the first which must be of type Guid

2) The order of the columns in the table must match the order of the properties of the object type. If they don't, a column could easily be mapped with a wrong property without no warning because they must all be strings. You definitely need some type checking.

3) A data table usually has a (unique) Id column, which the instances of the class should be instantiated with, but it seems, that you create a new object Id for every new instance/row. Is that Id saved to the database? And what about the next time the Row/Item are mapped - is a new Id again created for that item? It seems not logical to have a new id for an item every time it is fetched from a database?

I would say that the method is so aimed at a special situation where you need to map a data table with a certain class that it is not a candidate as an extension method - but just as a private method in a class. An extension method should be generic as much as possible and should handle all possible errors in a consistent way.

What about private vs. public members?

I personally don't like the use of an indexed iteration here: I would go along these lines:

  PropertyInfo[] pis = typeof(T).GetProperties();

  foreach (DataRow row in dt.Rows)
  {
    T item = new T();

    foreach (DataColumn column in dt.Columns)
    {
      PropertyInfo pi = pis.FirstOrDefault(p => p.Name == column.ColumnName);
      if (pi != null)
      {
        pi.SetValue(item, row[column]);
      }
    }
  }

or

  PropertyInfo[] pis = typeof(T).GetProperties();

  foreach (DataRow row in dt.Rows)
  {
    T item = new T();

    foreach (PropertyInfo pi in pis)
    {
        pi.SetValue(item, row[dt.Columns[pi.Name]]);
    }
  }

Notice, that the call to typeof(T).GetProperties() is outside the loops, because there is no need to call it for every row/item. In this way you are half the way to a nearly completely generic extension method.

In order to make it more generic, you'll have to consider the mapping strategy:

1) Should the column names match the property names of the class or should you consider to be able to provide a mapping table?:

public static List<T> DataTableToList<T>(this DataTable dt, Dictionary<string, string> mappings) where T : class, new()

or:

public static List<T> DataTableToList<T>(this DataTable dt, (string column, string property)[] mappings) where T : class, new()

You'll also have have to consider conversion of certain types: for instance is DateTime often stored in a data table as SqlDateTime which can not implicitly cast to a DateTime structure, so you should maybe be able to provide some converters:

public static List<T> DataTableToList<T>(this DataTable dt, (string column, string property)[] mappings, Dictionary<string, Func<object, object>> converters) where T : class, new()

called like:

    Dictionary<string, Func<object, object>> converters = new Dictionary<string, Func<object, object>>
    {
      { "BirthDay", (sqlDate) => ((SqlDateTime)sqlDate).Value }
    };

    List<MyItem> items = table.DataTableToList<MyItem>(null, converters);

Both the mapping and converter strategy can probably be more elegant or sophisticated - the above is merely some quick suggestions.


Python/Django Filter with Datetime and extra options I'm creating a filter in python/django. I want to make a datetime human-readable.
You have multiple options:
Plain Datetime ("04. Februar 2018, 18:18")

(bools="False,False"),

sentence Datetime ("am 04. Februar 2018 um 18:18")

(bools="True,False"),

onlydate Datetime ("04. Februar 2018")

(bools="False,True"),

sentence and onlyDate ("am 04. Februar 2018")

(bools="True,True")

Here's my filter:

@register.filter
def naturaldaytime(daytime, bools="False,False"):
    if daytime is None:
        return daytime

    sentence = str2bool(bools.split(",")[0])
    onlyDate = str2bool(bools.split(",")[1])

    now = datetime.datetime.now()
    difference = now.day - daytime.replace(tzinfo=None).day
    loc = locale.setlocale(locale.LC_TIME, "de")

    if onlyDate:
        if difference == 0:
            return daytime.strftime(settings.DATE_FORMAT_TODAY) if not sentence else daytime.strftime(settings.DATE_FORMAT_SENTENCE)
        elif difference == 1:
            return daytime.strftime(settings.DATE_FORMAT_YESTERDAY) if not sentence else daytime.strftime(settings.DATE_FORMAT_YESTERDAY)
        elif now.isocalendar()[0] == daytime.isocalendar()[0]:
            if now.isocalendar()[1] == daytime.isocalendar()[1]:
                return daytime.strftime(settings.DATE_FORMAT_THIS_WEEK) if not sentence else daytime.strftime(settings.DATE_FORMAT_THIS_WEEK_SENTENCE)
            return daytime.strftime(settings.DATE_FORMAT_THIS_YEAR) if not sentence else daytime.strftime(settings.DATE_FORMAT_THIS_YEAR_SENTENCE)
        return daytime.strftime(settings.DATE_FORMAT) if not sentence else daytime.strftime(settings.DATE_FORMAT_SENTENCE)
    else:
        pass
        #so what...

def str2bool(v):
    return v.lower() in ("yes", "true", "t", "1")

and my settings vars:

TIME_FORMAT = "%H:%M"

DATE_FORMAT = "%d. %B %Y, {0}".format(TIME_FORMAT)
DATE_FORMAT_THIS_YEAR = "%d. %B, {0}".format(TIME_FORMAT)
DATE_FORMAT_THIS_WEEK = "%A, {0}".format(TIME_FORMAT)
DATE_FORMAT_YESTERDAY = "Gestern, {0}".format(TIME_FORMAT)
DATE_FORMAT_TODAY = "Heute, {0}".format(TIME_FORMAT)

DATE_FORMAT_SENTENCE = "am %d. %B %Y um {0} Uhr".format(TIME_FORMAT)
DATE_FORMAT_THIS_YEAR_SENTENCE = "am %d. %B um {0} Uhr".format(TIME_FORMAT)
DATE_FORMAT_THIS_WEEK_SENTENCE = "diesen %A um {0} Uhr".format(TIME_FORMAT)
DATE_FORMAT_YESTERDAY_SENTENCE = "gestern um {0} Uhr".format(TIME_FORMAT)
DATE_FORMAT_TODAY_SENTENCE = "heute um {0} Uhr".format(TIME_FORMAT)

Set your locale globally

I would recommend doing this in your settings.py file or within a global __init__.py file.

locale.setlocale(locale.LC_TIME, "de")

Django template tags allow multiple parameters, use them

Right now you have a single complex parameter which is being used to store two pieces of information: whether to use a sentence and if only the date should be rendered. Since this is passed in as a single string, you have to use the str2bool function from Stack Overflow in order to parse it. You don't need to parse that string if you use separate arguments.

@register.filter
def naturaldaytime(daytime, sentence=False, only_date=False):
    from django.conf import settings

    if daytime is None:
        return daytime

Use descriptive variable names in your comparisons

Right now you have to look into nested calls to understand what comparisons are actually being done. While this still needs to happen, you can make us to intermediate variables which makes it a lot more clear what is happening.

todays_date = datetime.now().date()
yesterdays_date = todays_date - timedelta(days=1)

daytime_week_number = daytime.date().isocalendar()[1]
todays_week_number = todays_date.isocalendar()[1]

Construct the format key you need so you can avoid repetition

Right now you have a complex set of conditional statements that you are using to determine what date format to use. This is resulting in a ton of repetition when it comes to doing the actual formatting, so I would recommend separating the "getting the right format settings" part from the actual "do the formatting".

if not only_date:
    setting_key = "DATETIME_FORMAT"
else:
    setting_key = "DATE_FORMAT"

if daytime.date() == todays_date:
    setting_key += "_TODAY"
elif daytime.date() == yesterdays_date:
    setting_key += "_YESTERDAY"
elif daytime_week_number == todays_week_number:
    setting_key += "_THIS_WEEK"
elif daytime.date().year == todays_date.year:
    setting_key += "_THIS_YEAR"

if sentence:
    setting_key += "_SENTENCE"

This way you don't need to keep repeating the same logic for every "sentence" or "not only the date" variant, and you can keep settings keys consistent at the same time.

Get the right date format using that settings key you already built

This part doesn't change much, except you now need to dynamically retrieve the setting from the settings object.

date_format = getattr(settings, setting_key)

return daytime.strptime(date_format)


Draw line that expands over time This is a simple practice app that draws a line, and the line 'balloons' up for a second.

This code shows some issues I tend to have a little too often, where I end up defining functions inside of functions, and it feels like there's got to be a better way.

I'm also unsure of the way I updated the size and pos values in the line_expand function.

Any suggestions on how I can write cleaner code in this example?

from kivy.app import App
from kivy.uix.widget import Widget
from kivy.graphics import Color, Rectangle, Line, Ellipse
from kivy.clock import Clock

class Test(Widget):

    def on_touch_down(self, touch):
        with self.canvas:
            Color(0, 0, 0)
            Rectangle(pos=(0, 0), size=(self.width, self.height))
            Color(1, 1, 1)
            touch.ud['ellipse'] = Ellipse(pos=(touch.x - 10, touch.y - 10), size=[20, 20])
            touch.ud['line'] = Line(points=[touch.x, touch.y], width=10)

    def on_touch_move(self, touch):
        touch.ud['line'].points += [touch.x, touch.y]

    def on_touch_up(self, touch):

        def line_expand(dt):
            touch.ud['ellipse'].size = (touch.ud['ellipse'].size[0] + 1,
                                        touch.ud['ellipse'].size[1] + 1)
            touch.ud['ellipse'].pos = (touch.ud['ellipse'].pos[0] - 0.5,
                                       touch.ud['ellipse'].pos[1] - 0.5)
            touch.ud['line'].width += 0.5

        growclock = Clock.schedule_interval(line_expand, 0.01)
        def stop_clock(dt):
            Clock.unschedule(growclock)

        Clock.schedule_once(stop_clock, 1)

class TestApp(App):
    def build(self):
        return Test()

if __name__ == '__main__':
    TestApp().run()

At the bottom is a new version, addressing most of the issues you're wondering about. But let us give a review to explain the changes:

Intermixing functions and code – In your on_touch_up you are intermixing code with the new functions. That is not good. You can introduce functions within a functions, but if you do so, then do it at the top of the function, so that it is very clear where you actual function body starts, and where the other functions are defined.
Alternative to function within function – There does however exist alternatives to those recursive function definition, and I'll present two lambda's and partial functions:

functools.partial – The other inside function you had was the call to line_expand(), and this would be nicer, in my opinion, if you put in the class scope. This does however need for more information to be passed, i.e. the touch data structure, so the definition for it to be a valid function is:

def _line_expand(self, touch, dt):

However, this is also used as a callback, so it should only take that one parameter, i.e. dt. To accomodate both these demands, you can use functools.partial to prefill a parameter like this:

growclock = Clock.schedule_interval(partial(self._line_expand, touch), 0.01)

lambda – In some cases, like a callback which will actually only do that one thing, you could use lambda's which is kind of a fake function for one expression.

Clock.schedule_once(lambda dt: Clock.unschedule(growclock), 1)

These two tricks allows you to remove both of the inner functions in your original on_touch_up(), and the full alternate version is presented below as alt_on_touch_up()
Combine the animation sequence into a method – Instead of having to always combine the two operations you could make a method which does the animation for you, with the given interval time, and the end time. This is presented below in the animate() function. Now the kind of 'messy' Clock.schedule stuff is hidden away in that method.
Alternate ways to change a tuple – A tuple in Python is immutable, or unchangeable. You have to replace it, but how you replace it is another story. In the new _line_expand() I'll show two alternate methods, one which lifts out the values from the tuple to dedicated values, and one which makes an alias for the touch.ud['ellipse'] part, which makes the changes a little easier on the eye.
Comments are good! – Try working on giving some comments or docstrings on your code, as it will help you and others understand what happens.

So without further ado, here is the revamped version of class Test:

from kivy.app import App
from kivy.uix.widget import Widget
from kivy.graphics import Color, Rectangle, Line, Ellipse
from kivy.clock import Clock

from functools import partial

class Test(Widget):

    def _line_expand(self, touch, dt):
        """Expands the size of the ellipse, and width of line"""

        # Extract values in front of setting new
        width, height = touch.ud['ellipse'].size
        touch.ud['ellipse'].size = (width +1, height +1)

        # Create an alias for later use
        ellipse = touch.ud['ellipse']
        ellipse.pos = (ellipse.pos[0] - 0.5,
                       ellipse.pos[1] - 0.5)

        # ellipse.size = (ellipse.size[0] + 1, ellipse.size[1] +1)

        touch.ud['line'].width += 0.5

    def on_touch_down(self, touch):
        """Clear canvas, and create an ellipse and line starting at touch down point."""
        with self.canvas:
            Color(0, 0, 0)
            Rectangle(pos=(0, 0), size=(self.width, self.height))
            Color(1, 1, 1)
            touch.ud['ellipse'] = Ellipse(pos=(touch.x - 10, touch.y - 10), size=[20, 20])
            touch.ud['line'] = Line(points=[touch.x, touch.y], width=10)

    def on_touch_move(self, touch):
        """Add points to the line when dragging mouse"""
        touch.ud['line'].points += [touch.x, touch.y]

    def on_touch_up(self, touch):
        """When releasing touch, call animation to expand line and ellipse."""

        self.animate(partial(self._line_expand, touch), 0.01, 1)

    def alt_on_touch_up(self, touch):
        """First rewrite of on_touch_up."""

        growclock = Clock.schedule_interval(partial(self._line_expand, touch), 0.01)
        Clock.schedule_once(lambda dt: Clock.unschedule(growclock), 1)

    def animate(self, animate_func, interval_time, end_time):
        """Schedule animate_func every interval_time for end_time seconds."""

        animation = Clock.schedule_interval(animate_func, interval_time)
        Clock.schedule_once(lambda dt: Clock.unschedule(animation), end_time)

The TestApp and main call code is untouched, so keep them as they were. Hope this gives some hint on how to avoid or make it cleaner when using inner functions.


Two BDD unit tests for an account repository with identical code I am testing my repository class:

interface AccountRepository{
    void save(Account account);
    Optional<Account> findById(Long id);
}

And when writing tests, I ended up with two semantically different tests, but they essentially contain exactly the same code:

@DisplayName("user exists -- account does not exist -- save persists")
@Test
void save_userExistsAccountDoesNotExist_persisted() {
    //arrange
    User existingUser = userRepo.save(new User("John", "Smith"));
    Account account = new Account(existingUser, null);
    //act
    repo.save(account);
    //assert
    assertThat(repo.findById(account.getId())).contains(account);
}

@DisplayName("user exists -- account exists -- findById finds")
@Test
void findById_userExistsAccountExists_found() {
    //arrange
    User existingUser = userRepo.save(new User("John", "Smith"));
    Account existingAccount = repo.save(new Account(existingUser, null));
    //act & assert
    assertThat(repo.findById(existingAccount.getId())).contains(existingAccount);
}

I don't really want to remove one of them because that serves different purposes in my head but I am worried that I may have understood something wrong.

One of the hardest piece of knowledge to learn as a developer is: code duplication does not mean responsibility duplication.

Having said this, you are testing different methods so it's ok to have more than one test. In this case, you are testing complementary (or were they supplementary? I never remember it) methods, so to test one, you need to have the other one working properly.

However, there are some code duplication that will bite you in the future (but since this question is from the past, you can tell me if I'm wrong or not).

You are duplicating the way a User entity is created, so if you change its constructor, you will have to modify your tests, even though it might not matter in the actual logic you are testing. It happens something similar with the Account entity. To avoid this you can have a private method or to a public builder.
You are duplicating the way a User is created and persisted. The same problem as in point #1 happens. You probably don't care how or when is created, you just need it to be created and persisted properly. This applies also for the Account entity with a slight different: in the first test you need to make the code of save into the repo explicit since it's what you are testing.

Besides all of this, when you are writing tests you need to tell the reader what's important and what's not and describe that accordingly. It's a balance between explicitness and abstraction. For instance in the example, I've written a method called createAccount to encapsulate the logic of creation and persistence of the Account entity. However, someone would say that if a developer reading the test needs to know what the arrange does, it has to move to the method implementation (a test should be self-explanatory and auto).

@DisplayName("user exists -- account does not exist -- save persists")
@Test
void save_userExistsAndAccountDoesNotExist_accountIsSaved() {
    // Arrange
    User existingUser = createUser();
    Account createdAccount = makeAccount(existingUser);

    // Act
    repo.save(createdAccount);

    // Assert
    assertThat(repo.findById(createdAccount.getId())).contains(createdAccount);
}

@DisplayName("user exists -- account exists -- findById finds")
@Test
void findById_userExistsAndAccountExists_returnTheAccount() {
    // Arrange
    User existingUser = createUser();
    Account existingAccount = createAccount(existingUser);

    // Act
    Optional<Account> foundAccount = repo.findById(existingAccount.getId());

    // Assert
    assertThat(foundAccount).contains(existingAccount);
}

private User createUser(){
    return userRepo.save(new User("John", "Smith"));
}

private Account createAccount(User existingUser){
    return repo.save(makeAccount(existingUser));
}

private Account makeAccount(User user){
    new Account(user, null);
}


ASP.NET Web API HTTP GET with optional parameters Writing my first ASP.NET Web API web service. Still learing the power of Linq as well. I feel like there is a more ideomatic way of doing this:

public class StylesController : ApiController
{
    private Entities db = new Entities();

    // GET api/Styles
    public IEnumerable<Style> GetStyles(int division = 0, int family = 0, int model = 0, int year = 0, string market = "ANY")
    {
        market = market.ToUpper(); // make sure market is upper case

        IEnumerable<Style> q = db.Styles;

        if (division > 0) { q = q.Where(s => s.DivisionId == division); }
        if (family > 0) { q = q.Where(s => s.FamilyId == family); }
        if (model > 0) { q = q.Where(s => s.ModelId == model); }
        if (year > 0) { q = q.Where(s => s.Year == year); }
        if (market != "ANY") { q = q.Where(s => s.MarketCode == market); }

        return q.Distinct().OrderBy(s => s.Id).ToList();
    }
}

Good layout and white space.

Although the logic works, I think it is very confusing, and not overly obvious.  At first glance, it the if statements are very confusing.  I would also rename the variables to have an Id or Code to the end of them to keep naming consistent.

I think this is a good place where you could incorporate the Specification Pattern.

public class StylesController : ApiController
{
    private Entities db = new Entities();

    public IEnumerable<Style> GetStyles(int divisionId = 0, int familyId = 0, int modelId = 0, int year = 0, string marketCode = "ANY")
    {
        var divisionIsAMatch = new DivisionIsAMatchSpecification(divisionId);
        var familyIsAMatch = new FamilyIsAMatchSpecification(familyId);
        var modelIsAMatch = new ModelIsAMatchSpecification(modelId);
        var yearIsAMatch = new YearIsAMatchSpecification(year);
        var marketIsAMatch = new MarketIsAMatchSpecification(marketCode);

        var q = db.Styles

        return q.Where(
                divisionIsAMatch
                .And(familyIsAMatch)
                .And(modelIsAMatch)
                .And(yearIsAMatch)
                .And(marketIsAMatch).IsSatisfiedBy
            ).Distinct()
           .OrderBy(s => s.Id);
    }
}

Where

internal class DivisionIsAMatchSpecification : Specification<Style>
{
    private readonly int _divisionId;

    public DivisionIsAMatchSpecification(int divisionId)
    {
        _divisionId = divisionId;
    }

    public bool IsSatisfiedBy(Style style)
    {
        if (_divisionId == 0)
        {
            return true;
        }

        return style.DivisionId == _divisionId;
    }
}

Just rinse and repeat for each of the specifications you need.  Of course, you will have to create the base classes for them too.

I haven't tested this, but I have used this pattern before, and it works really nicely.


Snakes and Ladders random world generator second implementation The following is an implementation of a random world generator for the classic Snakes and Ladders game. This is a follow up to this question, where I reviewed my own original implementation. I am asking for a review of my review.

The world rules are:

The head of a snake can not be on the last square (you'll never win)
The base of a ladder can not be on the last square (you'll have no where to go)
A square can only have one thing, either the head or base of a ladder or a snake

class World
  attr_reader :size, :snakes, :ladders

  def initialize(size, snakes = 10, ladders = 10)
    @size = size
    @snakes, @ladders = {}, {}
    place_snakes(snakes)
    place_ladders(ladders)
  end

  def place_snakes(count)
    count.times do
      start_point = place_point
      end_point_range = habitable_range.begin..(start_point - 1)

      snakes[start_point] = place_point(end_point_range)
    end
  end

  def place_ladders(count)
    count.times do
      start_point = place_point
      end_point_range = (start_point + 1)..habitable_range.end

      ladders[start_point] = place_point(end_point_range)
    end
  end

private

  def place_point(range = habitable_range)
    begin
      point = rand(range)
    end while occupied?(point)
    point
  end

  def occupied?(number)
    [snakes.keys, snakes.values, ladders.keys, ladders.values].flatten.include?(number)
  end

  def habitable_range
    1..(size - 1)
  end
end

Here's an attempt:

class World
  attr_reader :board

  def initialize(size, snakes_count = 10, ladders_count = 10)
    @board = Array.new(size * size)

    place snakes_count,  :snake
    place ladders_count, :ladder
  end

private

  def free_point
    point = rand(@board.size) until point && @board[point].nil?
    point
  end

  def place(count, entity)
    count.times do
      start_point = free_point
      end_point   = free_point until end_point && end_point != start_point && (entity == :ladder || end_point != @board.size - 1)
      pair = [start_point, end_point].sort
      pair.reverse! if entity == :snake
      @board[pair.first] = @board[pair.last] = pair
    end
  end
end

The idea here is that:

A snake or ladder consists of a point pair. A snake is a desending pair (ie, go back from a higher point to a lower point), while a ladder is an ascending pair (go forward from a lower point to a higher point)
A snake cannot have either of its points be the final point on the board.

The board is represented as a size x size array, so a world of size 12 would produce a board that is 12x12, or 144 squares. Checking for occupation is then as easy as just checking if @board[point].nil?. It also gives you an easy way to iterate the board for rendering or rules checking; as you're playing, if the player lands on square 63, you can check square 63. If it has a value, then check if v[0] > v[1] (snake) or otherwise (ladder).

If desired, you could have #place keep a separate list of snakes and ladders, but it's not strictly necessary, since you can think of them as the same construct (a movement from one point to another), differentiated only by the direction of movement.


Computing a list of divisors of a number I developed the following Python functions when solving a few Project Euler problems, and since I'm not that familiar with Python, I'm curious at how I could improve them.

The code consists of a data holder class, and a few functions.

Option class:

This object stores data for a combination option. It has two data members:

value: the value of the option
count: how many times this option can be chosen

class Option:
    def __init__(self, value, count):
        self.value = value
        self.count = count

Get Prime Factorization:

This function calculates a number's prime factorization. It returns the factorization as a list of Options where the option's value is the prime and it's count is the power.

def getPrimeFactorization(n):
    f = []

    nth = 0

    while n > 1:
        prime = nthPrime(nth)

        if n % prime == 0:
            f.append(Option(prime, 1))

            n = n // prime

            while n % prime == 0:
                f[-1].count += 1

                n = n // prime

        nth += 1

    return f

Get Combinations:

To calculate the combinations, I use getCombinationsIncrement to iterate through each possible combination. The index is the index of the current option, options is the list of Options, and chosen is the number of times the Option will be chosen.

def getCombinationsIncrement(index, options, chosen):
    if index >= len(options):
        return False
    else:
        chosen[index] += 1

    if chosen[index] > options[index].count:
        chosen[index] = 0
        return getCombinationsIncrement(index + 1, options, chosen)
    else:
        return True

The options parameter is a list of options, combine is the function used to combine options, base is the base amount to combine with other values (it's similar to reduce if your familiar with JavaScript).

def getCombinations(options, combine, base):
    combos = [base]
    chosen = [0] * len(options)

    while getCombinationsIncrement(0, options, chosen):
        combos.append(base)

        for i in range(len(options)):
            for _ in range(chosen[i]):
                combos[-1] = combine(combos[-1], options[i].value)

    combos.sort()

    return combos

Get Divisors

Using the functions above it's easy to calculate all the divisors of a number.

def multiply(x, y):
    return x * y

def getDivisors(n):
    return getCombinations(getPrimeFactorization(n), multiply, 1)

My primary concern is with the getCombinationsIncrement function. I'd like it to be scoped only into the getCombinations function, and I don't particularly like having to make a class to fix that. It also might not be the best way to go about creating the combinations.

You code is hard to understand. Very hard.

In Python we prefer iteration over recursion, not only does recursion limit you to a maximum depth of 1000 function calls, it also has severe performance impacts.

You should:

Use snake_case in Python not camelCase. The only time you use CamelCase is when creating a class.
Start using generators. In most cases you can just replace list.append with a yield.
Change nthPrime to be a generator, say primes. As I'm not here to re-write nthPrime you can just use itertools.count to get an infinite generator.
Don't duplicate logic before and during a loop. In prime_factorization, you wrote the floor division, n //= prime, twice.
It's simpler to use a for loop and break from it, than while looping with a condition and manually taking the next item.
You can use collections.namedtuple to remove the need to create your own class.

Merging the above together for just prime_factorization can get you:

from collections import namedtuple
from itertools import count

Option = namedtuple('Option', 'value count')

def primes():
    for i in count():
        yield nthPrime(i)

def prime_factorization(n):
    for prime in primes():
        count = 0
        while n % prime == 0:
            count += 1
            n //= prime
        if count:
            yield Option(prime, count)
        if n <= 1:
            break

After this you want to move more code into combinations, so that it's self contained in getCombinationsIncrement, and to rename it combinations.

Change combinations to instead yield a tuple of chosen.
Create chosen within combinations.
Pass a single one dimensional list to combinations so that it's more usable later.
When you finish running through all of options without yielding, you want to stop. To do this you can use the for ... else syntax and just break.

This should be able to get you:

def combinations(options):
    chosen = [0] * len(options)
    while True:
        for i, option in enumerate(options):
            chosen[i] += 1
            if chosen[i] > option:
                chosen[i] = 0
                break
            yield tuple(chosen)
        else:
            break

Finally we want to change combinations_reduce to use combinations.

First you should change options from a 2d lists to two 1d lists. You should want to get values and counts into their own lists. And so you can use zip with the argument unpacking operator, *, to expand the list to \$n\$ 2 long lists.
You want to create combos off the bat, and can use enumerate to get the current index.

Alternately you could try using yield instead, and off-load the sorting to the user.
You can use reduce to replace your innermost loop.
You can change the input to use default variables of operators.mul and 1. This simplifies the usage, as you don't need to specify as much.

This can get you:

def combinations_reduced(options, combine=mul, base=1):
    values, counts = zip(*options)
    combos = [base] * len(counts)
    for i, chosen in enumerate(combinations(counts), 1):
        for j, value in enumerate(values):
            combos[i] = reduce(combine, [combos[i]] + [value] * chosen[j])
    combos.sort()
    return combos

def get_divisors(n):
    return combinations_reduced(prime_factorization(n))


Express middleware to validate URL and tags I am building an API where the client can send a url and some comma-delimited tags:

{
   "url": "https://www.youtube.com/watch?v=pquxHIBx8ks",
   "tags": "JavaScript, Sequelize"
}

Before processing this data, I want to validate the input. To do that, I wrote a middleware function called validateSubmitScreencastReq:

import config from 'config'
import youtubeUrl from 'youtube-url'
import db from 'sequelize-connect'
import * as youtubeClient from '../../source/util/youtubeClient'

async function validateUrl (url) {
  if (!url) {
    return 'url cannot be undefined'
  }

  if (!youtubeUrl.valid(url)) {
    return 'url must be a valid YouTube URL'
  }

  const client = youtubeClient.create(config.youtubeApiKey)
  if (!await client.videoExists(url)) {
    return 'url must link to an existent, public YouTube video'
  }

  const foundScreencast = await db.models.screencast.findOne({
    where: {
      url: url
    }
  })
  if (foundScreencast !== null) {
    return 'url has already been submitted'
  }

  return undefined
}

function validateTags (tags) {
  if (!tags) {
    return 'tags cannot be undefined'
  }

  if (typeof tags !== 'string') {
    return 'tags must be a string'
  }

  return undefined
}

export async function validateSubmitScreencastReq (req, res, next) {
  try {
    const urlError = await validateUrl(req.body.url)
    const tagError = validateTags(req.body.tags)
    const errors = []
    if (urlError) {
      errors.push({
        field: 'url',
        message: urlError
      })
    }
    if (tagError) {
      errors.push({
        field: 'tags',
        message: tagError
      })
    }
    if (errors.length !== 0) {
      res.status(400).json({errors})
    } else {
      next()
    }
  } catch (error) {
    next(error)
  }
}

I'm not too happy with this code.

The function essentially aggregates error messages, which makes me think of using reduce but I can't see how to make it work elegantly.

What's more, I don't think I'm expressing that the function aggregates errors very clearly.

How can I improve the readability the above code, preferably using functional concepts?

(If it's at all useful, here is the validateSubmitScreencastReq function on GitHub and here are the corresponding unit tests.)

export async function validateSubmitScreencastReq (req, res, next) {
  try {
    const errors = [
        {field: 'url', message: await validateUrl(req.body.url)},
        {field: 'tags', message: validateTags(req.body.tags)}
    ].filter((obj) => obj.message !== undefined);

    if (errors.length !== 0) {
      res.status(400).json({errors})
    } else {
      next()
    }
  } catch (error) {
    next(error)
  }
}

This removes any non-errors from the originally constructed array and should give the same end-result as your code.

You can also improve your validate url/valide tag functions:

const assert = require("assert");//common nodejs package

async function validateUrl (url) {
  try {
    assert(url, "url can not be undefined");
    assert(youtubeUrl.valid(url), 'url must be a valid YouTube URL');

    const client = youtubeClient.create(config.youtubeApiKey)

    assert(await client.videoExists(url), 'url must link to an existent, public YouTube video');
    assert(!(await db.models.screencast.findOne({where: {url: url}})), 'url has already been submitted');
  } catch (error) {
    /* remove the if, if you want to also return whatever those functions may
       have thrown as an error */
    if (error instanceof AssertionError) {
      return error.message;
    }
  }
  //No return needed if you want to return undefined
};


Exercise: Design a Stack Exercise: Design a Stack

  A Stack is a data structure for storing a list of elements in a LIFO
  (last in, first out) fashion. Design a class called Stack with three
  methods.

void Push(object obj)
object Pop()
void Clear()

  The Push() method stores the given object on top of the stack. We use
  the “object” type here so we can store any objects inside the stack.
  Remember the “object” class is the base of all classes in the .NET
  Framework. So any types can be automatically upcast to the object.
  Make sure to take into account the scenario that null is passed to
  this object. We should not store null references in the stack. So if
  null is passed to this method, you should throw an
  InvalidOperationException. Remember, when coding every method, you
  should think of all possibilities and make sure the method behaves
  properly in all these edge cases. That’s what distinguishes you from
  an “average” programmer.

  The Pop() method removes the object on top of the stack and returns
  it. Make sure to take into account the scenario that we call the Pop()
  method on an empty stack. In this case, this method should throw an
  InvalidOperationException. Remember, your classes should always be in
  a valid state and used properly. When they are misused, they should
  throw exceptions. Again, thinking of all these edge cases, separates
  you from an average programmer. The code written this way will be more
  robust and with less bugs.

  The Clear() method removes all objects from the stack.

  We should be able to use this stack class as follows:

var stack = new Stack();

stack.Push(1);
stack.Push(2);
stack.Push(3);

Console.WriteLine(stack.Pop());
Console.WriteLine(stack.Pop());
Console.WriteLine(stack.Pop());

  The output of this program will be

3
2
1

This was the task assigned to me.  Following is my code.

using System;

namespace Exercise
{
    class Program
    {
        static void Main()
        {
            var stack = new Stack();

            stack.Push(1);
            stack.Push(2);
            stack.Push(3);

            Console.WriteLine(stack.Pop());
            Console.WriteLine(stack.Pop());
            Console.WriteLine(stack.Pop());
        }
    }
}

using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;

namespace Exercise
{
    internal class Stack : IEnumerable
    {
        private object _object;
        private List<object> list = new List<object>();

        public IEnumerator GetEnumerator()
        {
            while (list.Any())
                yield return Pop();
        }

        internal object Pop()
        {
            if (list.Count == 0)
                throw new InvalidOperationException("Cannot use .Pop() if list count equals 0.");

            _object = list.FirstOrDefault();

            list.RemoveAt(0);

            return _object;
        }

        internal void Push(object obj)
        {
            _object = obj;

            if (_object == null)
                throw new InvalidOperationException("Cannot use .Push() if object is null.");

            list.Insert(0, _object);
        }

        internal void Clear()
        {
            if (list.Count == 0)
                throw new InvalidOperationException("Cannot use .Clear() if list is empty.");

            list.Clear();
        }

        public void Print()
        {
            if (list.Count == 0)
                throw new InvalidOperationException("Stack is empty.");

            foreach (var s in list)
            {
                Console.WriteLine(s);
            }
        }
    }
}

I'm a beginner in C#.  Is there anything left to improve on with this that is within the boundaries of what the instructions expect?  Any advice or suggestions if anything to teach me something new would be great.  Thank you for all the help so far on this site.

Questions:

Why do you use the class field _object? Does it serve any real purpose that a method-local variable cannot?
Why do you implement IEnumerable?
(only consider this if you have a decent understanding of concurrency issues, and want to practice) How can you make this code thread-safe?

Suggestions:

Several method throw exceptions in unexceptionable scenarios. Why do you blow up if I want to print or clear an empty stack? That's like having the waiter shoot herself if I try to order food right as the kitchen has closed.
When you're done with the exercise, consider making your stack generic. Casting from object is so old-school C#.
Consider improving your error messages. For one, "Cannot use .Pop() if list count equals 0." could be something like: "Cannot pop an element off an empty stack".


Search dictionary for matching values I have a dictionary self.indexList = {} filled with 3-item-lists values = [lineId, nominalLineId, vertex] for each dictionary key (its a supplement to a QgsSpatialIndex in PyQGIS, btw).

I need to reverse-search this dictionary for matching tuples of lineId and vertex. This function is the dealbreaker in a larger PyQGIS algorithm; called multiple times (almost 40.000 with a large test dataset) and causing 2/3 of total running time.

def get_idx_from_values(instance, lineID, vertex):
    """Searches the indexList for a lineID and a vertex number (= unique line segment).
    Returns dic index (=fid)."""
    items = instance.indexList.items()
    for idx, values in items:
        if values[0] == lineID and values[2] == vertex:
            return idx
    print("No index entry found for line " + str(lineID) + ", Vertex " + str(vertex))
    return None

I already found Search in a big dictionary Python where a second dictionary with a reversion of key and value is suggested.

I consider this, but I'm open for other suggestions how to speed up the code above, as my case - matching two values with a certain key - adds complexity to the simple key/value reversion.

Tuples can be used as dictionary keys, so creating a second dictionary for reverse lookups can be as simple as:

reverse_lookup = {(lineID, vertex): idx
                  for idx, (lineId, _, vertex)
                  in instance.indexList.items()}


Chat bot for posting recent answers I am the half-robot side of syb0rg that will be posting the recent answers of Code Review to the CR Answers chatroom.  Here is the list of review suggestions I would like, in order of preference:

Efficiency (with API requests, speed of login and posting answers, etc.)
Security issues
Best practices

For feature requests regarding the chat bot, please see this meta post.

Any and all reviews are acceptable however.  Don't be too harsh please, this is one of my first times using Ruby.

ACCESS_TOKEN = '<insert key>'
# get your access token here:
# https://stackexchange.com/oauth/dialog?client_id=2666&redirect_uri=http://keyboardfire.com/chatdump.html&scope=no_expiry
$root = 'http://stackexchange.com'
$chatroot = 'http://chat.stackexchange.com'
$room_number = 12723
site = 'codereview'
email = '<insert email>'
password = '<insert password>'

require 'rubygems'
require 'mechanize'
require 'json'
require 'net/http'

loop
{
    begin

    $agent = Mechanize.new
    $agent.agent.http.verify_mode = OpenSSL::SSL::VERIFY_NONE

    login_form = $agent.get('https://openid.stackexchange.com/account/login').forms.first
    login_form.email = email
    login_form.password = password
    $agent.submit login_form, login_form.buttons.first
    puts 'logged in with SE openid'

    meta_login_form = $agent.get($root + '/users/login').forms.last
    meta_login_form.openid_identifier = 'https://openid.stackexchange.com/'
    $agent.submit meta_login_form, meta_login_form.buttons.last
    puts 'logged in to root'

    chat_login_form = $agent.get('http://stackexchange.com/users/chat-login').forms.last
    $agent.submit chat_login_form, chat_login_form.buttons.last
    puts 'logged in to chat'

    $fkey = $agent.get($chatroot + '/chats/join/favorite').forms.last.fkey
    puts 'found fkey'

    def send_message text
        loop
        {
            begin
            resp = $agent.post("#{$chatroot}/chats/#{$room_number}/messages/new", [['text', text], ['fkey', $fkey]]).body
            success = JSON.parse(resp)['id'] != nil
            return if success
            rescue Mechanize::ResponseCodeError => e
            puts "Error: #{e.inspect}"
            end
            puts 'sleeping'
            sleep 3
        }
    end

    puts $ERR ? "An unknown error occurred. Bot restarted." : "Bot initialized."

    last_date = 0
    loop
    {
        uri = URI.parse "https://api.stackexchange.com/2.2/events?pagesize=100&since=#{last_date}&site=#{site}&filter=!9WgJfejF6&key=thqRkHjZhayoReI9ARAODA((&access_token=#{ACCESS_TOKEN}"
        http = Net::HTTP.new(uri.host, uri.port)
        http.use_ssl = true
        http.verify_mode = OpenSSL::SSL::VERIFY_NONE
        data = JSON.parse http.get(uri.request_uri).body
        events = data['items']

        data['items'].each do |event|
            last_date = [last_date, event['creation_date'].to_i + 1].max
            if ['answer_posted'].include? event['event_type']
                send_message "[tag:rob0t] New answer detected:"
                send_message event['link']
                puts "Answer posted."
            end
        end
        puts "#{data['quota_remaining']}/#{data['quota_max']} quota remaining"
        sleep(40 + (data['backoff'] || 0).to_i) # add backoff time if any, just in case
    }

    rescue => e
        $ERR = e
        p e
    end
}

(Attribution to original author, code above is a modified version of it.)

For starters, indent your code consistently — the standard in Ruby is two spaces.  That includes indenting the contents of your begin-rescue-end blocks.

Normally, I don't like to make such a huge fuss about indentation, but in this case I think it's highly important, because:

Your program has a highly unusual outline (infinite loops and a function definition(!) inside an infinite loop)
The stakes are high: if you misbehave, you could make a lot of people upset.  Therefore, good software engineering practices should be used.

An outline like this would be more idiomatic for Ruby:

class AnswerBot
  ROOT = 'http://stackexchange.com'
  CHAT_ROOT = 'http://chat.stackexchange.com'

  def initialize(options)
    @agent = Mechanize.new
    @options = options
  end

  def login
    # Do stuff with @agent
    login_form = $agent.get('https://openid.stackexchange.com/account/login').forms.first
    login_form.email = @options[:email]
    # ...
    @fkey = @agent.get(CHAT_ROOT + '/chats/join/favorite').forms.last.fkey
  end

  def fetch_answers
    # Make request to api.stackexchange.com
    # ...
    data['items'].each { |event| yield event }
    return (data['backoff'] || 0).to_i
  end

  def send_message(text, retries=5, backoff=40)
    # ...
  end
end

bot = AnswerBot.new(:access_token => ...,
                    :room_number = 12723,
                    :site => 'codereview',
                    :email => ...,
                    :password => ...)
loop {
  begin
    bot.login

    do
      backoff = bot.fetch_answers do |event|
        if ['answer_posted'].include?(event['event_type']) # <-- Is that right?
          bot.send_message(...)
        end
      end
    while sleep(40 + backoff)
  rescue => e
    puts "An error occurred."
    p e
  end
  puts "Bot restarted."
}


Trim trailing lines and remove blank lines in C This simple program, that I call trim(1), trims trailing lines and, if the flag -l is passed as an argument, it also removes empty lines and lines containing only blanks.

I did this as an exercise for learning C and good practices of C programming in UNIX.

It reads from standard input if no argument is passed or read from the files passed as arguments.

I am a bit suspicious of that realloc idiom for reallocing the buffer if it gets full. Is it right?

#include <err.h>
#include <errno.h>
#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

#define MID 0   /* in middle of line */
#define BEG 1   /* at beginning of line */

static int blankline = 0;
static int exitval = EXIT_SUCCESS;

static void trim(FILE *fp);
static void usage(void);

/* remove trailing blanks and delete blank lines */
int
main(int argc, char *argv[])
{
    int ch;
    FILE *fp;

    while ((ch = getopt(argc, argv, "l")) != -1) {
        switch (ch) {
        case 'l':
            blankline = 1;
            break;
        default:
            usage();
            break;
        }
    }
    argc -= optind;
    argv += optind;

    if (argc == 0) {
        trim(stdin);
    } else {
        while (*argv) {
            if ((fp = fopen(*argv, "r")) == NULL) {
                warn("%s", *argv);
                exitval = EXIT_FAILURE;
            } else {
                trim(fp);
                fclose(fp);
            }
            argv++;
        }
    }
}

/* trim trailing blanks from fp; if blankline is set also remove blank lines */
static void
trim(FILE *fp)
{
    char *buf = NULL;
    size_t i, size, newsize;
    int status;
    int c;

    size = BUFSIZ;

    if (buf == NULL)
        if ((buf = malloc(size)) == NULL)
            err(EXIT_FAILURE, "malloc");

    i = 0;
    status = BEG;
    while ((c = getc(fp)) != EOF) {

        if (isblank(c)) {               /* collect blanks in buf */
            if (i >= size) {            /* realloc */
                newsize = size + BUFSIZ;
                if (newsize <= size)    /* check for overflow */
                    errc(EXIT_FAILURE, EOVERFLOW, "realloc");
                size = newsize;
                if ((buf = realloc(buf, size)) == NULL)
                    err(EXIT_FAILURE, "realloc");
            }
            buf[i++] = c;
        } else if (c == '\n') {
            if (status == MID || (!blankline))
                putchar('\n');
            status = BEG;
            i = 0;
        } else {
            fwrite(buf, 1, i, stdout);
            putchar(c);
            status = MID;
            i = 0;
        }
    }

    if (ferror(fp)) {
        warn("getc");
        exitval = EXIT_FAILURE;
    }
}

/* show usage */
static void
usage(void)
{
    (void)fprintf(stderr, "usage: trim [-l] [file...]\n");
    exit(EXIT_FAILURE);
}

Well designed code.

  I am a bit suspicious of that realloc idiom for reallocing the buffer if it gets full. Is it right?

I see no errors.

Nit: I'd rather see newsize declared locally.  It is only used there.

// newsize = size + BUFSIZ;
size_t newsize = size + BUFSIZ;

Other

Unneeded code

The initial allocation is not needed.  realloc() in the loop can handle starting from 0

//size = BUFSIZ;
//if (buf == NULL)
//    if ((buf = malloc(size)) == NULL)
//      err(EXIT_FAILURE, "malloc");
size = 0;

Perhaps there is a concern about ...

fwrite(NULL, 1, 0, stdout);

... yet given the spec "If size or nmemb is zero, fwrite returns zero and the state of the stream remains unchanged.", I see no issue.

Either way I'd consider the micro-optimization below, just for the sake of avoiding the fwite() function call as i==0 will be quite common.  (Usually I do not encourage any micro-op - I'll make an exception here)

// fwrite(buf, 1, i, stdout);
if (i > 0) fwrite(buf, 1, i, stdout);

Checking output

Code does not check the return values of fwrite(), putchar(c).  Robust code would and report the problem - even if it is rare.

Missing free()

Free the allocation, else trim() leaks memory.

free(buf);

ferror()

Good that code checks this.

main() always reports success

main() employs the implied return 0;.  Yet failure is possible.

Given code has exitval = EXIT_FAILURE;, I'd expect main() to return exitval; in the end.

Initialize

Rather than declare then assign lines later, consider declare with initialization.

// int status;
// ...
// status = BEG;
int status = BEG;

// size_t size;
// ...
// size = BUFSIZ;
size_t size = BUFSIZ;

Same for i.


Recursive function application I use a lot T4 templates and want to avoid extra code writing when it could be generated. Currently I have following class:

class Recursion
{
    private readonly string _function;
    private readonly Func<int, string> _appFunc;

    public Recursion(string function, Func<int, string> appFunc)
    {
        _function = function;
        _appFunc = appFunc;
    }

    public string Apply(int n)
    {
        return Apply(0, n);
    }

    private string Apply(int k, int n)
    {
        var diff = n - k;
        if (diff == 0)
            return "0";
        if (diff == 1)
            return _appFunc(k);
        if (diff == 2)
            return string.Format("{0}({1}, {2})", _function, _appFunc(k), _appFunc(k + 1));
        int half = (n + k) / 2;
        return string.Format("{0}({1}, {2})", _function, Apply(k, half), Apply(half, n));
    }
}

Example of usage:

var rec = new Recursion("Math.Max", i => string.Format("source[{0}]", i));
Console.WriteLine(rec.Apply(8));

output:

  Math.Max(Math.Max(Math.Max(source[0], source[1]), Math.Max(source[2],
  source[3])), Math.Max(Math.Max(source[4], source[5]),
  Math.Max(source[6], source[7])))

It works fine but for large files generation takes a lot of time (even when N is small). I can't use it for larger N (now I'm using N = 4 and generation takes 5+ seconds, i expect that for 8 and more it would take hours).

I want to get this output, but with smaller cost, if possible, possibly with a different algorithm.

Consider using the string concatenation operator + instead. The reason why this works and is not bad is because behind the scenes the method String.Concat is being called.

While String.Format will have to do a lot of different things, such as parsing, getting the string after and before placeholders to be able to join them and so on.

This is important both on the callback and implementation.

private string Apply(int k, int n)
{
    var diff = n - k;
    if (diff == 0)
        return "0";
    if (diff == 1)
        return _appFunc(k);
    if (diff == 2)
        return _function + "(" + _appFunc(k) + ", " + _appFunc(k + 1) + ")";
    int half = (n + k) / 2;
    return _function + "(" + ApplyB(k, half) + ", " + ApplyB(half, n) + ")";
}

var rec = new Recursion("Math.Max", i => "source[" + i + "]");

By applying this changes I could measure a 0.5s difference on my machine over 60000 iterations calling apply with 8 which previously took 3s so it was a 17% improvement.

Going with StringBuilder

I was a bit reluctant going with a StringBuilder because it seemed an hard job but when we think two more times about it solutions come. My idea was to put a StringBuilder on a field, so I don't have to pass it has a parameter to the function, and make the method return void.

The StringBuilder is the ideal class to build large strings, it is efficient because it manages an array of characters. It's like having mutable strings.

    class Recursion
    {
        private readonly string _function;
        private readonly Func<int, string> _appFunc;
        private readonly StringBuilder _builder;
        public Recursion(string function, Func<int, string> appFunc)
        {
            _function = function;
            _appFunc = appFunc;
            _builder = new StringBuilder();
        }
        public string Apply(int n)
        {
            Apply(0, n);
            var result = _builder.ToString();
            _builder.Clear();
            return result;
        }

        private void Apply(int k, int n)
        {
            var diff = n - k;
            if (diff == 0)
            {
                _builder.Append("0");
                return;
            }
            if (diff == 1)
            {
                _builder.Append(_appFunc(k));
                return;
            }
            if (diff == 2)
            {
                _builder.Append(_function)
                .Append('(')
                .Append(_appFunc(k))
                .Append(", ")
                .Append(_appFunc(k + 1))
                .Append(')');
                return;
            }
            int half = (n + k) / 2;
            _builder.Append(_function).Append('(');
            Apply(k, half);
            _builder.Append(", ");
            Apply(half, n);
            _builder.Append(')');
        }
    }

This decreased execution 0.9s upon my previous attempt, on the same circumstances, giving it a total of 46% improvement.


Finding the type of number entered by the user I am making a fractions calculator that can calculate fractions, whole numbers and mixed fractions.
So there is a String fraction, which is the input.
Also, the user will enter the fraction in a specified format which is:

For normal fraction, "a/b" (a and b are integers)
For whole numbers, "c" (c is an integer)
For mixed fraction, "c a/b" (c is integer, a/b is the fraction separated by a space)

I want the program to check and find out which type of fraction the user has entered and then proceed. I'm using If-Else If-Else for this.
fun fractionalize(fraction: String): List<Int>{
        //Finds type of number (input) and converts to List<Int> with [numerator, denominator, whole number]
        var result: List<Int> = listOf(0,1,0)
        var numerator: Int = 0
        var denominator:Int = 1
        var whole_number:Int = 0
        try {
            if ((fraction.contains('/') && fraction.indexOf('/') == fraction.lastIndexOf('/')) && !fraction.contains(" ")) {
                //input is a regular fraction
                var fraction_string = fraction.split("/")
                numerator = fraction_string[0].toInt()
                denominator = fraction_string[1].toInt()
                result = listOf(numerator, denominator, 0)
            } else if (fraction.isDigitsOnly()) {
                //input is a whole number
                result = listOf(fraction.toInt(), 1, 0)
            } else if ((fraction.contains('/') && fraction.indexOf('/') == fraction.lastIndexOf('/')) && (fraction.contains(" ") && fraction.indexOf(' ') == fraction.lastIndexOf(' '))) {
                //input is mixed fraction
                var wholeNumber_split = fraction.split(" ")
                whole_number = wholeNumber_split[0].toInt()
                numerator = wholeNumber_split[1].split("/")[0].toInt()
                numerator += whole_number * denominator
                denominator = wholeNumber_split[1].split("/")[1].toInt()
                result = listOf(numerator, denominator, 0)
            }
            else {
                Toast.makeText(applicationContext,
                "The number you entered is not in a valid format.",
                Toast.LENGTH_LONG).show()
            }
            //simplifying input
            if (result[2] != 0){
                numerator += whole_number * denominator
                result = listOf(numerator, denominator, 0)
            }
            return result
        }
        catch (e: Exception){
            Toast.makeText(applicationContext,
                "The number you entered is not in a valid format.",
                Toast.LENGTH_LONG).show()
            return result
        }
    }

Is this the best way I can do that or is there cleaner and more efficient code I can make (personally I think this is too messy and can be made more efficient)?

Your code has several areas where it can be improved.
The first is that you should separate the parsing code from the platform code (Toast). Kotlin has a great testing framework, in which it is easy to write "parsing "1 2/3" should return listOf(1, 2, 3)". This testing code works best if you run it directly on your developing machine, and without any Toast or Activity.
The next thing is that you should not use List<Int> in places where you originally wanted to say Fraction or maybe, to be more specific, MixedFraction. The point is that listOf(1, 2, 3, 4, 5) is a valid list but not a valid fraction. By using a specifically constrained type you force the Kotlin compiler to check your code for you.
class Fraction(
    val int: Int,
    val num: Int,
    val den: Int
)

To make your parser extensible to expressions that are more complicated, you must not use contains. Instead, the best-practice way is to split the input into tokens first (often using regular expressions) and then to combine these tokens to expression trees (using a parser that only looks at the very next token to decide what to do next). The details of all of this are covered in any good book on compiler construction.
In your case, the tokens would be:

unsigned integer
fraction slash
unary minus sign

Note that there is no "space" token, as that is usually left out when defining an expression language.


Basic command-line calculator Its an Basic Calculator I worked on, and now I wanna know if I can make something better like not so "much" Code or I made somthing to complicated? Have a nice day! Keep safe!
 import java.util.Scanner;

public class learningCode {
    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);

        System.out.println("Bitte geben sie einen Rechenoperator für ihre Rechnung ein! (+, -, *, /)");

        String numberTest = scan.nextLine();

        if (numberTest.equals("+")) {
            String questionOne = "Bitte gebe deine erste Zahl ein: ";
            System.out.println(questionOne);
            double number1 = scan.nextDouble();
            String questionTwo = "Bitte gebe deine zweite Zahl ein: ";
            System.out.println(questionTwo);
            double number2 = scan.nextDouble();
            double finalResult = number1 + number2;

            String Answer = "Dein Ergebnis ist: ";

            System.out.println(Answer  + finalResult );

            scan.close();
        }
        else {

            if (!"+".equals(numberTest) && !"-".equals(numberTest) && !"*".equals(numberTest) && !"/".equals(numberTest)) {
                System.out.println("Das ist kein Rechenoperator. Bitte versuche es erneut!");
            }

            if (numberTest.equals("*")) {
                String questionTwo = "Bitte gebe deine erste Zahl ein: ";
                System.out.println(questionTwo);
                double number1 = scan.nextDouble();
                String questionThree = "Bitte gebe deine zweite Zahl ein: ";
                System.out.println(questionThree);
                double number2 = scan.nextDouble();
                double finalResult = number1 * number2;

                String Answer = "Dein Ergebnis ist: ";

                System.out.println(Answer  + finalResult );

                scan.close();
            }
            if (numberTest.equals("/")) {
                String questionThree = "Bitte gebe deine erste Zahl ein: ";
                System.out.println(questionThree);
                double number1 = scan.nextDouble();
                String questionFour = "Bitte gebe deine zweite Zahl ein: ";
                System.out.println(questionFour);
                double number2 = scan.nextDouble();
                double finalResult = number1 / number2;

                String Answer = "Dein Ergebnis ist: ";

                System.out.println(Answer  + finalResult );

                scan.close();
            }
            if (numberTest.equals("-")) {
                String questionFive = "Bitte gebe deine erste Zahl ein: ";
                System.out.println(questionFive);
                double number1 = scan.nextDouble();
                String questionSix = "Bitte gebe deine zweite Zahl ein: ";
                System.out.println(questionSix);
                double number2 = scan.nextDouble();
                double finalResult = number1 - number2;

                String Answer = "Dein Ergebnis ist: ";

                System.out.println(Answer  + finalResult );

                scan.close();
            }

        }

    }
}

Naming
Adhere to the Java Naming Conventions.

Names of methods and variables start with a lowercase letter, names of classes with an uppercase letter.

Method names should start with a verb so that they read like an instruction. Names of variables should start with a noun.

Exception to that are variables holding or methods returning a boolean which both should start with is, can, has or alike.

Don't surprise your readers
You have a variable named numberTest but it does not contain a test for a number but the current operation or the users choice.
Code Quality
Structure  your code with methods.
You wrote a single long method.
This method contains an if/else cascade that somehow separates the method in to different sections.
You should extract this blocks in to separate Methods (leaving the if/else instructions in main) so that the main method becomes shorter and easier to read (and to maintain).
Don't repeat yourself
All over your code you repeat this two lines with varying variable names:
            System.out.println(questionSix);
            double number2 = scan.nextDouble();

This two lines could be extracted to a separate method that takes the String as parameter and returns the double value. That would shrink four lines in your code to just one.


Node.js parallel file download, the ES6 way I wrote a script that downloads all PDFs found on the web page of a particular government agency. I would have chosen bash for such a task, but I want the script to run in Node.js. The downloaded files are a few hundred kilobytes big, if that matters.

It is the first time I use ES6, but I want to learn how to take advantage of all ES6 features, and I want to make my script very much in the spirit of ES6. In bash I would have downloaded the list of PDF files first, then proceeded to download them with indentation back to zero, but I got the impression that putting everything within the first request.get block is more in the ES6 spirit, correct me if I am wrong.

The script works, and validates with ESLint after disabling the following rules:

no-console because it is intended to be run on the console
no-undef because it tells me require is not defined
one-var because it would force me to mix require and other declaration thus triggering no-mixed-requires
sort-vars because I want to order constants in a more sensible way
strict because it would not accept "use strict"; as the first line

My code:

"use strict";

// Imports
const async = require("async"),
    fs = require("fs"),
    http = require("http"),
    request = require("request");

// Settings
const INDEX_URL = "http://www.kokuminhogo.go.jp/hinan/index.html",
    MAX_PARALLEL_DOWNLOADS = 3,
    PREFECTURES_REGEX = /area.*hinan_(.*)\.pdf/g,
    REMOTE_PREFIX = "http://www.kokuminhogo.go.jp/pdf/hinan_",
    LOCAL_PREFIX = "data/file",
    FORMAT_EXTENSION = ".pdf",
    REGEX_MATCH_INDEX = 1,
    HTTP_STATUS_OK = 200;

// ///////////////////////////////////////////////////////////
// Utility to download a file, with callback
// ///////////////////////////////////////////////////////////
const download = (url, dest, next) => {

    console.log(`Downloading ${url} to ${dest}`);
    const file = fs.createWriteStream(dest);

    http.get(url, (response) => {

        response.pipe(file);
        file.on("finish", () => {

            file.close();
            next();

        });

    });

};

// ///////////////////////////////////////////////////////////
// Get list of available prefectures
// ///////////////////////////////////////////////////////////
request.get(INDEX_URL, (error, response, body) => {

    const prefectures = [];

    if (!error && response.statusCode === HTTP_STATUS_OK) {

        let match = PREFECTURES_REGEX.exec(body);

        while (match !== null) {

            prefectures.push(match[REGEX_MATCH_INDEX]);
            match = PREFECTURES_REGEX.exec(body);

        }

    }

    console.log(`Number of prefectures available: ${prefectures.length}`);

// ///////////////////////////////////////////////////////////
// Download the PDF files for all prefectures
// ///////////////////////////////////////////////////////////
    // Files will be numbered 1, 2, etc
    let fileNumber = 1;

    // Process in parallel
    async.eachLimit(
        prefectures,
        MAX_PARALLEL_DOWNLOADS,
        (prefecture, next) => {

            download(
                REMOTE_PREFIX + prefecture + FORMAT_EXTENSION,
                LOCAL_PREFIX + fileNumber + FORMAT_EXTENSION,
                next
            );
            fileNumber += 1;

        }

        ,
        () => {

            console.log("Finished downloading");

        }
    );

});

Things that come to my mind after a quick read:

do not use const someFunction = (...) => {...} instead of function someFunction(...) {...})
use module.exports even if it is a script
group functionality into functions; you have comments beautifully spaced, but a function name will be more readable and maintainable
try to have functions that do one thing; when explaining to someone what a function does, and you have to use and then you should split the function
try to use map, filter, etc. instead of having iterations with a variable (e.g. fileNumber)
whenever you have someString + someOther you could use string templating (i.e. ${someString}${someOther}
prefer using promises instead of callbacks; the code ends up being more readable; e.g. use bluebird.promisify
use a good eslint config (e.g. airbnb)

To enhance collaboration, you could use a github repository and reference it here.


Sorting software version list using Python 2.7 (no modules) I am participating in a challenge and one of the challenges was to write a script to sort a list of software versions (ex: 1.0, 1.3.2, 12, 1.3.0). Here is the my solution:

def answer(l):
     return sorted(l, key=lambda s: [int(i) for i in s.split('.')])
print(answer(eval(raw_input())))

In this code, I am essentially taking the list, and splitting each element by '.', converting them to integers, and then sorting the list. One thought is to use list comprehension to remove lambda altogether, but unsure how to achieve it.

The best way would be to use the distutils.StrictVersion as your sort key argument.

However, as you do not want to use imports, you may choose to copy the _cmp, __eq__, __le__ and other comparison methods as applicable, from the source file linked above. You'd also modify the parse method to use str.split instead of a regex matching.

I do not think you'd have to worry about pre-releases etc.

Why I'd go with distutils is because it has been tested since a long time. Writing your own version would be prone to errors, and edge cases.


Assigning jobs in a functional style I'm new to F# and functional programming in general (coming from an object oriented background). I just wanted to get some criticism on an algorithm for assigning jobs to crew members. I want to be as functional as possible and avoid loops and mutable state. Here is what I've come up with.

let rec AssignJobs crewMembers jobs jobAssignments =
    let AssignJob job crewMembers =
        let bestCrewMember = crewMembers |> Seq.sortWith crewMemberComparison
                                         |> Seq.head
        {CrewMember = bestCrewMember; Job = job}

    match jobs with
    | job :: remainingJobs ->
        let jobAssignment = AssignJob job crewMembers
        let remainingCrewMembers = crewMembers |> Seq.filter ((<>)jobAssignment.CrewMember)
        AssignJobs remainingCrewMembers remainingJobs (jobAssignments @ [jobAssignment])
    | [] -> jobAssignments

Assume that crewMemberComparison is working for this review. I'm more concerned about whether I have a good approach to managing the crewMembers, jobs and jobAssignments sequences.

I don't like how I have to invoke this with an empty list i.e.

let assignedJobs = AssignJobs crewMembers jobs []

Is there another way? Is recursion the best solution at all (presuming I'm not allowed to loop)?

Thanks in advance!

I don't like how I have to invoke this with an empty list i.e.
let assignedJobs = AssignJobs crewMembers jobs []

Is there another way? Is recursion the best solution at all (presuming I'm not allowed to loop)?

The 'empty list' initiation is pretty standard in F#. The nice thing about it is if you load a list of currently assigned jobs from a data-source (database, API, etc.) you can seed the function with the current assignments.
Recursion is perfectly fine here, and in fact it's the route I would go to solve this issue. That said, I would actually consider an unfold as well, which would remove the empty-list requirement.
Basically, unfold takes some seed data (here it would be the crewMembers and jobs lists) and it creates a sequence/list/array from that seed data. (It's the opposite of fold: whereas fold creates a single result from a list, even if that's another list, unfold takes a single result and creates the original list.)
With regard to your current method, I have minimal comments. This is quite idiomatic F#, and follows most of the standard themes we use.

let AssignJob job crewMembers =
    let bestCrewMember = crewMembers |> Seq.sortWith crewMemberComparison
                                     |> Seq.head
    {CrewMember = bestCrewMember; Job = job}

I'm not a big fan of your formatting, but beyond that I see no issues here. Personally, I would break it down as follows:
let AssignJob job crewMembers =
    let bestCrewMember =
        crewMembers
        |> Seq.sortWith crewMemberComparison
        |> Seq.head
    { CrewMember = bestCrewMember; Job = job }

(I don't like leaving unnecessary whitespace on the left — makes it harder to find the code you need to see.)
Also, I would name that assignJob as is normal F# parlance for function names.
Lastly, you could take advantage of Seq.exept instead of Seq.filter ((<>) ...), but you need to instantiate an extra list or array to do that, so I'm not sure if it's worth it.


Call method of specific parent class This question is related to this topic.

Context:

class OperationDevice(Device):
    def __init__(self):
        super(OperationDevice, self).__init__()

    def operation_start(self):
        # Do operation specific code
        self.start()

    def operation_stop(self):
        # Do operation specific code
        self.stop()

class SimulationDevice(Device):
    def __init__(self):
        super(SimulationDevice, self).__init__()

    def simulation_start(self):
        # Do simulation specific code
        self.fsm.start()

    def simulation_stop(self):
        # Do simulation specific code
        self.fsm.stop()

class DualModeDevice(OperationDevice, SimulationDevice):
    def __init__(self, mode='simulation'):
        super(DualModeDevice, self).__init__()
        self._mode = mode
        self._start_map = {
            'simulation': self.simulation_start,
            'operation': self.operation_start
        }
        self._stop_map = {
            'simulation': self.simulation_stop,
            'operation': self.operation_stop
        }

    def start(self):
        self._start_map[self._mode]()

    def stop(self):
        self._stop_map[self._mode]()

Here I have to define in OperationDevice and SimulationDevice different method name like simulation_start and operation_start (because of MRO).

I actually want to define the same method name for both class and be able to call each one from DualModeDevice or subclasses.
For example operation_start from OperationDevice and simulation_start from SimulationDevice will become start. Is it possible and how?
This solution is a way to switch between class OperationDevice and SimulationDevice following value of mode. I am wondering wether is it possible to define automatically all method from SimulationDevice linked with mode = "simulation" (same for OperationDevice with "operation") without having to specify for each method (self._start_map for example) in constructor (init) of DualModeDevice.

You could use composition instead of inheritance:

class OperationDevice(Device):
    def __init__(self):
        super(OperationDevice, self).__init__()

    def start(self):
        # Do operation specific code

    def stop(self):
        # Do operation specific code

class SimulationDevice(Device):
    def __init__(self):
        super(SimulationDevice, self).__init__()

    def start(self):
        # Do simulation specific code
        self.fsm.start()

    def stop(self):
        # Do simulation specific code
        self.fsm.stop()

class DualModeDevice(Device):
    def __init__(self, mode='simulation'):
        super(DualModeDevice, self).__init__()
        self._mode = mode
        self._mode_map = {
            'simulation': SimulationDevice(),
            'operation': OperationDevice()
        }

    def start(self):
        self._mode_map[self._mode].start()

    def stop(self):
        self._mode_map[self._mode].stop()


Manually parsing HTTP response header Due to some constraints I had to manually parse an HTTP response coming out of a stream.

I could not use buffered reader or scanner because the perform buffering and end up buffering bytes beyond the response header itself.

I'll be hitting my AWS S3 and only expect 200 or 206 status response.

This seems to work, but I'd appreciate some review.

final InputStream reader = socket.getInputStream();
stringBuilder.setLength(0);
//first fetch header
while (true) {

    final char read = (char) reader.read();
    if (read < 1) {
        //fail
        MiscUtils.closeAndIgnore(socket, reader);
        return false;
    }

    stringBuilder.append(read);
    if (read == '\n')
        break; //status header fetched
}

//check status
final String status = stringBuilder.toString();
Log.i("Downloader", status);
if (!(status.contains("200") || status.contains("206"))) {
    //fail
    MiscUtils.closeAndIgnore(socket, reader);
    return false;
}

//now consume the header
stringBuilder.setLength(0);
while (true) {

    final char read = (char) reader.read();
    if (read < 1) {
        MiscUtils.closeAndIgnore(socket, reader);
        return false;
    }

    stringBuilder.append(read);
    final int currentCount = stringBuilder.length();
    if (currentCount > 4 &&
            stringBuilder.charAt(currentCount - 1) == '\n' &&
            stringBuilder.charAt(currentCount - 2) == '\r' &&
            stringBuilder.charAt(currentCount - 3) == '\n' &&
            stringBuilder.charAt(currentCount - 4) == '\r') {
        break; //response consumed
    }
}

//print the header
final String completeResponse = stringBuilder.toString();
Log.i("Downloader", completeResponse);

This is a bit surprising:

stringBuilder.setLength(0);

Seeing the posted code starting with this is fishy,
as it suggests that there is more code before it,
and that the method is not decomposed enough,
and should be multiple methods instead.

This is a bit surprising:

    final char read = (char) reader.read();
    if (read < 1) {

The read() method returns -1 in case of EOF.
I suggest to change this check to read < 0 or read == EOF to avoid making reviewers ponder.

        MiscUtils.closeAndIgnore(socket, reader);

A common name for this kind of thing is closeQuietly,
usually in a utility class called IOUtils (in Apache commons, for example).
I suggest to follow that good example.
MiscUtils is a bad idea.

    if (read == '\n')
        break; //status header fetched

It's recommended to use braces always, even with single-statement expressions like this.

if (!(status.contains("200") || status.contains("206"))) {

I'd go for the more strict and compact regular expression check:

if (!status.matches("HTTP/.* 20[06] .*")) {

It would be good to split the posted code to two methods:

A method that consumes the status line (the first line)
A method that consumes the remaining lines


Does setting the same element in a list does something at all? I have a code similar to this:

public class PlayerRound {
    private final List<Strip> playerStrips = new ArrayList<>();

    public boolean addStrip(final Strip aStrip) {
        // ...
        if (playerStrips.contains(aStrip)) {
            playerStrips.set(playerStrips.indexOf(aStrip), aStrip); // DOES THIS LINE CHANGE SOMETHING?
        }
        // ...
        return true;
    }
}

@Entity
@Cacheable(false)
@Table(
        appliesTo = "Strip",
        indexes = {
            @Index(name = "IDX_RoundStrip", columnNames = {"round_id"}),
            @Index(name = "IDX_UserStrip", columnNames = {"user_id"})
        }
)
public class Strip implements Serializable, IAnnotatedProxy {

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Integer id;

    @Transient
    private Integer tempId = tempIdx++;

    @Override
    public boolean equals(Object obj) {
        Boolean areEqual = Boolean.FALSE;
        if (obj != null && getClass() == obj.getClass()) {
            final Strip other = (Strip) obj;
            if (null == this.id && null == other.id) {
                if (this.tempId.equals(other.tempId)) {
                    areEqual = Boolean.TRUE;
                }
            } else if (null != this.id) {
                if (this.id.equals(other.id)
                        || this.id.equals(other.tempId)) {
                    areEqual = Boolean.TRUE;
                }
            } else if (null != other.id) {
                if (other.id.equals(this.id)
                        || other.id.equals(this.tempId)) {
                    areEqual = Boolean.TRUE;
                }
            }
        }
        return areEqual;
    }
}

My question is: can I safely delete the line that sets the element that already is on the list on the same position?

Does this line does something that I don't know?

No, that code is totally useless. Unless....

Same element twice

As it is a List, what happens if it contains the element twice?

Let's say we have a List containing two elements of the type Strip:

stripA, stripB

stripA and stripB are equal, that is, the class have implemented .equals. Then and only then, this code will do something useful. If aStrip is stripB then it will modify the list to become:

stripB, stripB

One inside, one outside, both .equals

The same is true if the List contains stripA only and you call the method with aStrip again as stripB. Remember that stripA.equals(stripB) is true so here's what happens:

 if (playerStrips.contains(aStrip)) {

Yes. .contains on a Collection uses .equals to see if it contains or not, so this will return true.

 playerStrips.indexOf(aStrip)

indexOf also uses .equals and the index of stripA is returned.

playerStrips.set(playerStrips.indexOf(aStrip), aStrip);

So what happens here is that the index of stripA is set to stripB.

Small Improvement

There's really no reason to use both indexOf and contains. You could do this instead:

int index = playerStrips.indexOf(aStrip);
if (index != -1) {
    playerStrips.set(index, aStrip);
}

However

If this functionality has been added because of this reason, I personally think it is a bad reason to add it. This code is more confusing than anything else. Do it differently. Either aStrip should not really be equal to bStrip, or something else should be modified. The reason for why one would want to replace aStrip with bStrip when they are already equal goes beyond my understanding.

Strip.equals

Holy.... mess!

First of, why use Boolean when you can use boolean? Secondly, why use a boolean at all when you can use return?

Also, you could probably use instanceof instead of getClass. But you should look at the StackOverflow question about this.

I have to question the usage of tempID, it does not feel like a clean solution to me - whatever problem it is meant to solve.

By following the program execution, you can see that if one of those if (condition) return true is not true that the only possible return value is false. Therefore, it can be simplified with return condition;

Because of the above, there will be no reason to use any else as there's a return inside the previous if.

Therefore, your .equals method can be simplified to:

@Override
public boolean equals(Object obj) {
    if (obj instanceof Strip) {
        final Strip other = (Strip) obj;
        if (null == this.id && null == other.id) {
            return this.tempId.equals(other.tempId);
        }
        if (null != this.id) {
            return this.id.equals(other.id)
                    || this.id.equals(other.tempId);
        }
        if (null != other.id) {
            return other.id.equals(this.id)
                    || other.id.equals(this.tempId);
        }
    }
    return false;
}


Zork look-a-like written in TypeScript I've been working on a a text based game with a very simple battling system. There are way to many questions I can ask to get reviewed about this project, but I'll take small steps at a time (this is my first post).
I'm mostly concerned about how I handle command input and the execution of said commands within the game. Just to give a simple example of what a command might look like:
go north

go - command (called action in the code, as Command is the whole class)
north (and anything after that) - arguments, you most likely know about these

INPUT
Currently, I've made it work using the readline package from Node. The input is being sent to a "commander" where it is parsed, made into a Command which will in the end be passed to an "executor" which will execute the command. Here's the code:
//Player.ts
import readline from 'readline';
//other imports...

class Player extends Entity {
    // fields, constructor and other methods...

    async getInput(questionPrompt?: string): Promise<string> {
        return new Promise((resolve, _) => {
            const prompt = readline.createInterface({
                input: process.stdin,
                output: process.stdout,
            });

            const question = (questionPrompt) ? questionPrompt : ">";

            prompt.question(question, (answer) => {
                prompt.close()

                resolve(answer);
            })
        })
    }
}

export default Player;

//commander.ts (technically it's inside commander/index.ts)
import Command from '../models/Command';
import Player from '../models/Player';

const actions = ['go', 'look', 'take', 'equip', 'inventory', 'help'];

export const parse = (text: string): (Command | null) => {
    const action = text.split(' ')[0];
    const args = text.split(' ').slice(1);

    if (!actions.find((a) => action === a))
        return null;

    return new Command(action, args);
}

export const execute = (command: Command, receiver: Player) => {
    switch (command.action) {
        case 'go':
            const success = receiver.move(command.args[0]);
            if (success) {
                return console.log(`You have entered a new room.\n${receiver.currentRoom.getDescription()}`);
            } else {
                return console.log(`You can't go there. Please try another one.`);
            }
        case 'look':
            const text = `You are in ${receiver.currentRoom.name}.\n` +
                `${receiver.currentRoom.description}`;
            return console.log(text);
        case 'take':
            const item = receiver.currentRoom.takeItem(command.args[0]);
            if (item)
                return receiver.takeItem(item);
            else
                return console.log(`A ${command.args[0]} couldn't be found.`);
        case 'equip':
            return receiver.inventory.map((i) => {
                if (i.name === command.args[0]) {
                    receiver.equip(i.id);
                }
            })
        case 'inventory':
            return console.log(receiver.inventory);
        case 'help':
            return console.log("No help for now... Maybe later?");
        default:
            console.log('Command error.');
            return console.log(command);
    }
}

export default {
    parse,
    execute,
}

I'm not entirely sure if this is an appropriate way to handle this use case, but this is all I could come up with. The other solution I managed to think of was to have the different commands in a separate folder where I could have them as modules and I could try to import them. This way they can maybe be classes and have their own help messages which will also make it easier to document the commands. The reason I decided to stick with this approach is because it seems a bit easier, and having the commands as separate modules kinds seems overkill to me, maybe I'm totally wrong.
I know I've probably missed something very important, so a link to the whole project can be found here.

From my experiences in MUDs and text games, I think you would do well to create a class/module that handles each command then inject them into your processor (even manually). It will help a lot with the maintainability but also isolating commands.

import {registerLook} from "./commands/look";
import {registerGo} from "./commands/go";

// Build up a hash of commands. With a MUD style, you would register the shorten
// versions also (so look registers as "l", "lo", "loo", and "look") but removes
// entries that would have a conflict (so "left" would remove the "l" for look but
// register "le", "lef", and "left".
let commands = {};

registerLook(commands);
registerGo(commands);

// Then when you are running the command.
if (command.action in commands
{
  commands[command.action].run(command, receiver);
}
else
{
  // Show error of missing command
}

You could go further with the SOLID implementation by having the commands injected from the top-level, but the main reason for doing this is to let you have the complex logic for the commands moved into a separate file that does one thing only (Single Responsibility Principle) while treating the switch of the commands against an interface (functions inserted into the hash by a register).

Ideally, having a DI figure out the commands would be nice, but I haven't found a good DI system for Typescript I like. (You could also scan the commands directory and inject the commands that way.)

It also lets you nest the commands if you need to. So if you have look at and look into, you could have commands/look.ts break apart the first word and then pass it into look commands (commands/look/at.ts) to further isolate it. Or whatever makes sense.


jQuery fadeToggle/fadeout function I have the following code to open and close some popup divs. I'd like to know whether there is a shorter solution to close the currently opened divs, as soon as another div is opened, so that there is only one open div.

The current solution works as you can see in this jsFiddle, but I guess it is a bit inconvenient.

jQuery:

$('.details').hide();
$('#item-a').click(function () {
$('#detail-1').fadeToggle(600);
 $('#detail-2,#detail-3').fadeOut("slow");
});

$('#item-b').click(function () {
$('#detail-2').fadeToggle(600);
 $('#detail-1,#detail-3').fadeOut("slow");
});

$('#item-c').click(function () {
$('#detail-3').fadeToggle(600);
 $('#detail-1,#detail-2').fadeOut("slow");
});

$('.btn-close').click(function () {
 $(this).parent().parent().fadeOut("slow");
});

HTML:

 item a

 item b

 item c

close

 Some detailed text 1..

close

 Some detailed text 2..

close

 Some detailed text 3..

You can use the not function to get all the details elements except one:

function show(sel) {
  var el = $(sel);
  el.fadeToggle(600);
  $('.details').not(el).fadeOut("slow");
}

$('.details').hide();

$('#item-a').click(function () {
  show('#detail-1');
});

$('#item-b').click(function () {
  show('#detail-2');
});

$('#item-c').click(function () {
  show('#detail-3');
});

$('.btn-close').click(function () {
  $(this).parent().parent().fadeOut("slow");
});


Uploading a file to the server I wrote a simple PHP script which allows the user to upload a file to the server. It works, but I'm not very sure about security. Could you give me some hints about security mistakes?

In a nutshell: The user uploads a file from form.html, then a file is saved in uploads/ directory on the server, and a direct link to it is recorded in a database. Then that link is presented to the user, so he/she is able to download the file. The execution of that file is forbidden using .htaccess.

form.html

                <form enctype="multipart/form-data" class="smallForm"
                      action="action.php?act=quickFileSubmit" method="post"
                      style="margin-top: 5px;">
                    File:
                    <input type="file" name="uploadfile" readonly="true" size="30"/>
                    <input type="submit" value="Submit" />
                </form>

action.php

//some actions

else if ($_REQUEST["act"] == "quickFileSubmit") {
    $basename = getUploadBasename();
    uploadFile();
    header("location:index.php");
}

function uploadFile() {

    if (strlen($_FILES['uploadfile']["name"]) > 0) {
        $target_path = "uploads/";
        $basename = basename( $_FILES['uploadfile']['name']);
        $target_path = $target_path . $basename;
        if (encode($basename) == $basename) {
            if(move_uploaded_file($_FILES['uploadfile']['tmp_name'], $target_path)) {

                // here goes saving of file info

                return $basename;
            } else{
                // upload failed.
                echo "File upload failed.";
                return null;
            }
        } else {
            echo "Illegal characters in file name.";
            return null;
        }
    }
}

Also, I have put .htaccess in the uploads dir:

.htaccess

SetHandler default-handler
Options -Indexes

One thing that immediately stands out to me is whether $_FILES['uploadfile'] actually exists for each upload request.  Do an isset on it first to make sure a client doesn't send a post with fabricated input values - for example: some custom html form of their own making.  If they do, you'll get 'function expects some value, boolean/null given' errors all over the place as is.

Or better yet, to allow client flexibility if they choose to use some sort of API for your website, just cycle through $_FILES array to work on whatever files they submit to this script.  Your choice though if you want to instead maintain strict input, but it wouldn't matter really as long as your script receives the file input of a field named "uploadfile" from any source.

Also, for $_FILES[$x]["name"], validate or sanitize it to ensure it contains only alphanumeric, underscore, dash, and dot characters.  Yes OS's can work with more characters than that, but not every protocol or application can (text, browsers or email editors breaking long filenames or links on spaces, etc).