Loop to extract json from dataframe and storing in a new dataframe I have a dataframe (obtained from a csv saved from mysql) with several columns, and one of them consist of a string which is the representation of a json. The data looks like:

id   email_id     provider  raw_data       ts
1    aa@gmail.com    A      {'a':'A',   2019-23-08 00:00:00
                             'b':'B',
                             'c':'C'}

And what my desired output is:

email_id      a   b   c
aa@gmail.com  A   B   C

What I have coded so far is the following:

import pandas as pd
import ast

df = pd.read_csv('data.csv')
df1 = pd.DataFrame()

for i in range(len(df)):
    dict_1 = ast.literal_eval(df['raw_content'][i])
    df1 = df1.append(pd.Series(dict_1),ignore_index=True)

pd.concat([df['email_id'],df1])

This works but it has a very big problem: it is extremely low (it takes hours for 100k rows). How could I make this operation faster?

Finally I got an amazing improvement thanks to stack overflow, regarding two things:
https://stackoverflow.com/questions/10715965/add-one-row-to-pandas-dataframe
https://stackoverflow.com/questions/37757844/pandas-df-locz-x-y-how-to-improve-speed

Also, as hpaulj pointed, changing to json.loads slightly increases the performance.

It went from 16 hours to 30 seconds

row_list = []

for i in range(len(df)):
    dict1 = {}
    dict1.update(json.loads(df.at[i,'raw_content']))
    row_list.append(dict1)

df1 = pd.DataFrame(row_list)

df2 = pd.concat([df['email_id'],df1],axis=1)


Classifying integers as "weird" or "not weird" Problem Statement

  Given an integer N, perform the following conditional actions:

  If N is odd, print Weird
  If N is even and in the inclusive range of 2 to 5, print Not Weird
  If N is even and in the inclusive range of 6 to 20, print Weird
  If N is even and greater than 20, print Not Weird

  Complete the stub code provided in your editor to print whether or not
  N is weird.

  Input Format: A single line containing a positive integer, N.

  Constraints: 1<=N<=100

  Output Format: Print Weird if the number is weird; otherwise, print
  Not Weird.

My Solution

if(N%2!=0)
    System.out.println("Weird");
else{
            if(N>=2&&N<=5||N>20)
            System.out.println("Not Weird");
            else
            System.out.println("Weird");
        }

Another Solution-

if(N%2==1)
            System.out.println("Weird");
        else
            {
            if(N>=2&&N<=5)
                System.out.println("Not Weird");
            else if(N<=20)
                System.out.println("Weird");
                else
                System.out.println("Not Weird");
        }

My question is, does one algorithm have better performance than the other, in other words, is there any advantage to using else if statements rather than logical operators.

P.S- I am new to coding in Java, and was just curious if there is an optimum way to write code that has better performance.

In addition to the things dfhwze already mentioned:

You should separate calculation and output. Even the shorter solution mentioned by dfhwze contains a repetition of the output code (System.out.println) and the output message ("Not weird"). Furthermore, thinking on to real world problems, code that only outputs something is really hard to unit-test.

Thus, do something like:

private static boolean isWeird(int n) {
   return (n % 2 != 0) || (n >= 6 && n <= 20); // or however you want to do the calculation
}

... // and in main:
boolean weird = isWeird(n);
if (weird) {
   System.out.println("Weird");
}
else {
   System.out.println("Not Weird");
}

One missing convention: variables should start with a lowecase character: N should be renamed to n


Unity Gravity script lowering FPS because of overlap sphere checking collision What are some ideas or ways that I can increase the efficiency of this gravity script because it is way to depending for what i want it for.
The game has a target at the center and there are objects far away all around the target and when gravity script is enabled I create an overlap sphere check which objects have a rigidbody and collision and pull them to target at a constant speed, but my FPS drops to the floor.
Would it be more efficient to have a script in each of these objects with a AddForce and direction towards target? Or any other ideas? Maybe something to change in this current script?
Am I right on the gravity script being too demanding because of the distance the gravity script is checking?
[RequireComponent(typeof(Rigidbody))]
public class Gravity : MonoBehaviour
{
private Rigidbody ownBody;
[HideInInspector]
public float maxVelocityOfInfluence;
private float rangeOfInfluence = 7000.0f;

private void OnEnable()
{
    if (ownBody == null)
    {
        ownBody = GetComponent<Rigidbody>();
    }
}

private void FixedUpdate()
{
    Collider[] cols = Physics.OverlapSphere(transform.position, rangeOfInfluence);
    List<Rigidbody> rbs = new List<Rigidbody>();

    foreach (Collider c in cols)
    {
        Rigidbody otherBody = c.attachedRigidbody;
        if (otherBody != null && otherBody != ownBody && !rbs.Contains(otherBody) &&
HasTagDownHierarchy(otherBody.transform, "Boat"))
        {
            rbs.Add(otherBody);

            Vector3 offset = transform.position - c.transform.position;
            otherBody.velocity = (offset.normalized * maxVelocityOfInfluence);
        }
    }
}

private static bool HasTagDownHierarchy(Transform transform, string tag)
{
    if (!transform)
    {
        return false;
    }

    if (transform.CompareTag(tag))
    {
        return true;
    }

    foreach (Transform child in transform)
    {
        if (HasTagDownHierarchy(child, tag))
        {
            return true;
        }
    }

    return false;
}

You are allocating like crazy use Physics.OverlapSphereNonAlloc
dont do List<Rigidbody> rbs = new List<Rigidbody>();
Replace with o(1) lookup HasTagDownHierarchy(otherBody.transform, "Boat") also avoid tags. Use markup classes.
edit: With look up I mean, in Boat Awake method cache the Colliders in a look up Dictionary<Collider, Boat>


Mongodb Post/Comment use case I am trying to learn Mongodb and decided to build a simple blog application using Zend Framework and Mongodb (using Shanty for ZF).

I have the following document for Post

class App_Model_Post extends Shanty_Mongo_Document
{
    protected static $_db = 'blog';
    protected static $_collection = 'posts';
    protected static $_requirements = array(
        'Title' => 'Required',
        'Content' => 'Required',
        'CreatedOn' => 'Required',
        'Author' => array('Document:App_Model_User', 'Required', 'AsReference'),
        'Slug' => 'Required',
    );

    public function preSave()
    {
       $title = $this->Title;
       $this->Slug = strtolower($title);
    }
}

Document for Comment:

class App_Model_Comment extends Shanty_Mongo_Document
{
    protected static $_db = 'blog';
    protected static $_collection = 'comments';
    protected static $_requirements = array(
        'Content' => 'Required',
        'Author' => array('Required', 'Document:App_Model_User', 'AsReference'),
        'Post' => array('Required', 'Document:App_Model_Post'),
    );
}

In my controller I do the following to get the Post and Comments for this Post:

public function viewAction()
{
    $slug = $this->_getParam('id');
    $post = App_Model_Post::one(array('Slug' => $slug));
    $this->view->post = $post;
    $this->view->title = 'Post Title : ' . $post->Title;

    $comments = App_Model_Comment::all(array('Post.Slug' => $post->Slug))->skip(0)->limit(10);
    $this->view->comments = $comments;
}

I wonder if this is a good way to accomplish this.  Is it good practice to reference to Post in the Comment or should I inject Comment documents to Post document?

The correct answer depends on a few application design decisions.

Reasons why you should embed Comments in Posts:

You have a limit on the number of comments that can be on a post
The comment limit is relatively small (mongodb docs have a hard limit
of 4mb but you don't want to be pulling 4mb data down at once, so
your limit should take this into consideration)
You need access to all comments on a post when you load a post

Reasons why you should reference Posts from Comments:

You want to have unlimited comments on a post
You're ok with loading the post first then going back to the database every time you need comments
You want to fetch comments without posts

I offer a combined approach. Store comments in their own collection but store a total comments counter and possibly recent comments against the post. This way you can have unlimited comments but still have access to important information when listing posts without having to go back to the database.

I hope this helps


Extracting JSON from string using regex I need to get the value of item inside of this string and parse it as JSON. I've got some working code but I feel like it could be optimized (a lot) and cleaned up (a lot). Any tips or pointers are appreciated. Thanks!

My current code:

let result  = script.match(/item:(.*|[\s\S]*)onSelected/gm)[0];
result = result.replace('onSelected', '').replace('item:', '').trim().slice(0, -1);
JSON.parse(result);

Here's the string of javascript I need to pull from:

jQuery(function($) {

    new Selector('itemSelect', {
      item: {"id":9343513159,"title":"The thing","description":"\u003cp\u003eThis is the description\u003c\/p\u003e\n\u003cp\u003eIt has details\u003c\/p\u003e\n","published_at":"2016-12-10T10:08:00-05:00","available":true,"individuals":[{"id":34972221767},{"id":34972221768},{"id":34972221769}],"images":["\/\/cdn.site.com\/s\/files\/1\/0094\/2252\/items\/image.jpg?v=1481382473"],"options":["Time"],"content":""},
      onSelected: selectCallback,
      enableState: true
    });

   // Some stuff here

    $('.main:eq(0)').prepend('<label class="forever" for="time">Forever</label>');

});

window.mainItem = window.mainItem || {};

mainItem.variables = {
  available : false
};

(function() {
  if (true) {
    mainItem.variables.available = true;
  }
})();

Use .exec to match a group
Add comma removed by slice directly into the regexp
Use capturing groups: query for [1], not for [0]

let script = document.getElementById('source').text;
var result = /item:(.*|[\s\S]*),\s*onSelected/gm.exec(script)[1];
console.log(JSON.parse(result));
<script id="source" type="text/plain">
jQuery(function($) {

        new Selector('itemSelect', {
          item: {"id":9343513159,"title":"The thing","description":"\u003cp\u003eThis is the description\u003c\/p\u003e\n\u003cp\u003eIt has details\u003c\/p\u003e\n","published_at":"2016-12-10T10:08:00-05:00","available":true,"individuals":[{"id":34972221767},{"id":34972221768},{"id":34972221769}],"images":["\/\/cdn.site.com\/s\/files\/1\/0094\/2252\/items\/image.jpg?v=1481382473"],"options":["Time"],"content":""},
          onSelected: selectCallback,
          enableState: true
        });

       // Some stuff here

        $('.main:eq(0)').prepend('<label class="forever" for="time">Forever</label>');

    });

    window.mainItem = window.mainItem || {};

    mainItem.variables = {
      available : false
    };

    (function() {
      if (true) {
        mainItem.variables.available = true;
      }
    })()
</script>


Rails routes.rb constraints I'm trying to think of a better way to write these routes in my routes.rb file:

Rails.application.routes.draw do
  root 'home#index'

  s = /section1|section2|section3|section4/
  y = /\d{4}/
  m = /\d{1,2}/

  resources :articles, except: [:index, :show]

  # The slug will replace the default params[:id] of the following resources:
  resources :articles, only: [:index, :show], path: '/:section/:year/:month', as: :my_articles, constraints: {:section => s, :year => y, month: m}

  get ':section/:year/:month', to: 'articles#by_month', as: :month, constraints: {section: s, year: y, month: m}
  get ':section/:year', to: 'articles#by_year', as: :year, constraints: {section: s, year: y}
  get ':section', to: 'articles#by_section', as: :section, constraints: {section: s}
end

This enables me to search articles by section, by year+section, by month+section+year. It also lets me add a nice slug to URLs.

One of the problems is that the constraints are common among all these routes. I'm wondering if there's a more clever way to go about writing this. Someone suggested I use concerns but I haven't figured out a way to do that.

Not a game changer, but I'm curious as to how I can write this properly.

Got it:

Rails.application.routes.draw do
  root 'home#index'

  resources :articles, except: [:index, :show]
  constraints section: /section1|section2|section3|section4/ do
    constraints year: /\d{4}/ do
      constraints month: /\d{1,2}/ do
        resources :articles, only: [:index, :show], path: '/:section/:year/:month', as: :my_articles
        get ':section/:year/:month', to: 'articles#by_month', as: :month
      end
      get ':section/:year', to: 'articles#by_year', as: :year
    end
    get ':section', to: 'articles#by_section', as: :section
  end

I feel this is much cleaner :)


Generic repository pattern Please have a through look my custom implementation of generic repository pattern and suggest anything I may be lacking.

I am using a generic interface with 6 methods
I'll be using a repository class for each entity type (e.g UserRepository, ProductRepository, StoreRepository) which are not only going to implement the generic interface but also can add additional methods as per the need with respect to each repository class. for example:

UserRepository can add a new method named as CountUsers() which gets called from controller.
ProductRepository can add a new method named as GetExpiredProducts() which gets called from controller.

I am declaring IGenericRepository<User> in UserController, similarly in other controllers and using repositoryPattern.

Generic Repository Interface

public interface IGenericRepository<T> : IDisposable where T : class
{
    IEnumerable<T> SelectAll();
    T SelectByID(object id);
    void Insert(T obj);
    void Update(T obj);
    void Delete(object id);
    void Save();
}

Implementation of Generic Repository Interface

public class UserRepository : IGenericRepository<User>
{
    private ModelDBEntities dataContext = null;
    public UserRepository()
    {
        this.dataContext = new ModelDBEntities();
    }
    public UserRepository(ModelDBEntities dataContext)
    {
        this.dataContext = dataContext;
    }

    public IEnumerable<User> SelectAll()
    {
        return dataContext.Users;
    }
    public User SelectByID(object argId)
    {
        long userId = long.Parse(argId.ToString());
        return dataContext.Users.Single(u => u.UserId == userId);
    }
    public void Insert(User obj)
    {
       dataContext.Users.AddObject(obj);
    }
    public void Update(User obj)
    {
        if (obj.EntityState == EntityState.Detached)
            dataContext.Users.Attach(obj);
        dataContext.ObjectStateManager.ChangeObjectState(obj, System.Data.EntityState.Modified);
    }
    public void Delete(object argUserId)
    {
        long userId = long.Parse(argUserId.ToString());
        dataContext.Users.Single(u => u.UserId == userId).Status = "inactive";
    }
    public void Save()
    {
        dataContext.SaveChanges();
    }

    private bool disposed = false;
    protected virtual void Dispose(bool disposing)
    {
        if (!this.disposed)
        {
            if (disposing)
            {
                dataContext.Dispose();
            }
        }
        this.disposed = true;
    }
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
}

My User Controller

public class UserController : Controller
{
    private IGenericRepository<User> _repositoryUser = null;
    public UserController()
    {
        _repositoryUser = new UserRepository();
    }
    public UserController(IGenericRepository<User> repository)
    {
        _repositoryUser = repository;
    }

    [HttpPost]
    public JsonResult SubmitHandler(User argUser)
    {
        if (ModelState.IsValid)
        {
            _repositoryUser.Insert(argUser);
            _repositoryUser.Save();
            return Json("success");
        }
        else
            return Json("failure");
    }
    public ActionResult Delete(object Id)
    {
            if (Id != null && Id.ToString().Trim().Length > 0)
            {
                int param = int.Parse(Id.ToString());
                _repositoryUser.Delete(Id as object);
                _repositoryUser.Save();
                return View("User", _repositoryUser.SelectAll());
            }
            else
                return View("404");
    }
}

I have created generic repositories many times and what I found was that you're going to need to create interfaces for each entity typed repository.
So IUserRepository that implements IGenericRepository<User>, this way you'll be able to

Inject the repository easier with Inversion Of Control
Create extra methods for each entity type as and when
Test your controller easier

Update
I realise this update comes a long time after the question, but time has past and experience has been gained.
You don't necessarily need a new interface for entity type. IoC containers can inject a specific implementation based on the generic parameter of the IGenericRepositoy interface. You will need an interface however for each implementation that has specific methods for that type, such as GetExpiredProducts() as mentioned in the initial question.
You could also remove the generic parameter from the interface and move the where clause to each method:
public interface IGenericRepository : IDisposable
{
    IEnumerable<T> SelectAll() where T : class;
    T SelectByID(object id) where T : class;
    void Insert(T obj) where T : class;
    void Update(T obj) where T : class;
    void Delete(object id);
    void Save();
}

then implement a GenericRepository class:
public class GenericRepository : IGenericRepository
{
    public GenericRepository()
    {
        blah...
    }
    public GenericRepository(ModelDBEntities dataContext)
    {
        blah...
    }

    public IEnumerable<T> SelectAll() where T : class
    {
        blah...
    }
    public T SelectByID(object argId) where T : class
    {
        blah...
    }
    public void Insert(T obj) where T : class
    {
       blah...
    }
    public void Update(T obj) where T : class
    {
        blah...
    }
    public void Delete(object argId)
    {
        blah...
    }
    public void Save()
    {
        blah...
    }
}

this would mean that any implementation of a type specific repository could just inherit this saving you some work
public interface IUserRepository : IGenericRepository
{
    int CountUsers();
}

public class UserRepository : GenericRepository, IUserRepository
{
    public UserRepository()
    {
        blah...
    }
    public UserRepository(ModelDBEntities dataContext)
        : base(dataContext)
    {
        blah...
    }

    public int CountUsers()
    {
        blah...
    }
}

You can then decide on what you want to inject into your controller, if you just want the base CRUD methods then you will only need to pass in the implementation of IGenericRepository and use for several types.


Circumference and Area of a Circle in COBOL I recently started Learning COBOL from Lynda and this is my code for the first challenge .

The Objective is that the user will provide the radius and we have to calculate the Circumference and the area of the circle and then display it.

Here is the Code.

       IDENTIFICATION DIVISION.
   PROGRAM-ID. CIRCUMFERENCE AS "CIRCUMFERENCE".
   AUTHOR. ASWIN MOHAN.

  ***************************************************************
  *    Create a Program to read the Radius of a circle and THEN
  *    print out the Circumference and the radius of the circle.
  ***************************************************************

   ENVIRONMENT DIVISION.

   CONFIGURATION SECTION.
   SOURCE-COMPUTER.
   OBJECT-COMPUTER.

   INPUT-OUTPUT SECTION.

   DATA DIVISION.

   FILE SECTION.

   WORKING-STORAGE SECTION.
    01   WS-CONSTANTS.
       05    WS-PI                 PIC 9V9999  VALUE 3.1415.
    01   WS-CIRCLE.
       05    WS-RADIUS             PIC 999V99 VALUE ZEROES.
       05    WS-CIRCUMFERENCE      PIC 9999V99 VALUE ZEROES.
       05    WS-AREA               PIC 99999V99 VALUE ZEROES.
    01   WS-DISPLAY.
       05    WS-DISPLAY-CIRCUMFERECE   PIC ZZ99.99.
       05    WS-DISPLAY-AREA           PIC ZZ999.99.

   PROCEDURE DIVISION.
    0100-PROCESS-RECORDS.

        DISPLAY "Enter the Radius".
        ACCEPT WS-RADIUS.

        COMPUTE WS-CIRCUMFERENCE = 2 * WS-PI * WS-RADIUS.
        MOVE WS-CIRCUMFERENCE TO WS-DISPLAY-CIRCUMFERECE.

        COMPUTE WS-AREA = WS-PI * WS-RADIUS * WS-RADIUS.
        MOVE WS-AREA TO WS-DISPLAY-AREA.

        DISPLAY "The Circumferece is " , WS-DISPLAY-CIRCUMFERECE.
        DISPLAY "The Area is " , WS-DISPLAY-AREA.

        STOP RUN.

        END PROGRAM CIRCUMFERENCE.

I'm using GNU-COBOL and I'm worried that my COBOL coding style is not so great. I'm just starting out so all feedback is appreciated.

Is Lynda that woman from LinkedIn, or just some mate of yours? I wonder how good a teacher she is, and what her intent is. Is it to learn COBOL, or GnuCOBOL.

The reason I say that is that COBOL has Standards. The latest is 2014, which supersedes 2002, then there's 1985 and 1974. There is a 1968 Standard, but it is extremely unlikely that any current compiler in use is to that Standard.

There are current compilers to the 1974 Standard. However, I'd not recommend that as a lowest common denominator, because the 1985 Standard introduced so much that eases coding and use of COBOL.

If learning COBOL (or being taught COBOL) with the intention of using it in a paid job, I'd recommend sticking to the 1985 Standard.

There are also Language Extensions. Everyone has them. Sometimes they coincided, sometimes they don't. GnuCOBOL supports many Extensions from other compilers. However, again, if the aim is to learn COBOL to get a job, beware of the casual use of Extensions.

An area where there are an absolute plethora of Extensions (to 1985) is in human interaction. DISPLAY and ACCEPT can be entirely different beasts from compiler to compiler. To use them in a portable way, use their very simplest forms.

This is sometimes not easy, because although your PROCEDURE DIVISION use is simple, you are, unknowingly, using a GnuCOBOL extension which provides conversion when the target of ACCEPT is a numeric value. If you were to work on an IBM COBOL, the same code would not be reliable.

I agree with Bruce and Edward in their answers. Do something about how your program looks. Exactly what you do in a personal sense depends. Be aware that if you get a COBOL job, you will likely be working in a team, and there will likely be local standards for how your program looks, and how things are done. This eases the ability to pick up someone else's program. You will be able to suggest changes for "bad" practices, but if you can't win the argument, do what the others do at your site.

Next, not so much a COBOL thing as a "programming" thing, I hope, although there are COBOL specifics affecting it. Your program doesn't work.

You can take from the user a maximum value of 999.99 for a radius. Your definitions, specifically for the area, do not allow for a radius of that size.

In COBOL you have to think longer about such things, because definitions, except when they are not, are fixed in size. The size you have for your fields containing "area" are orders of magnitude off. The maximum area is over three million for your maximum radius, but you only allow for five integer digits in your definitions.

Edward suggests using a more accurate value for PI. This is not something to be done blindly. The precision of PI to use should be defined in the specification for the program, as should any and all other limits and required precision. You can't write a COBOL program and then say to the Accountant "well, it works the way I've defined it".

Readability of output is also important. Use separators for large numbers, and zero-suppress down to the single digit before the decimal point. Unless your specification says otherwise. Line the numbers in the output on the decimal point. Much easier to consume as a human reader. Or switch the output to columnar with headings. Depends on the spec.

Be careful of using VALUE clauses unnecessarily. All of your VALUE ZEROES (which I'd code as "VALUE ZERO", but what the heck) are unnecessary. All of those fields are targets before they are sources, so the value of zero will never be relevant to anything. Having the VALUE implies that the value has a significance, which is misleading when it does not. Don't worry, very few programmers actually do that :-)

Indentation is for meaning. If the indentation makes the meaning clearer, do it. If not, don't. There is no point that I can discern in indenting 01-levels beyond what is necessary, or paragraph/SECTION-names.

No one mentioned the commas. Don't use commas (except in literals). They have no meaning. Don't use semi-colons. They have no meaning. To confirm this, add this line anywhere in the PROCEDURE DIVISION and recompile.

        ,,,,;;; , , , ; ; ;

Still compile as before? Still run as before? What is the point then. Ok, how do you make it easier for a human to read when there are multiple targets/sources:

        ADD A
            B
            C                    TO D
                                    E
                                    F

The names are just for examples, and you should always use meaningful names, for everything. It helps the source code tell the story of the program.

If you are going to use comments, make sure they are accurate. Comments are never compiled, so are only ever "checked" by the human reader. Your comments have significant errors and misinformation. Don't do that. If you then find there is little to say in a comment, because you have structured your program, and used good names, feel free not to use many comments (subject to local standards, of course, which can at times be thoroughly stupid in this particular area).

Here's your program applying most of the above. I've left the ACCEPT changes as an exercise for you.

   IDENTIFICATION DIVISION.
   PROGRAM-ID. CIRCUMF.
  *    Obtain the Radius of a circle and THEN
  *    print out the Circumference and the area of the circle.
   DATA DIVISION.
   WORKING-STORAGE SECTION.
   01  WS-PI                               PIC 9V9999
                                            VALUE 3.1415.
   01  WS-CIRCLE.
       05  WS-RADIUS                       PIC 999V99.
       05  WS-CIRCUMFERENCE                PIC 9(4)V99.
       05  WS-AREA                         PIC 9(7)V99.
   01  WS-DISPLAY.
       05  WS-DISPLAY-CIRCUMFERECE         PIC Z,ZZZ,ZZ9.99.
       05  WS-DISPLAY-AREA                 PIC Z,ZZZ,ZZ9.99.
   PROCEDURE DIVISION.
       DISPLAY
               "Enter the Radius"
       ACCEPT
              WS-RADIUS
       COMPUTE WS-CIRCUMFERENCE     = 2 * WS-PI
                                      * WS-RADIUS
       MOVE WS-CIRCUMFERENCE        TO WS-DISPLAY-CIRCUMFERECE
       COMPUTE WS-AREA              = ( WS-PI
                                        * ( WS-RADIUS
                                          * WS-RADIUS ) )
       MOVE WS-AREA                 TO WS-DISPLAY-AREA
       DISPLAY
               "The Circumferece is "
               WS-DISPLAY-CIRCUMFERECE
       DISPLAY
               "The Area is         "
               WS-DISPLAY-AREA
       STOP RUN
       .

For the ACCEPT, you should first DISPLAY to tell the user what they are doing, and the limits to what they enter. The target of the ACCEPT should be an alpha-numeric (PIC X field of some size) and you should process, including validation, what is entered and only treat it as a number within the permissible range if that is confirmed. Try running your program with 999999999.999999 as input, and ponder.

In looking at this, I encountered something curious, and have posted as a possible issue on the GnuCOBOL site at SourceForge.Net. On the ACCEPT, for any input value above 99.99 I get a zero as the second decimal place, even if I have entered a non-zero digit there.

Confirmed as a bug. The fix is in shortly.

I you are building your compiler from a package, or installing a pre-built one, a work-around for the issue:

       05  WS-RADIUS-OVERSIZE              PIC 9(4)V99.
       05  FILLER
           REDEFINES WS-RADIUS-OVERSIZE.
           10  FILLER                      PIC X.
           10  WS-RADIUS                   PIC 999V99.

And

       ACCEPT WS-RADIUS-OVERSIZE

Instead of the existing ACCEPT, and then use WS-RADIUS as previously.

The code will then continue to work even with the new compiler when updated.

You should clearly comment any code like that, that is a work-around for some issue or other.


F# Simple Tree evaluation I'm currently trying to learn the idiomatic way to write F# code. The code simulates a basic calculator with only binary plus, minus and unary minus.

I'd like to know if there is a better way to group union cases than the way I did it. Should the Add/Sub/MinusExpression be cases in the Expression union? Is there a better way to write the eval function?

type Expression =
    | BinExpression of BinExpression
    | UnExpression  of UnExpression
    | IntExpression of int

and BinExpression =
    | AddExpression of Expression * Expression
    | SubExpression of Expression * Expression

and UnExpression =
    | MinusExpression of Expression

let rec eval tree =
    let evalBin tree =
        match tree with
        | AddExpression(e1, e2) -> (eval e1) + (eval e2)
        | SubExpression(e1, e2) -> (eval e1) - (eval e2)

    let evalUn tree =
        match tree with
        | MinusExpression(v) -> -(eval v)

    match tree with
    | BinExpression(e) -> evalBin e
    | UnExpression(e)  -> evalUn e
    | IntExpression(v) -> v

For a simple domain like that, wouldn't it be simpler to define the data like this?

type Expression =
| IntExpr of int
| AddExpr of Expression * Expression
| SubtractExpr of Expression * Expression
| NegationExpr of Expression

This would mean that you can implement the eval function like this:

let rec eval = function
| IntExpr i -> i
| AddExpr (x, y) -> eval x + eval y
| SubtractExpr (x, y) -> eval x - eval y
| NegationExpr x -> -(eval x)

Here are some FSI examples:

> let x1 = IntExpr 42 |> eval;;
val x1 : int = 42

> let x2 = AddExpr (IntExpr 1, IntExpr 32) |> eval;;
val x2 : int = 33

> let x3 = NegationExpr (SubtractExpr (NegationExpr (IntExpr 3), (AddExpr (IntExpr 4, IntExpr 7)))) |> eval;;
val x3 : int = 14


"Pretty" date generator I have this "pretty" date string generator in C# - pass it a date and it returns a string with "5 minutes ago" or "2 weeks, 3 days ago", etc.

It's a little verbose and hogs 61 lines, and I'm wondering if I'm missing out on some nice C# features or something. What (if any) is the best way to clean up this code? Are there any cool c# features I can use here?

public static string getTimeSpan(DateTime postDate)
{
    string stringy = "";
    TimeSpan diff = DateTime.Now.Subtract(postDate);
    double years = Math.Floor(diff.TotalDays / 365);
    double weeks = Math.Floor(diff.TotalDays / 7);
    double days = diff.Days;
    double hours = diff.Hours + days * 24;
    double minutes = diff.Minutes + hours * 60;
    if (minutes <= 1) {
        stringy = "Just Now";
    } else if (years >= 1) {
        if (years >= 2) {
            stringy = years.ToString() + " years ago";
        } else {
            stringy = "1 year ago";
        }
    } else if (weeks >= 1) {
        if ((days - weeks * 7) > 0) {
            if ((days - weeks * 7) > 1) {
                stringy = ", " + (days - weeks * 7).ToString() + " days";
            } else {
                stringy = ", " + (days - weeks * 7).ToString() + " day";
            }
        }
        if (weeks >= 2) {
            stringy = weeks.ToString() + " weeks" + stringy + " ago";
        } else {
            stringy = "1 week" + stringy + " ago";
        }
    } else if (days >= 1) {
        if ((hours - days * 24) > 0) {
            if ((hours - days * 24) > 1) {
                stringy = ", " + (hours - days * 24).ToString() + " hours";
            } else {
                stringy = ", " + (hours - days * 24).ToString() + " hour";
            }
        }
        if (days >= 2) {
            stringy = days.ToString() + " days" + stringy + " ago";
        } else {
            stringy = "1 day" + stringy + " ago";
        }
    } else if (hours >= 1) {
        if ((minutes - hours * 60) > 0) {
            if ((minutes - hours * 60) > 1) {
                stringy = ", " + (minutes - hours * 60).ToString() + " minutes";
            } else {
                stringy = ", " + (minutes - hours * 60).ToString() + " minute";
            }
        }
        if (hours >= 2) {
            stringy = hours.ToString() + " hours" + stringy + " ago";
        } else {
            stringy = "1 hour" + stringy + " ago";
        }
    } else if (minutes > 1) {
        stringy = minutes.ToString() + " minutes ago";
    }
    return stringy;
}

Use PascalCase for the method name
Move the calc for years and months lower to be minutely "more efficient"
Use inline return to reduces nesting
Use ternary operator (?:) for simple logic to reduce if/else clutter
Use the format override of ToString(string format) to reduce string concats
Use string.Format with a ternary to reduce duplication

The shorter version I came up with is 40 lines, but you can judge if it readable enough.

    public static string GetTimeSpan(DateTime postDate) {
        string stringy = string.Empty;
        TimeSpan diff = DateTime.Now.Subtract(postDate);
        double days = diff.Days;
        double hours = diff.Hours + days*24;
        double minutes = diff.Minutes + hours*60;
        if (minutes <= 1) {
            return "Just Now";
        }
        double years = Math.Floor(diff.TotalDays/365);
        if (years >= 1) {
            return string.Format("{0} year{1} ago", years, years >= 2 ? "s" : null);
        }
        double weeks = Math.Floor(diff.TotalDays/7);
        if (weeks >= 1) {
            double partOfWeek = days - weeks*7;
            if (partOfWeek > 0) {
                stringy = string.Format(", {0} day{1}", partOfWeek, partOfWeek > 1 ? "s" : null);
            }
            return string.Format("{0} week{1}{2} ago", weeks, weeks >= 2 ? "s" : null, stringy);
        }
        if (days >= 1) {
            double partOfDay = hours - days*24;
            if (partOfDay > 0) {
                stringy = string.Format(", {0} hour{1}", partOfDay, partOfDay > 1 ? "s" : null);
            }
            return string.Format("{0} day{1}{2} ago", days, days >= 2 ? "s" : null, stringy);
        }
        if (hours >= 1) {
            double partOfHour = minutes - hours*60;
            if (partOfHour > 0) {
                stringy = string.Format(", {0} minute{1}", partOfHour, partOfHour > 1 ? "s" : null);
            }
            return string.Format("{0} hour{1}{2} ago", hours, hours >= 2 ? "s" : null, stringy);
        }

        // Only condition left is minutes > 1
        return minutes.ToString("# minutes ago");
    }


Saving settings to Windows registry as a tuple Regarding an application written in C++ using the Windows API. I'd like to store a user setting like the window position on program exit, to the Windows registry and retrieve settings like these on the next start of the program.

I currently have 3 settings to be stored/retrieved to/from the registry.

typedef struct tagWINPOS
{
  DWORD dwWindowStyle;
  int iWindowX;
  int iWindowY;
} winpos_t;

HKEY hKeyApp = (HKEY)INVALID_HANDLE_VALUE;
const char szWindowStyle[] = "WindowStyle";
const char szWindowX[] = "WindowX";
const char szWindowY[] = "WindowY";

Later on in the code I'm calling functions like RegSetValueEx and RegQueryValueEx always three times. Like so:

int SaveSettings( HWND hWnd )
{
  winpos_t wpos;
  // window dimensions from hWnd are stored to wpos, not shown

  RegSetValueEx( hKeyApp, szWindowStyle, 0,
    REG_DWORD,
    (const BYTE*)&wpos.dwWindowStyle,
    sizeof(wpos.dwWindowStyle) );
  RegSetValueEx( hKeyApp, szWindowX, 0,
    REG_DWORD,
    (const BYTE*)&wpos.iWindowX,
    sizeof(wpos.iWindowX) );
  RegSetValueEx( hKeyApp, szWindowY, 0,
    REG_DWORD,
    (const BYTE*)&wpos.iWindowY,
    sizeof(wpos.iWindowY) );

  // further irrelevant code removed
}

Things start to look a bit tedious. So I was thinking to put all the parameters for one function call (say RegSetValueEx) into an std::array and then make a for loop iterating over all the array elements.

Because the Windows registry can store multiple data types, like DWORD (32-bit), QWORD (64-bit) and strings. I thought about std::variant to list all of these for this data field.

I'm started with a little test program like this:

#include <cstddef>
#include <cstdio>
#include <cstdint>
#include <tuple>
#include <variant>

typedef std::variant< int32_t, int64_t, char* > data_t;

typedef std::tuple<
  data_t,       // setting data
  const char*,  // setting name
  std::size_t   // size of setting [bytes]
> setting_t;

typedef std::array< setting_t, 2 > settings_t;

constexpr int reg_dword = 0;
constexpr int reg_qword = 1;

constexpr settings_t settings{
  std::make_tuple( (std::in_place_index<reg_dword>, 33), "Mydata", 4 ),
  std::make_tuple( (std::in_place_index<reg_qword>, 34), "Mydata2", sizeof(std::in_place_index_t<reg_qword>) ) };

int main()
{
  printf( "name: %s, value %ld\n", std::get<1>( settings[0] ), std::get<0>( std::get<0>( settings[0] ) ));

  getchar();
  return EXIT_SUCCESS;
}

Anyway, do people consider this approach a good one to get to my goal of having to spell out a single function call of RegSetValueEx inside a for loop?

The initialization of the array has much boilerplate as well as the effort to obtain values. What is a good approach to improve the readability of this? Maybe a constexpr function like std::make_tuple but then more specific for my case?

you need save winpos_t as single value with REG_BINARY instead 3 different values. possible implementation:

class CConfig
{
    HKEY _hKey;
public:

    ~CConfig()
    {
        if (_hKey)
        {
            RegCloseKey(_hKey);
        }
    }

    CConfig() : _hKey (0)
    {
    }

    LSTATUS Save(
        PCWSTR lpValueName,
        DWORD dwType,
        const void* lpData,
        DWORD cbData)
    {
        return RegSetValueExW(_hKey, lpValueName, 0, dwType, (BYTE*)lpData, cbData);
    }

    LSTATUS Load(
        PCWSTR lpValueName,
        PDWORD lpType,
        void* lpData,
        PDWORD lpcbData)
    {
        return RegQueryValueExW(_hKey, lpValueName, 0, lpType, (BYTE*)lpData, lpcbData);
    }

    LSTATUS Init(HKEY hKey, PCWSTR lpSubKey)
    {
        return RegCreateKeyExW(hKey, lpSubKey, 0, 0, 0, KEY_ALL_ACCESS, 0, &_hKey, 0);
    }
};

void test_cfg()
{
    CConfig cfg;
    if (!cfg.Init(HKEY_CURRENT_USER, L"Software\\MyKey"))
    {
        struct winpos_t
        {
            DWORD dwWindowStyle;
            int iWindowX;
            int iWindowY;
        };

        winpos_t test_pos = { 1, 2, 3};

        static const PCWSTR szwinpos_t = L"winpos_t";

        if (!cfg.Save(szwinpos_t, REG_BINARY, &test_pos, sizeof(test_pos)))
        {
            ULONG type, cb = sizeof(test_pos);
            RtlZeroMemory(&test_pos, sizeof(test_pos));
            if (!cfg.Load(szwinpos_t, &type, &test_pos, &cb) && type == REG_BINARY && cb == sizeof(test_pos))
            {
                DbgPrint("{%x, %x, %x}\n", test_pos.dwWindowStyle, test_pos.iWindowX, test_pos.iWindowY);
            }
        }
    }
}


Getting saddle points of a matrix My homework assignment:

  Write a function called saddle that finds saddle points in the input matrix M. For the purposes of this problem, a saddle point is defined as an element whose value is greater than or equal to every element in its row, and less than or equal to every element in its column. Note that there may be more than one saddle point in M. Return a matrix called indices that has exactly two columns. Each row of indices corresponds to one saddle point with the first element of the row containing the row index of the saddle point and the second element containing the column index. If there is no saddle point in M, then indices is the empty array.

My solution:

function ind=saddle(matrix)

size_=size(matrix);
a=[];
for ii=1:size_(1)

    for jj=1:size_(2)
        if (matrix(ii,jj)==max(matrix(ii,:))) && (matrix(ii,jj)==min(matrix(:,jj)))
            a=[a;[ii ,jj]];
        end
    end
end
ind=a;

Your solution works, but it will be quite slow. For every element in your matrix (m*n many), you check if it is the maximum in the row (n comparisons), and then check if it is the minimum in the column (m comparisons). This will mean that you do a total of m*n*(m+n) many comparisons. You can save some time by only computing the maximum and minimum once, going down to m*n + m + n.

Also, MATLAB hates for loops. Built-ins are optimized and can take advantage of any hardware acceleration or parallelism that your computer offers. Loops on the other hand are always executed sequentially. If you can avoid them and use built-ins instead, it will usually make your code faster.

function [row, col]=saddle(matrix)

% first lets get the min/max for the column/row respectively
minima = min(matrix,[],1) % minimum of each column
maxima = max(matrix,[],2) % maximum of each row

% then, for each element in the column/row check if it matches the min/max
isMinimum = minima == matrix
isMaximum = matrix == maxima

% it needs to be both to be a saddle
saddles = isMinimum & isMaximum

% lastly, figure out the index of each saddle point
% and transform it to the desired output format
[row, col] = find(saddles)

Disclaimer: My MATLAB licence expired this week and I have to wait to renew it. The above code is untested, so there might be the odd typo.


DM::OJ Slot Machines challenge in Python 3 I have written a solution to the Slot Machines challenge on DM::OJ:

  Martha takes a jar of quarters to the casino with the intention of becoming rich. She plays three machines in turn. Unknown to her, the machines are entirely predictable. Each play costs one quarter. The first machine pays 30 quarters every 35th time it is played; the second machine pays 60 quarters every 100th time it is played; the third pays
  9 quarters every 10th time it is played.

  Input Specification

  Your program should take as input the number of quarters in Martha's jar (there will be at least one and fewer than 1000), and the number of times each machine has been played since it last paid.

  Output Specification

  Your program should output the number of times Martha plays until she goes broke.

  Sample Input

48
3
10
4

  Sample Output

Martha plays 66 times before going broke.

I am seeking to make it run in 2 seconds or less.

money = int(input())
m1 = int(input())
m2 = int(input())
m3 = int(input())
turns = 0

while (money != 0):
  #machine 1
  money -= 1
  m1 += 1
  if (m1 == 35):
    money += 30
    m1 = 0
  turns += 1

  #machine 2
  money -= 1
  m2 += 1
  if (m2 == 100):
    money += 60
    m2 = 0
  turns += 1

  #machine 3
  money -= 1
  m3 += 1
  if (m3 == 10):
    money += 9
    m3 = 0
  turns += 1

print ('Martha plays {} times before going broke.'.format(turns))

Code organisation, separation of concerns and tests

The code is a single monolithic piece of code. It could be a good idea to reorganise things a bit: to make it clearer, to make it easier to test, to make it easier to maintain, etc.

The first major improvement could be write a function with a clear input (money, m1, m2, m3) and a clear output solving the issue we care about: computing the number of turns.
The logic handling the input/output is separated from the logic performing the computation - seel also https://en.wikipedia.org/wiki/Separation_of_concerns .

Then, this function could be used with inputs from the user but we could also feed it hardcoded values. This can be used to write tests to ensure that the function behaves properly (at least on the example provided by dmoj).

(In the code provided below, I've a simple assert statement to write the tests but it could be a good chance to read about unit test frameworks and start to use them).

Then, you can add any number of tests easily.

Now that we have unit-tests to prevent us from breaking things too badly, we can start tweaking the code.

Finally, you could use if __name__ == "__main__": to separate the definitions in your code from the code actually using these definitions. It makes your code easier to reuse via the import mechanism.

Style

Python has a Style Guide called PEP 8. It is followed (more or less stricly) by the Python community. It is highly recommended to read it and try to stick to it when relevant. In your case, you have superfluous parenthesis, indentations of 2 spaces instead of 4, superfluous whitespaces.

At this stage, we have:

def get_number_of_games(money, m1, m2, m3):
    """Return the number of games <to be continued>."""
    turns = 0
    while money != 0:
        #machine 1
        money -= 1
        m1 += 1
        if m1 == 35:
            money += 30
            m1 = 0
        turns += 1

        #machine 2
        money -= 1
        m2 += 1
        if m2 == 100:
            money += 60
            m2 = 0
        turns += 1

        #machine 3
        money -= 1
        m3 += 1
        if m3 == 10:
            money += 9
            m3 = 0
        turns += 1
    return turns

def interactive_main():
    money = int(input())
    m1 = int(input())
    m2 = int(input())
    m3 = int(input())
    print('Martha plays {} times before going broke.'.format(get_number_of_games(money, m1, m2, m3)))

def unit_test():
    """Run unit-tests."""
    assert get_number_of_games(48, 3, 10, 4) == 66
    print("OK")

if __name__ == "__main__":
    unit_test()

Magic numbers

The code is full of magic numbers, making the logic hard to understand.
We could store these values into constant with a meaningful name.

Even better, we could store these into appropriate data structures. For a simple solutions, we could decide to use namedtuples.

from collections import namedtuple

Machine = namedtuple('Machine', ['winning_freq', 'profit'])

MACHINE1 = Machine(35, 30)
MACHINE2 = Machine(100, 60)
MACHINE3 = Machine(10, 9)

def get_number_of_games(money, m1, m2, m3):
    """Return the number of games <to be continued>."""
    turns = 0
    while money != 0:
        money -= 1
        m1 += 1
        if m1 == MACHINE1.winning_freq:
            money += MACHINE1.profit
            m1 = 0
        turns += 1

        #machine 2
        money -= 1
        m2 += 1
        if m2 == MACHINE2.winning_freq:
            money += MACHINE2.profit
            m2 = 0
        turns += 1

        #machine 3
        money -= 1
        m3 += 1
        if m3 == MACHINE3.winning_freq:
            money += MACHINE3.profit
            m3 = 0
        turns += 1
    return turns

Un-handled situations (also known as bugs)

The code is not very robust. A few situations can lead it astray.

Let's take a simple example: we have 1 quarter and the machines haven't been used. This can be written as:

print(get_number_of_games(1, 0, 0, 0))

When we run this, we are stuck into a infinite loop. I'll let you investigate the reason why.

Another example - which could be considered invalid but is definitly worth fixing because of how simple it is: we have a lot of money and the machines have been used a lot. For instance, we have:

print(get_number_of_games(1000, 36, 101, 11))

Assuming we fix the previous issue: how many turns do we play ? How many times have we won ?

As we fix behaviors, you can add corresponding unit-tests to be sure you don't fall into the same issue later on.

Going further

The code relies on duplicated logic to handle the different machines. A better solution could be to handle them via the same logic. We could imagine having a list of machines.

Also, we could imagine having the numbers of turns played on a given machine stored along with the other information in a Machine class. Then, the get_number_of_games functions could take as inputs an amount of money and a list of properly defined Machines.

Take-away for Python and non-Python code

Make your code testable
Make your code tested and correct
Improve it while keeping properties 1 and 2.


Calculate permutation from Factorial representation of the K-th iteration of the N-th permutation As a follow of a post I made on the Mathematics Stack Exchange I wrote out a simple program that computes the K-th permutation from 0 to N-1 where the number of permutations is N!. What I was wondering is could I get some pointers on how to improve and re-write the permutation function to be more efficient--e.g. possibly getting rid of the vector and the for-loop to calculate the factorial.

#include <iostream>
#include <vector>
#include <cmath>
using namespace std;

///<summary>Calculates the permutation of K in range of 0 to N!. (MAX of N = 20--2^61).</summary>
vector<size_t> factorial_permutation(size_t N, uint64_t K) {
    vector<size_t> Perm;

    uint64_t FactN = N,  //Factorial of N
        RmdrK = K, //Remainder of K/(N-1)!
        IterN = N, //Downward iterator for N
        PostK; //The factorial position of K for each sub permutation.

    for(uint64_t I = N-1; I > 1; I--)
        FactN *= I;
    for (uint64_t I = 1; I <= N; I++) {
        FactN /= IterN--;
        PostK = RmdrK / FactN;
        RmdrK = RmdrK - (PostK * FactN);
        Perm.push_back(PostK);
    }

    return Perm;
}

int main() {
    size_t N = 0;
    uint64_t K = 0;
    vector<size_t> c = factorial_permutation(N, K);
    vector<size_t> s; //The factorial permutation sequence
    vector<size_t> e; //The permutation built from the factorial sequence

    //Builds the basic order sequence 0 to N-1 before permutation.
    for(int i = 0; i < N; i++)
        s.push_back(i);

    //Pushes the final result converted from factorial permutation to real permutation. For each entry of sequence s[c[i]] is an index into e[i] to build the permutation.
    for (int i = 0; i < N; i++) {
        e.push_back(s[c[i]]);
        s.erase(s.begin() + c[i]);
    }

    // Outputs the final permutation.
    for (int i = 0; i < N; i++)
        cout << e[i];

    cin.get();
};

The function factorial_permutation calculates the factorial representation of K--the K-th iteration--in a sequence from 0 to (N-1)! possible permutations. The main() function does simple conversion from the factorial representation to building the actual sequence. I would like to combine the two and see if I can negate the vectors all together for the sake of performance.

I'm also aware that I can remove the vector in factorial_permutation by using a basic array, but I can't seem to negate the one in the main() function.

The loop

for(int i = 0; i < N; i++)
    s.push_back(i);

is idiomatically expressed as std::iota(s.begin(), s.end(), 0) (you'd need to #include <numeric>).
A no naked loops principle demands giving a name to the

    for (int i = 0; i < N; i++) {
        e.push_back(s[c[i]]);
        s.erase(s.begin() + c[i]);
    }

loop. It surely represents an important algorithm. What exactly does it do?

In any case, the s and e vectors shall not be exposed to the client.
Computing factorials restricts the utility of the code to N < 21 (21! = 0x2_c507_7d36_b8c4_0000 doesn't fit in 64 bits). Unfortunately you cannot even express K within uint64_t limits for larger N. Consider BigInteger.
It is possible to avoid factorials (as well as ancillary vectors) by working other way around. Consider a pseudocode:

    while N > 0:
        index = K % N
        swap a[N-1] with a[index]
        K /= N
        N -= 1

While correct, it doesn't produce the same K'th permutation as your code does. May I suggest changing it to produce the correct sequence as an exercise?

PS: a mandatory warning on using namespace std


Multithread the variant of matrix multiplication This is matrix multiplication. Big matrix split on the submatrix and multiplication in parallel, result matrices merge to result. How i can make this code better?

public class MatrixMultiplication implements Callable<int[][]> {
private static int LENGTH_OF_SIDE = 1000;
private int taskCount = 4;
private int[][] matrixA;
private int[][] matrixB;

public MatrixMultiplication(int[][] matrixA, int[][] matrixB) {
    this.matrixA = matrixA;
    this.matrixB = matrixB;
}

public void setTaskCount(int taskCount) {
    this.taskCount = taskCount;
}

@Override
public int[][] call() {
    int n = matrixA.length;
    int m = matrixA[0].length;
    int p = matrixB[0].length;
    try {
        if (n >= LENGTH_OF_SIDE || m >= LENGTH_OF_SIDE || p >= LENGTH_OF_SIDE) {
            return splitAndMerge(matrixA, matrixB);
        } else {
            return MultiplicationMatrixSingleThread.multiplicationMatrices(matrixA, matrixB);
        }
    } catch (InterruptedException | ExecutionException e) {
        e.printStackTrace();
        return null;
    }
}

public int[][] splitAndMerge(int[][] matrixA, int[][] matrixB) throws ExecutionException,
        InterruptedException {
    ExecutorService service = Executors.newCachedThreadPool();
    int n = matrixA.length;
    int m = matrixA[0].length;
    int k = matrixB[0].length;
    int bound = n / taskCount;
    int start;
    int end;
    Future<int[][]> future;
    List<Future<int[][]>> matrices = new ArrayList<>();
    for (int task = 0; task < taskCount; task++) {
        start = task * bound;
        end = (task + 1) * bound;
        future = service.submit(new MultiplicationSubMatrixN(matrixA, matrixB, start, end));
        matrices.add(future);
    }
    int[][] result = new int[n][k];
    int[][] currentMatrix;
    for (int st = 0; st < taskCount; st++) {
        currentMatrix = matrices.get(st).get();
        System.arraycopy(currentMatrix, 0, result, st * bound, currentMatrix.length);
    }
    if (n % taskCount != 0) {
        int begin = n - n % taskCount;
        currentMatrix = new int[n % taskCount][m];
        for (int i = 0; i < currentMatrix.length; i++) {
            System.arraycopy(matrixA[i + begin], 0, currentMatrix[i], 0, currentMatrix[0].length);
        }
        currentMatrix = MultiplicationMatrixSingleThread.multiplicationMatrices(currentMatrix, matrixB);
        System.arraycopy(currentMatrix, 0, result, begin, currentMatrix.length);
    }
    service.shutdown();
    return result;
}

private class MultiplicationSubMatrixN implements Callable<int[][]> {
    private int[][] matrixA;
    private int[][] matrixB;
    private int start;
    private int end;

    private MultiplicationSubMatrixN(int[][] matrixA, int[][] matrixB, int start, int end) {
        this.matrixA = matrixA;
        this.matrixB = matrixB;
        this.start = start;
        this.end = end;
    }

    @Override
    public int[][] call() throws Exception {
        int n = matrixA.length;
        int m = matrixA[0].length;
        int k = matrixB[0].length;
        int[][] matrixC = new int[n / taskCount][k];
        for (int i = start; i < end; i++) {
            for (int j = 0; j < k; j++) {
                for (int l = 0; l < m; l++) {
                    matrixC[i - start][j] += matrixA[i][l] * matrixB[l][j];
                }
            }
        }
        return matrixC;
    }
}
}

Let's start with the simplification changes:

Declare variables and fields as final wherever possible. This drastically simplifies reasoning about mutation behaviour in your code. Since matrixA and matrixB are not supposed to change for the lifetime of the Object, they should be declared final.
Make use of caching when it makes sense: Currently your code will always recompute the result if the call method is executed, regardless of whether it has already been or not. That's basically a waste of computing power you could prevent by just computing the result once and allowing future calls to just lookup that result.
Keep the scope of variables as small as possible. Double check that introducing a variable actually makes sense before doing that. The code currently declares almost all variables it uses upfront. That reminds me of old vba programs. In either case: it's easier for you (and the runtime) when variables are in the smallest scope possible

future, start and end can be declared inside the respective loop body. in call you only use n, m and k to decide whether you splitAndMerge or run the calculation on a single thread. Those variables are basically just fluff. Inline them to prevent confusion when reading the code.
You seem to have an aversion to whitespace. It's hard to subidivide the code into logical sections because there's no distinction between blocks. This makes grasping the code significantly harder.

There's a large possibility for simplification in your design. Currently the code attempts to break the problem into "subresult" computations by "tasks". The problem I see with this is that matrices make this harder than it needs to be. To compute the result at \$(i,j)\$ you need the left hand sides \$i\$th row and the right hand sides \$j\$th column.
Since it's a bit unclear to me how the matrices are organized in your code, I'll not get into the details, but I agree with oopexpert when he comments:

  If that is the case and there is nothing special about the matrix multiplication I would follow a totally other approach. I would forget about any thread handling and formulate the problem in a way that I am able to use parallel streams

Now to make myself sufficiently clear: I'm not proposing you reformulate this as a solution using Streams. I haven't quite found a way to make that look nice. Instead I'd strive to calculate each entry independently. This will allow you to trivially control how much CPU-power you want to throw at the problem (instead of this somewhat messy solution).

I think you could have a good chance at reformulating this using a simplification. Since you only read from the input matrices and result matrix entries are fully independent of one another you can have great success enqueueing a separate Task for each entry that task only needs matrixA, matrixB as well as a row and column to perform it's duties. You can control the level of parallelism by setting a limit on how many Threads your executor pool may use.

Overall that solution strikes me as a heavy simplification over the current code, because it doesn't rely on subdividing the matrices into submatrices. That makes it much more understandable: one task for one result.


Super simple spam program x = int(input("Choose the number of times you want to spam. "))

y = input("What message do you want to spam? ")

w = x + 1

for i in range(1,w):
    print(y)

Any way to make this program better? I'm currently learning Python.

Use descriptive variable names
No need to add one to x and then use range(1, w)  - get used to using zero-based numbering
By convention in Python you can use _ as the name for a loop variable when you won't actually be referring to that variable anywhere.
As you develop this into a full programme, structure code under functions.
use blank lines sparingly

So:

def main():
    number_of_times = int(input("Choose the number of times you want to spam. "))
    message = input("What message do you want to spam? ")
    for _ in range(number_of_times):
        print(message)

Alternatively:

print((message + '\n') * number_of_times)


Sorting a numbered table of contents and the contents associated with it Given a numbered table of contents with headers and the content of each section, I wanted to sort them correctly according to only the numeric value.

The first challenge: section numbers can be tricky to sort (i.e. sorted(["1.1.1", "1.1.2", "1.1.10"]) results in: ["1.1.1", "1.1.10", "1.1.2"] which is alphabetically correct, but not what I want).

Thankfully, this problem is already solved here.

Using similar logic, I want to make an OrderedDict that has for key a list of headers (where each header is a string concatenation of the section number and its title) and for value the contents of each section.

I came up with the following solution, but I want some feedback, because it seems convoluted (although it seems to be doing what I want):

from collections import OrderedDict
import re

headers = ['4.2.10 Context 4', '4.2.11 Context 5', '4.2.0 Context 1', '4.2.1 Context 2', '4.2.2 Context 3']
sections = ['C4', 'C5', 'C1', 'C2', 'C3']

def section_sort(t):
    section = t[0]
    numbering_pattern = re.compile('\d.\d[.\d]*')

    if numbering_pattern.match(section.split(' ')[0]):
        s_nbr = section.split(' ')[0]
        return [int(_) for _ in s_nbr.split('.')]

contents = OrderedDict(sorted(zip(headers, sections), key=section_sort))

for k, v in contents.items():
    print('{header}\n\t{section}'.format(header=k, section=v))

Output:

4.2.0 Context 1
  C1
4.2.1 Context 2
  C2
4.2.2 Context 3
  C3
4.2.10 Context 4
  C4
4.2.11 Context 5
  C5

What's your take on this?

Just a performance tip. The function section_sort is called multiples times and you are compiling the RegEx each time. Also you are using section.split() twice in the function to get the same value. You can save these values in vars. The code would be like this:

from collections import OrderedDict
import re

headers = ['4.2.10 Context 4', '4.2.11 Context 5', '4.2.0 Context 1', '4.2.1 Context 2', '4.2.2 Context 3']
sections = ['C4', 'C5', 'C1', 'C2', 'C3']
numbering_pattern = re.compile('\d.\d[.\d]*')

def section_sort(t):
    section = t[0]

    snbr = section.split(' ')[0]
    if numbering_pattern.match(snbr):
        return [int(_) for _ in s_nbr.split('.')]

contents = OrderedDict(sorted(zip(headers, sections), key=section_sort))

for k, v in contents.items():
    print('{header}\n\t{section}'.format(header=k, section=v))


Implementation of Graham Scan algorithm in Haskell Here is an implementation of Graham Scan algorithm for computing the convex hull of a finite set of points:

data Point = Point {
  xcoord :: Double,
  ycoord :: Double
} deriving (Show)

data Direction = ToRight | Straight | ToLeft  deriving (Show, Eq)

distance :: Point -> Point -> Double
distance a b = sqrt (((xcoord a) - (xcoord b))^2 + ((ycoord a) - (ycoord b))^2)

-- The direction function can be simplified after some algebraic analysis:
direction :: Point -> Point -> Point -> Direction
direction a b c
  | s < 0 = ToRight
  | s == 0 = Straight
  | s > 0 = ToLeft
  where
    s = ((xcoord c) - (xcoord a)) * ((ycoord a) - (ycoord b)) + ((ycoord c) - (ycoord a)) * ((xcoord b) - (xcoord a))

directions_ :: [Point] -> [Direction]
directions_ [] = []
directions_ [_] = []
directions_ [_, _] = []
directions_ (a:b:c:xs) = (direction a b c) : (directions_ (b:c:xs))

directions :: [Point] -> [Direction]
directions xs = ToLeft : ToLeft : (directions_ xs)

lowestYPoint :: [Point] -> Point
lowestYPoint [x] = x
lowestYPoint (x:xs)
  | (ycoord x) <= (ycoord lowestRest) = x
  | otherwise = lowestRest
  where
    lowestRest = lowestYPoint xs

cosineAB :: Point -> Point -> Double
cosineAB a b = ((xcoord b) - (xcoord a)) / (distance a b)

sortByAngle :: Point -> [Point] -> [Point]
sortByAngle x xs = sortBy comparePoints xs where
  comparePoints :: Point -> Point -> Ordering
  comparePoints a b
    | cA > cB = LT
    | cA < cB = GT
    | otherwise = case () of
      _ | xA < xB  -> LT
        | xA > xB  -> GT
        | xA == xB -> case () of
          _ | yA < yB   -> LT
            | yA > yB   -> GT
            | yA == yB  -> EQ
    where
      yA = (ycoord a)
      yB = (ycoord b)
      xA = (xcoord a)
      xB = (xcoord b)
      cA = (cosineAB x a)
      cB = (cosineAB x b)

grahamScanDirections :: [Direction] -> [Bool]
grahamScanDirections [] = []
grahamScanDirections [_] = [True]
grahamScanDirections (ToLeft:ToRight:xs) = False : (grahamScanDirections (ToLeft:xs))
grahamScanDirections (ToLeft:ToLeft:xs)  = True  : (grahamScanDirections (ToLeft:xs))
grahamScanDirections (ToLeft:Straight:xs) = False : (grahamScanDirections (ToLeft:xs))
grahamScanDirections _ = error "Impossible scenario"

filterByList :: [Bool] -> [a] -> [a]
filterByList [] _ = []
filterByList _ [] = []
filterByList (x:xs) (y:ys)
  | x = y : (filterByList xs ys)
  | otherwise = (filterByList xs ys)

grahamScan :: [Point] -> [Point]
grahamScan ps = filterByList bool_list ps1 where
  -- sort ps by their angles (or cosine values) formed with the lowest point
  ps1 = sortByAngle (lowestYPoint ps) ps
  bool_list = grahamScanDirections $ directions ps1

Test:

*Main> let ps = [(Point 1 0), (Point 1 2), (Point 0 3), (Point 2 3), (Point 2 2), (Point 3 2), (Point 2 1)]
*Main> grahamScan ps
[Point {xcoord = 1.0, ycoord = 0.0},Point {xcoord = 3.0, ycoord = 2.0},Point {xcoord = 2.0, ycoord = 3.0},Point {xcoord = 0.0, ycoord = 3.0}]

Any suggestion about correctness/performance or any other possible improvement is appreciated.

Here are some improvements you may apply.

You can combine pattern matching and records. It allows you to avoid unnecessary numerous calls to xcoord and ycoord. It also makes your code easier to read.

You can force strictness on xcoord and ycoord fields.

These two improvements help with performance.

You can also do the following:

simplify pattern matching py reordering the patterns (see filterByList)
drop lots of parenthesis since space has bigger precedence than operators
simplify comparePoints with keeping only one level of guards
use more standard library functions (see lowestYPoint)

Most of these were given to me by hlint ;-)

import Data.List
import Data.Function (on)
import Control.DeepSeq

data Point = Point {
    xcoord :: !Double,
    ycoord :: !Double
} deriving Show

data Direction = ToRight | Straight | ToLeft  deriving (Show, Eq)

distance :: Point -> Point -> Double
distance (Point xA yA) (Point xB yB) = sqrt ((xA - xB)^2 + (yA - yB)^2)

direction :: Point -> Point -> Point -> Direction
direction (Point xA yA) (Point xB yB) (Point xC yC)
    | s <  0    = ToRight
    | s == 0    = Straight
    | otherwise = ToLeft
    where
        s = (xC - xA) * (yA - yB) + (yC - yA) * (xB - xA)

directions_ :: [Point] -> [Direction]
directions_ (a:b:c:xs) = direction a b c : directions_ (b:c:xs)
directions_ _ = []

directions :: [Point] -> [Direction]
directions xs = ToLeft : ToLeft : directions_ xs

lowestYPoint :: [Point] -> Point
lowestYPoint = minimumBy (compare `on` ycoord)

cosineAB :: Point -> Point -> Double
cosineAB a@(Point xA _) b@(Point xB _) = (xB - xA) / distance a b

comparePoints :: Point -> Point -> Point -> Ordering
comparePoints x a@(Point xA yA) b@(Point xB yB)
    | cA > cB   = LT
    | cA < cB   = GT
    | xA < xB   = LT
    | xA > xB   = GT
    | yA < yB   = LT
    | yA > yB   = GT
    | otherwise = EQ
    where
        cA = cosineAB x a
        cB = cosineAB x b

sortByAngle :: Point -> [Point] -> [Point]
sortByAngle x = sortBy (comparePoints x)

grahamScanDirections :: [Direction] -> [Bool]
grahamScanDirections [] = []
grahamScanDirections [_] = [True]
grahamScanDirections (ToLeft:ToRight:xs) = False : grahamScanDirections (ToLeft:xs)
grahamScanDirections (ToLeft:ToLeft:xs)  = True  : grahamScanDirections (ToLeft:xs)
grahamScanDirections (ToLeft:Straight:xs) = False : grahamScanDirections (ToLeft:xs)
grahamScanDirections _ = error "Impossible scenario"

filterByList :: [Bool] -> [a] -> [a]
filterByList (True :xs) (y:ys) = y : filterByList xs ys
filterByList (False:xs) (_:ys) = filterByList xs ys
filterByList _ _               = []

grahamScan :: [Point] -> [Point]
grahamScan ps = filterByList bool_list ps1
    where ps1       = sortByAngle (lowestYPoint ps) ps
          bool_list = grahamScanDirections $ directions ps1

pss = replicate 1000000
                [ Point 1 0
                , Point 1 2
                , Point 0 3
                , Point 2 3
                , Point 2 2
                , Point 3 2
                , Point 2 1
                ]

instance NFData Point where
    rnf a = a `seq` ()

main = (map grahamScan pss) `deepseq` putStrLn "Done"


Showing a table for open and closed tickets This shows a table for open and closed tickets. The code for open tickets is identical with the closed tickets. I was trying to figure out a way to just write one code segment that will show both tables and use half of the code lines. With some changes I ended up to this code. With this code, if I iterate it twice and at at first use the word "open" and then rename it to "closed" then I will accomplish what I am after.

I looked around and figured out that I need to use a dynamic variable name. Something like sum#{status}tickets (pretty sure that something like that doesn't work) where the "status" variable will be at the first iteration "open" and then "closed" at the second one.

I was wondering if this is somehow possible (to iterate twice) and then dynamically set the variable's name.

   <% if sumopentickets == 0 %>

Open Tickets

No open tickets.

  <% else %>

Open Tickets

      Ticket Number
      Title
      Submitted by
      Created at
      Status

    <% @tickets.each do |ticket| %>
    <% if ticket.status == "open" %>
    <% user = User.find(ticket.user_id) %>

        <%= ticket.id %>
        <%= link_to ticket.title, ticket_path(ticket.url_token) %>
        <%= user.username %>
        <%= ticket.created_at.strftime("%H:%M, %B %d, %Y") %>
        <% if ticket.ticketmessages.last && ticket.ticketmessages.last.user_id %>
        (user waiting for reply...)
        <% end %>

    <% end %>
    <% end %>

  <% end %>

  <% if sumclosedtickets == 0 %>

Closed Tickets

No closed tickets.

  <% else %>

Closed Tickets

      Ticket Number
      Title
      Submitted by
      Created at
      Status

    <% @tickets.each do |ticket| %>
    <% if ticket.status == "closed" %>
      <% user = User.find(ticket.user_id) %>

        <%= ticket.id %>
        <%= link_to ticket.title, ticket_path(ticket.url_token) %>
        <%= user.username %>
        <%= ticket.created_at.strftime("%H:%M, %B %d, %Y") %>
        <% if ticket.ticketmessages.last && ticket.ticketmessages.last.user_id %>
        (user waiting for reply...)
        <% end %>

    <% end %>
    <% end %>

  <% end %>

I'm going to assume that this is in fact a ruby-on-rails project - not just plain Ruby.

In that case, long story short: Stop doing everything in the view. Use Rails and its conventions.

In your model, add a proper relationship to the user, and a couple of scopes

belongs_to :user

scope :open, where(status: :open)
scope :closed, where(status: :closed)

Also add a method

def waiting_for_reply?
  # I don't quite understand your logic (or your naming) here,
  # so I'll just shorten it, but otherwise leave it
  ticketmessages.last.try(:user_id).present?
end

I say I don't understand your logic, because something called ticketmessages implies there's a class called Ticketmessage - when it should be called TicketMessage as it's two words (or simply Message). And in that case, the relation should be ticket_messages with an underscore (or, again, simply messages).
Secondly, you check whether there is a (last) ticket, and whether its user_id is non-nil/non-false. But what does that prove? If there's any system to how it's set up, a message will always have a user_id because every message was written by a user. The real question (I assume) is whether that user_id is equal to the ticket's user_id - otherwise someone else wrote it. But that's not what you're checking, so I'm confused.

Anyway, in your controller, load two collections (and eagerly load the associated users):

@open_tickets = Ticket.open.includes(:user)
@closed_tickets = Ticket.closed.includes(:user)

In your view, extract the table itself into a partial, and only include HTML for the parts that actually differ. Note the call to render where we point it to the partial template it should use, and the local variables that should be made available to that template.

Open Tickets

<% if @open_tickets.any? %>
  <%= render partial: "tickets", locals: { tickets: @open_tickets } %>
<% else %>

No open tickets

<% end %>

Closed Tickets

<% if @closed_tickets.any? %>
  <%= render partial: "tickets", locals: { tickets: @closed_tickets } %>
<% else %>

No closed tickets

<% end %>

And here's the partial

      Ticket Number
      Title
      Submitted by
      Created at
      Status

  <% tickets.each do |ticket| %>

      <%= ticket.id %>
      <%= link_to ticket.title, ticket_path(ticket.url_token) %>
      <%= ticket.user.username %>
      <%= ticket.created_at.strftime("%H:%M, %B %d, %Y") %>

      <% if ticket.waiting_for_reply? %>
        (user waiting for reply...)
      <% end %>

  <% end %>


String case reverse function in C This reverses the case of a character in a string (a string being an array of characters). How does it look? Anything I can improve on? Besides documentation, in particular, any thoughts on the type casting found in str_case_rev?

#include <stdio.h>  // fprintf
#include <stdlib.h> // malloc
#include <string.h> // strlen
#include <ctype.h>  // toupper, tolower

/*
 * a string is an array of characters, in C, all arrays
 * are always passed never passed value, always a pointer to
 * the variable
 * This is why the caller does not need to call the function like:
 * camel_case_rev(src, &dest, len)
 *
 * Since here the array of characters ("a string") is already being
 * passed as a pointer value
 */
void str_case_rev(const char *src, char *dest, size_t len)
{
        size_t i = 0;

        for(; i < len; i++)
        {
                if(src[i] <= 'Z' && src[i] >= 'A')
                {
                        *(dest + i) = (char)tolower((int)src[i]);
                }
                else if(src[i] <= 'z' && src[i] >= 'a')
                {
                        *(dest + i) = (char)toupper((int)src[i]);
                }
                else
                {
                        *(dest + i) = src[i];
                }
        }

        i++;
        *(dest + i) = '\0';
}

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

        char *dest = NULL;

        size_t len = strlen(argv[1]);

        dest = malloc(len + 1);

        if(NULL == dest)
        {
                fprintf(stderr, "Memory error\n");
                return EXIT_FAILURE;
        }

        str_case_rev(argv[1], dest, len);

        fprintf(stdout, "%s\n", dest);

        free(dest);

        return EXIT_SUCCESS;
}

The case determination code (src[i] <= 'Z' && src[i] >= 'A') doesn't look right (think about locales). isupper and islower are specifically designed to deal with that. Besides, you are already using toupper and tolower.


Calculating the ideal body weight The following question was taken from Absolute Java 5th ed. by Walter Savitch:

  A simple rule to estimate your ideal body weight is to allow 110 pounds for the first 5 feet of height and 5 pounds for each additional inch. Write a program with a variable for the height of a person in feet and another variable for the additional inches. Assume the person is at least 5 feet tall. For example, a person that is 6 feet and 3 inches tall would be represented with a variable that stores the number 6 and another variable that stores the number 3. Based on these values, calculate and output the ideal body weight.

This is the code that I have written:

 public class Question9 {

    private static final int INCH_PER_FEET = 12;
    private static final int POUND_PER_INCH = 5;
    private static final int MIN_HEIGHT_FEET = 5;
    private static final int MIN_WEIGHT_POUND = 110;

    public static void main(String[] args) {

        int feet = 6;
        int inches = 3;

        System.out.println(idealBodyWeight(feet, inches) + " pounds");
    }

    private static int idealBodyWeight(int feet, int inches) {
        int pounds = MIN_WEIGHT_POUND;

        if ((feet > MIN_HEIGHT_FEET)) {

            inches += (feet - MIN_HEIGHT_FEET) * INCH_PER_FEET;
        }

        pounds += inches * POUND_PER_INCH;

        return pounds;
    }
}

There isn't really all that much to say, your code is generally good.

Declare variables where they are needed: you do not use pounds in the if statement, so I would remove it and just use MIN_WEIGHT_POUND in the return directly. Alternatively, you could move the declaration of pounds to after the if.
Try to minimize white space: It's good to sort code by using newlines, but too much isn't good either. remove the newline after the if statement, and maybe some more.
Try to minimize use of brackets: No need to wrap the boolean expression in brackets.

With these changes, your code would look like this:

private static int idealBodyWeight(int feet, int inches) {
    if (feet > MIN_HEIGHT_FEET) {
        inches += (feet - MIN_HEIGHT_FEET) * INCH_PER_FEET;
    }
    return MIN_WEIGHT_POUND + inches * POUND_PER_INCH;
}


Repository code in Laravel I am practicing to code in the right way and make it testable. Please review this code and tell me anything bad practices of it.

Entity:

namespace MockBlog\Entities;

use Doctrine\ORM\Mapping as ORM;
//use Doctrine\Common\Collections\ArrayCollection;

/**
* @ORM\Entity
* @ORM\Table(name="mockblog_page")
*/
class Page {

/**
 * @ORM\Id
 * @ORM\GeneratedValue
 * @ORM\Column(type="integer")
 */
protected $id;

/**
 * @ORM\Column(type="string")
 */
protected $title;

/**
 * @ORM\Column(type="string")
 */
protected $slug;

/**
 * @param $title
 */
public function setTitle($title)
{
    $this->title = $title;
}

/**
 * @param $slug
 */
public function setSlug($slug)
{
    $this->slug = $slug;
}

/**
 * @return integer
 */
public function getId()
{
    return $this->id;
}

/**
 * @return string
 */
public function getTitle()
{
    return $this->title;
}

/**
 * get url of the page
 *
 * @return string
 */
public function getSlug()
{
    return $this->slug;
}
}

Interface post repository:

interface PostRepository {

/**
 * Find post by the title
 *
 * @param $title
 * @return mixed
 */
public function findByTitle($title);

/**
 * Find post by the url
 *
 * @param $slug
 * @return mixed
 */
public function findBySlug($slug);

/**
 * Find post by user id
 *
 * @param $userId
 * @return mixed
 */
public function findByUserId($userId);

}

Base repository (still blank but i think i will need it later):

class BaseRepository extends EntityRepository  {

}

Base post repository :

abstract class BasePostRepository extends BaseRepository implements PostRepository {

abstract public function save($data, $id);

public function findBySlug($slug)
{
    return $this->findBy(['slug' => $slug]);
}

public function findByTitle($title)
{
    return $this->findBy(['title' => $title]);
}

public function findByUserId($userId)
{
    return $this->findBy(['user_id' => $userId]);
}

}

Doctrine page repository:

class DoctrinePageRepository extends BasePostRepository  {

public function save($data, $id = 0)
{

    $entity = (empty($id)) ? $this->_class : $this->find($id);

    $entity->setTitle($data['title']);
    $entity->setSlug($data['slug']);

    try{

        EntityManager::persist($entity);
        EntityManager::flush();

        return true;

    } catch (Exception $err){

        return FALSE;

    }
}

}

Service provider:

public function register()
{
    // TODO: Implement register() method.

    $this->app->bind(DoctrinePageRepository::class, function($app){
        return new DoctrinePageRepository(
            $app['em'], $app['em']->getClassMetaData(Page::class)
        );
    });

}

You can merge the methods findBySlug, findByTitle and other findBy methods into one method that accepts parameter field and value

$this->findBy($field, $title);

And move it in BaseRepository Class. See this article Repository Pattern

so it would look something like this

public function findBy( $field, $value ) {
  return $this->model->where($field, $value);
}


Converting JSONP to JSON: Is this Regex correct? I'm fetching a string that is JSONP, and looking to convert it to JSON.

I'm using the following regular expression for matching (for removal) the padding on the JSON.

([a-zA-Z_0-9\.]*\()|(\);?$)

In python, here's the line I'm using for converting the JSONP to JSON:

apijson = re.sub(r'([a-zA-Z_0-9\.]*\()|(\);?$)','',jsonp)

Is there anything I should worry about using this regular expression? Any chance I could end up mangling the JSON?

The problem with the code is that it doesn't really express what you are trying to do:

apijson = re.sub(r'([a-zA-Z_0-9\.]*\()|(\);?$)','',jsonp)

The code would seem to indicate that you are trying to find and replace many instances of some regular expressions inside the string. However, you are really trying to strip parts off of the beginning and end. I'm also not a huge fan of regular expressions because they are usually pretty dense and hard to read. Sometimes they are awesome, but this is not one of those times.

Additionally, you aren't anchoring the regular expression to the beginning of the string which is what you'd need to strip off. The only case I could see that being a problem is perhaps if there were strings inside the json which matched the regular expression. Its best to be sure.

Also, I think JSON-P allows functions like alpha["beta"] which will doesn't fit your regular expression. Also what about additional whitespace or comments?

I would suggest doing something like:

   apijson = jsonp[ jsonp.index("(") + 1 : jsonp.rindex(")") ]

That way you are more clearly stripping everything outside of the first and last parenthesis.


Ensure that a Sinatra hash always has a valid value I'm trying to ensure that a Sinatra hash always has a valid value.

Is there a more-concise way to write this?

params[:v] = if [:icons,:list].include? (params[:v] ||= :list).downcase.to_sym then (params[:v] ||= :list).downcase.to_sym else :list end

I'd write this in multiple lines, so that you don't have to write out the complex conversion multiple times:

v = (params[:v] || :list).downcase.to_sym
params[:v] = if %i[icons list].include?(v) then v else :list end


Sending a HttpRequest Message, and retrying in case of unauthorised This code simply sends a HttpRequest message and, if it comes back Unauthorized, gets a new login and tries again.

This code is meant to deal with the moment a JSON Web Token goes out of date. In this system the client is notified of this by having its request become Unauthorized, it then picks up a new Token by logging on again. Hiding away that Unauthorized code, it continues to message.

Two things to note:

A HttpRequestMessage has to be recreated every time it is used.
I have a need to recreate the client to pick up the new JWT token

(Edit: Actually can just go in and change some properties for client, I assume it would be worth changing?)

Basically my question is: How unpleasant is my use of delegates and what lengths should I go to change this code, if any?

private async Task<HttpResponseMessage> SendToFooAsync(Func<HttpClient> client, Func<HttpRequestMessage> requestMessage)
{
    var response = await client().SendAsync(requestMessage());

    if (response.StatusCode == HttpStatusCode.Unauthorized)
    {
        //Re-authenticate, recreate client with new token, and retry
        if (await authService.LoginAsync())
        {
            response = await client().SendAsync(requestMessage());
            Log.Log("Unauthorized code received: Re-Authentication attempt succeeded");
        }
        else
        {
            Log.Log("Unauthorized code received: Authentication attempt has failed");
        }
    }
    return response;
}

Simplest code that calls it:

HttpResponseMessage result = await SendToFooAsync(InitialiseHttpClient, () => InitialiseRequest(HttpMethod.Post, path));

Most complex code that calls it

Func<HttpClient> client = () =>
            {
                var httpClient = InitialiseHttpClient();
                httpClient.DefaultRequestHeaders.Accept.Add("some json stuff");
                return httpClient;
            };

HttpResponseMessage result = await SendToFooAsync(client, () =>
{
    var message = InitialiseRequest(HttpMethod.Post, path);
    message.Content = new StringContent("some json stuff");
    return message;
});

Where InitialiseRequest and LoginAsync are my own methods outside the scope of this review. Hopefully their use is obvious here.

How unpleasant is my use of delegates and what lengths should I go to change this code, if any?

If you want the caller to provide some small piece of code, then a delegate is exactly the right way to do it, it's not unpleasant at all.

But looking at your two usage samples, it looks like they have some code in common (calling InitialiseHttpClient and InitialiseRequest), so it might make sense to change the signature to something like:

private async Task<HttpResponseMessage> SendToFooAsync(
    HttpMethod method, string uri,
    Action<HttpClient> clientAction = null,
    Action<HttpRequestMessage> requestMessageAction = null)

The two usage samples would then look like this:

SendToFooAsync(HttpMethod.Post, path);

SendToFooAsync(
    HttpMethod.Post, path,
    client => client.DefaultRequestHeaders.Accept.Add("some json stuff"),
    message => message.Content = new StringContent("some json stuff"));

Also, I don't like the way you're naming your delegates. Func<HttpClient> is not HttpClient, so it shouldn't be called just client. Something like clientFunc would be more accurate.


Highest andLowest in PLSQL I have made a procedure to display the highest and the lowest popular Items for a particular time of a given date. The procedure works with no errors or exceptions and everything is functional. As you can see, the queries are repeated twice for the purpose of showing the first record on Items, but the only differences are in Order (ASC and DESC).

Is there any way I can reduce the amount of the code? How can I show the highest and lowest Items in one query rather than two? I only want to make the code neat and easy to read.

      create or replace procedure hight_lowest (param in date)
    as
    V_PNO_LOW   number(5);
    V_PNO_HIGH  number(5);
    BEGIN
    SELECT Item_no INTO V_PNO_LOW
    FROM
    (SELECT Items.Item_no SUM(Items.Quantity) AS total,
   TO_CHAR(Prodcution_d,
   'dd-mm-yyyy') AS pro_date
   FROM Items
   JOIN Parts
   ON Parts.Serial_no = Items.Serial_no
   GROUP BY Item_no, TO_CHAR(Prodcution_d, 'dd-mm-yyyy')
   ORDER BY SUM(Items.Quantity) ASC)
   WHERE ROWNUM = 1
   AND pro_date = TO_CHAR(Param_DATE,'mm-yyyy') ;

   DBMS_OUTPUT.PUT_LINE('LOWEST ITEM: ' || V_PNO_LOW);

  SELECT Item_no INTO V_PNO_LOW
  FROM
  (SELECT Items.Item_no SUM(Items.Quantity) AS total, TO_CHAR(Prodcution_d,
  'dd-mm-yyyy') AS pro_date
  FROM Items
  JOIN Parts
  ON Parts.Serial_no = Items.Serial_no
  GROUP BY Item_no, TO_CHAR(Prodcution_d, 'dd-mm-yyyy')
  ORDER BY SUM(Items.Quantity) DESC)
  WHERE ROWNUM = 1
  AND pro_date = TO_CHAR(Param_DATE,'mm-yyyy') ;

 DBMS_OUTPUT.PUT_LINE('HIGHEST POPULAR ITEM: ' || V_PNO_HIGH);

 END;
  /

Your core problem is how to find out top-1 and bottom-1 in a single query. This is essentially a SQL problem and has not much to do with PL/SQL. First I present you the SQL solution and then simply wrap that into PL/SQL.

You didn't provided table definitions. I was lazy and didn't tried to reverse engineer your schema but instead created very simple table that illustrates the solution that you can apply to your real problem.

Other notes that might or might not be relevant to your case:

Use packages when applicable.
PL/SQL is case insensitive so it doesn't matter if use UPPER or lower or MiXed case. I prefer simplicity so I always use lower case. YMMV.
There is no need to convert dates into strings. Instead if you are looking for day granularity use trunc instead of to_char.

trunc example:

SQL> select sysdate, trunc(sysdate), date'2016-01-15' from dual;

SYSDATE             TRUNC(SYSDATE)      DATE'2016-01-15'
------------------- ------------------- -------------------
2016-01-15 11:05:10 2016-01-15 00:00:00 2016-01-15 00:00:00

Elapsed: 00:00:00.16
SQL>

As bonus I also introduced you to standard SQL datetime-literal syntax.

Let's create some data to play with:

-- demonstration purpose only, no resemblance to OP's code
create table items(
 id number
,quantity number
,production_date date
);

-- populate random data
insert into items
select level, floor(dbms_random.value(1, 1000)), sysdate - floor(dbms_random.value(1, 5))
from dual
connect by level <= 1000
;

Use analytic function row_number (you might also consider using rank or dense_rank functions instead) to assign an unique number in ordered sequence to each row. This is the standard way to implement top-N, bottom-N and inner-N queries in Oracle. Note that this example doesn't resolve ties.

with
ordered_items as (
  -- the actual SQL can be arbitrary complex
  select
   id
  ,quantity
  ,production_date
  ,row_number() over (order by quantity asc)  as top_rn
  ,row_number() over (order by quantity desc) as bottom_rn
  from items
  where trunc(production_date) = date'2016-01-14'
)
select
 o1.id as hi_id
,o2.id as lo_id
from       ordered_items o1
inner join ordered_items o2 on o2.bottom_rn = o1.top_rn
where o1.top_rn    = 1
and   o2.bottom_rn = 1
;

The above SQL will turn into the following PL/SQL code:

create or replace procedure get_hi_and_lo_id(
 p_production_date in date
,p_hi_id out number
,p_lo_id out number
) as
begin
  with
  ordered_items as (
    select
     id
    ,quantity
    ,production_date
    ,row_number() over (order by quantity asc)  as top_rn
    ,row_number() over (order by quantity desc) as bottom_rn
    from items
    where trunc(production_date) = trunc(p_production_date)
  )
  select   hi.id,   lo.id
  into   p_hi_id, p_lo_id
  from       ordered_items hi
  inner join ordered_items lo on lo.bottom_rn = hi.top_rn
  where hi.top_rn    = 1
  and   lo.bottom_rn = 1
  ;
exception
  when no_data_found then
    null;
end;
/
show errors

Usage example:

declare
  v_hi_id number;
  v_lo_id number;
begin
  get_hi_and_lo_id(
    p_production_date => sysdate - 1
   ,p_hi_id           => v_hi_id
   ,p_lo_id           => v_lo_id
  );

  dbms_output.put_line('v_hi_id: ' || v_hi_id);
  dbms_output.put_line('v_lo_id: ' || v_lo_id);
end;
/

v_hi_id: 689
v_lo_id: 370

PL/SQL procedure successfully completed.

That seems to be the correct answer with my data:

SQL> select * from items where trunc(production_date) = trunc(sysdate - 1) order by quantity;

        ID   QUANTITY PRODUCTION_DATE
---------- ---------- -------------------
       689         13 2016-01-14 10:44:28
        25         18 2016-01-14 10:44:28
[...]
       334        987 2016-01-14 10:44:28
       370        994 2016-01-14 10:44:28

235 rows selected.

Elapsed: 00:00:00.95
SQL>


A Nightmare on setTimeout Street I have script which plays audio, shows/hides elements:

   var audio = new Audio("audio/test.mp3");

    audio.addEventListener("ended", function(){
             element.fadeIn(0);
             setTimeout(function() {
                element2.fadeIn(0);
                setTimeout(function() {
                    element3.fadeIn();
                    sprite.play(); // a one lib for playing sprites
                },1000)
             },1000)
        });

// this is just event and we look for the last frame
sprite.on("frame", function(){
        if(this.frame === this.frames - 1)
        {
            this.pause();
            setTimeout(function() {
                //here we show some another elements and play another audio
            },1000)
        }
    });

audio.play();

As you can see there are a lot of setTimeout and spaghetti code. This is very short version, I have more than 5 audio files and a lot of elements which I have to show and hide, and, it's very difficult to understand the order audio files playing and showing/hiding elements in the end. May be you know how to organize this flow, some libraries? I am a noob at promises, is it good idea here?

To generate those nested, nearly identical setTimeout calls, instead of coding them all manually you could have a recursive function that builds and returns a function for you. Something like this would work:

// Accepts an array of elements and a final callback function
// Returns a function to show all elements and execute the callback
function getFunctionToShowElements(elements, callback, currentIndex) {
    var i = typeof currentIndex === "undefined" ? 0 : currentIndex;
    if (i === elements.length) {
        // after the final element, call the callback
        return function () {
            callback();
        };
    } else {
        // for any preceding element, show the element and call a timeout
        var nextMethod = getFunctionToShowElements(elements, callback, i + 1);
        return function () {
            elements[i].fadeIn();
            setTimeout(nextMethod, 1000);
        };
    }
}

Then you could set up a function for creating audio objects and attaching the event listener to fade in the given elements.

function createAudio(filename,elements){
    var audio = new Audio(filename);
    audio.addEventListener("ended",function(){
        getFunctionToShowElements(elements,function(){sprite.play();})(); // get the function and execute it immediately
    });
    return audio;
}

At this point you'll be able to create an audio object with the desired event handler like so:

var audio = createAudio("audio/test.mp3",[element1,element2,element3]);

That's just to give you an idea of how you could go about cleaning it up.

There are other improvements you'd want to make in your final code, such as using a variable for the timeout delay, and wrapping up the relevant code sections in closures as necessary. We'd need to see the full code to identify other areas for improvement.

Proof that the concept works:

var els = document.querySelectorAll(".showme");
var sprite = {
    play: function () {
        alert("sprite.play");
    }
};
createAudio("audio/test.mp3", els);

function createAudio(filename, elements) {
    var audio = document.querySelector("button"); //new Audio(filename);
    audio.addEventListener("click", function () {
        getFunctionToShowElements(elements, function () {
            sprite.play();
        })();
    });
}

function getFunctionToShowElements(elements, callback, currentIndex) {
    var i = typeof currentIndex == "undefined" ? 0 : currentIndex;
    if (i === elements.length) {

        return function () {
            callback();
        };
    } else {

        var nextMethod = getFunctionToShowElements(elements, callback, i + 1);
        return function () {
            elements[i].style.display = "inherit";
            setTimeout(nextMethod, 1000);
        };
    }
}
.showme {
    background-color:red;
    display:none;
}
<button>Click me</button>

Number 1

Number 2

Number 3

Number 4