Refactoring a class / map of query parameters that are passed between layers There is an interface:

interface JournalCreator {

    @Async
    Journal create(Parameters params);
}

And many different implementations:

class CarsJournal implements JournalCreator {

    @Autowired
    private JournalRepo journalRepo;

    @Override
    public Journal create(Parameters params) {
        String name = params.getName();
        long id = params.getId();
        // do something with name / id
        // ...
        return journalRepo.getCars(name, id);
    }
}

class MotoJournal implements JournalCreator {

    @Autowired
    private JournalRepo journalRepo;

    @Override
    public Journal create(Parameters params) {
        return journalRepo.getMoto(params.getName(),
                                   params.getId(),
                                   params.getAge(),
                                   params.getColor());
    }
}

The problem is that Parameters class acts like a container for all possible parameters that could be passed to different methods in different controllers:

class Parameters {

    private String name;
    private long id;
    private int age;
    private Color color;

    // dozens of other fields
    // getters and setters
}

One of many controllers example:

@RestController
class CarsController {

    private JournalCreator carsJCreator;

    @Autowired
    public JournalController(JournalCreator carsJCreator) {
        this.carsJCreator = carsJCreator;
    }

    @RequestMapping("...")
    public Journal createCarsJournal(@RequestBody Parameters params) {
        return carsJCreator.create(params);
    }
}

The Parameters class became some kind of a god-object that holds too may different parameters. Also, this is inefficient in terms of memory, because most often only several parameters are used to be passed between different layers.

There could be the Map<String, Object> params instead of Parameters, but I also don't like this idea too much, because it requires a bunch of casts (and it's a bad practice to use an Object as a generic type).

Are there any other alternatives of how to deal with such variations of parameters?

It's cleaner and possible to create types of Parameter class, such as CarParameter, MotoParameter, and use Spring type conversion.

First your signature would become something like

@RequestMapping(".../car/...")
public Journal createCarsJournal(@RequestParam CarParameter params) {
    return carsJCreator.create(params);
}

@RequestMapping(".../moto/...")
public Journal createMotosJournal(@RequestParam MotoParameter params) {
    return motosJCreator.create(params);
}

To build the customized conversion, check out Guide to Spring Type Conversions.


Authentication with JWT Please review my code for 'JWT' authentication.

Are there any security issues?
Where should I store the secret's key, DB or InMemory?
What's a good 'JWT' Lifetime?
Should I send the 'JWT' in Header for every request?
What should be done when a 'JWT' Expires?

Token Manager

using System;
using System.Security.Cryptography;
using System.Text;
using Newtonsoft.Json;
using Procoor_V4_Model.SecurityModel;

namespace Procoor_V4_Security
{
    public class TokenManager
    {
        public static string EncodeToken(JwtPayload jwtPayload, string secret)
        {
            const string algorithm = "HS256";

            var header = new JwtHeader
            {
                Typ = "JWT",
                Alg = algorithm
            };

            var jwt = Base64Encode(JsonConvert.SerializeObject(header)) + "." + Base64Encode(JsonConvert.SerializeObject(jwtPayload));

            jwt += "." + Sign(jwt, secret);

            return jwt;
        }

        public static JwtPayload DecodeToken(string token, string secret)
        {
            var segments = token.Split('.');

            if(segments.Length != 3)
                throw new Exception("Token structure is incorrect!");

            JwtHeader header = JsonConvert.DeserializeObject(Encoding.UTF8.GetString(Base64Decode(segments[0])), typeof(JwtHeader));
            JwtPayload jwtPayload = JsonConvert.DeserializeObject(Encoding.UTF8.GetString(Base64Decode(segments[1])), typeof(JwtPayload));

            var rawSignature = segments[0] + '.' + segments[1];

            if(!Verify(rawSignature, secret, segments[2]))
                throw new Exception("Verification Failed");

            return jwtPayload;
        }

        private static bool Verify(string rawSignature, string secret, string signature)
        {
            return signature == Sign(rawSignature, secret);
        }

        private static string Sign(string str, string key)
        {
            var encoding = new ASCIIEncoding();

            byte[] signature;

            using (var crypto = new HMACSHA256(encoding.GetBytes(key)))
            {
                signature = crypto.ComputeHash(encoding.GetBytes(str));
            }

            return Base64Encode(signature);
        }

        public static string Base64Encode(dynamic obj)
        {
            Type strType = obj.GetType();

            var base64EncodedValue = Convert.ToBase64String(strType.Name.ToLower() == "string" ? Encoding.UTF8.GetBytes(obj) : obj);

            return base64EncodedValue;
        }

        public static dynamic Base64Decode(string str)
        {
            var base64DecodedValue = Convert.FromBase64String(str);

            return base64DecodedValue;
        }
    }
}

How I currently use it:

var secret = TokenManager.Base64Encode(SecurityConstants.KeyForHmacSha256);
var currentTime = (long)(DateTime.Now - new DateTime(1970, 1, 1, 0, 0, 0, 0).ToLocalTime()).TotalSeconds;
var payload = new JwtPayload
{
    iss = SecurityConstants.TokenIssuer,
    sub = "46897484",
    iat = currentTime,
    exp = currentTime + 1800
};

var jwt = TokenManager.CreateJwtToken(payload, secret);

Are there any security issues? Where should I store the secret's key,
  DB or InMemory? What's a good 'JWT' Lifetime? Should I send the 'JWT'
  in Header for every request? What should be done when a 'JWT' Expires?

The contents are merely base64 encoded and thus are simple to decode -- so don't encode your valued customers personal data.

cut and paste your token in here as evidence
https://developers.google.com/wallet/digital/docs/jwtdecoder

the secret key is not a session one so in memory makes no sense. buy an oracle license and then stick it in there. or theres nothing wrong with something nice and simple like this.

    const string algorithm = "HS256";

if you're logging a user onto a site. 1 hour? its possible to copy/paste the token and gain access to data. so it depends on the nature of the data.
yes. its an awesome method to send to a REST server instead of some internal userid that never expires
kick them out of your site. make them relog in and get a new token (with another hour long token)


Retaining and passing data between Activities I'm making an Android app where users can create posts. This process of creating a new post  can potentially be a few activities/screens, so I need a way to save and pass the data between the activities. My first (bad) solution was just to pass the data with the intent in a Bundle, but this soon got very messy as I really needed to pass a Post object between the activities. I switched from that solution to using a Singleton class called, DataHolder.

In every activity, I call DataHolder.getInstance() and when the create post button is clicked, I create a new Post object by executing: DataHolder.getInstance().setPost(new Post()). In the following activities, as the user enters more information about a post, I set the Post's properties.

This was all working well until I ran into this issue. When I would return to my app (presumably it had been killed) I would get a NullPointerException because the Post object was null. I was looking through the Android docs on passing data between activites/services and needed a little help.

Should I be using the Singleton class pattern here?
What would be the most efficient and easiest way to pass this Post object between the activities?
Should I use Parcelable? (I don't like it though because doesn't it violate the single responsibility principle?)

Here is the DataHolder class:

import com.my_name.app_name.models.Post;

public class DataHolder {

    private static final DataHolder instance = new DataHolder();

    private Post post;
    private Venue venue;
    private int mediaType;
    private Uri imageUri;
    private Uri videoUri;

    // define the media types for a new post here
    public static final int PHOTO = 0;
    public static final int VIDEO = 1;
    public static final int TEXT = 2;
    public static final int PLACE = 3;

    public Uri getImageUri() {
        return imageUri;
    }

    public void setImageUri(Uri iu) {
        this.imageUri = iu;
    }

    public Uri getVideoUri() {
        return videoUri;
    }

    public void setVideoUri(Uri vu) {
        this.videoUri = vu;
    }

    public int getMediaType() {
        return mediaType;
    }

    public void setMediaType(int mt) {
        this.mediaType = mt;
    }

    public Venue getVenue() {
        return venue;
    }

    public void setVenue(Venue v) {
        this.venue = v;
    }

    public Post getPost() {
        return post;
    }

    public void setPost(Post p) {
        this.post = p;
    }

    public static DataHolder getInstance() {
        return instance;
    }
}

I would appreciate any feedback. Let me know if you would like to see more or have any questions.

Fixing a NullPointerException is not under the scope of Code Review, but you should be able to solve that problem by looking at a previous answer of mine where I'm describing How to make a singleton in Android. This might fix your problem. (The short answer is: Use the built-in Application class that Android manages for you)

I have some other comments regarding your questions:

Avoid the Singleton pattern in Android everywhere.
I personally haven't used Parcelable, but I believe it is the recommended approach. If you think implementing it would violate Single Responsibility Principle, then create a delegating wrapper object that implements Parcelable and just more or less stores a reference to the real object.
Or serialize things using the Jackson Library (I wouldn't do this for the simple task of passing data between activities though)
Another alternative for you is to use Fragments. By using fragments properly, you could keep the Post state in your activity, and let the Fragments use a callback on your activity to save parts of the post step-by-step, until it is finally finished in which case you perform the final save of it.

As for the rest of the code in your question, I got just a few comments:

DataHolder is a too generic name. What kind of data is it holding? Sooner or later you might end up with multiple DataHolders.
What if your mediaType would accidentally end up with the value 4? Use an enum to make sure that doesn't happen. And to make your code better. Limited alternatives should always point you to an enum.

public enum MediaType {
    PHOTO, VIDEO, TEXT, PLACE;
}


Tabulate k = 2^n minimizing multiplication Given the following problem:

;3.3 Tabulate the function k = 2^n for n = 1..50.
;Do this for the fewest possible multiplications.[3]

I wrote this answer:

(defun k (n) (ash 2 (1- n)))

(loop for n from 1 to 50 do
      (format t "k(~a) = ~a ~%" n (k n)))

What do you think?

If you use 1 instead of 2 as the number to shift, you won't have to subtract one from n. I.e. you can just define k as (ash 1 n).

Other than that your code looks fine.


Compact Braille lookup A related question got me thinking about a compact Braille dictionary.
It certainly would be easier with Int16 or string but was going for most compact.
I don't think there is a case insensitive char so went with dual entries.

Dictionary<char, byte> braile = new Dictionary<char, byte>()
                        { { 'a', 0x20 }, { 'b', 0x30 }, { 'c', 0x24 }, { 'd', 0x26 }, { 'e', 0x22 }, { 'f', 0x34 }
                        , { 'g', 0x36 }, { 'h', 0x32 }, { 'i', 0x14 }, { 'j', 0x16 }, { 'k', 0x28 }, { 'l', 0x38 }
                        , { 'm', 0x2c }, { 'n', 0x2e }, { 'o', 0x2a }, { 'p', 0x3c }, { 'q', 0x3e }, { 'r', 0x3a }
                        , { 's', 0x1c }, { 't', 0x1e }, { 'u', 0x29 }, { 'v', 0x39 }, { 'w', 0x17 }, { 'x', 0x2d }
                        , { 'y', 0x2f }, { 'z', 0x2b }
                        , { 'A', 0x20 }, { 'B', 0x30 }, { 'C', 0x24 }, { 'D', 0x26 }, { 'E', 0x22 }, { 'F', 0x34 }
                        , { 'G', 0x36 }, { 'H', 0x32 }, { 'I', 0x14 }, { 'J', 0x16 }, { 'K', 0x28 }, { 'L', 0x38 }
                        , { 'M', 0x2c }, { 'N', 0x2e }, { 'O', 0x2a }, { 'P', 0x3c }, { 'Q', 0x3e }, { 'R', 0x3a }
                        , { 'S', 0x1c }, { 'T', 0x1e }, { 'U', 0x29 }, { 'V', 0x39 }, { 'W', 0x17 }, { 'X', 0x2d }
                        , { 'Y', 0x2f }, { 'Z', 0x2b }};
foreach (KeyValuePair<char, byte> c in braile)
{
    Debug.WriteLine("{0} {1}", c.Key, c.Value.ToBraile());
}
Debug.WriteLine("{0} {1}", 'w', braile['w'].ToBraile());
Debug.WriteLine("{0} {1}", 'B', braile['B'].ToBraile());

public static class MyExtensions
{
    public static string ToBraile(this byte b)
    {
        return Convert.ToString(b, 2).PadLeft(6, '0');
    }
}

Just turning some of the comments into an answer, more specific I'm going to address these issues in your code:

No need to double the lookup table – By using char.ToLower(c) you could half the size of the lookup table.
Shift the offset by (char) 'a' – If you shift the offset by the value of a you get a zero-based index.
No need longer for the char dictionary – Since we now have a zero-based index we could simply do a lookup in a byte table. (Update: And it could be a simple array also, no need for a List<byte>)
Since we shifted the offset, no need to include empty entries – By shifting the offset, we don't need a sparse 256 byte array. It does however, fail miserably like your code if trying to convert non-alpha characters.
Wouldn't it be nice to convert longer texts also? – Wouldn't you need to convert longer texts, and not only single characters? For good measure, I've added a ToString(string) to the code below.

This leads to the following code (extending on the char value):

using System.Text;

public static class MyExtensions
{
    static byte[] braille = new byte[]
      { 0x20, 0x30, 0x24, 0x26, 0x22, 0x34, 0x36, 0x32, 0x14, 0x16,
        0x28, 0x38, 0x2c, 0x2e, 0x2a, 0x3c, 0x3e, 0x3a, 0x1c, 0x1e,
        0x29, 0x39, 0x17, 0x2d, 0x2f, 0x2b
      };

    public static string ToBraille(this char c) {
      return Convert.ToString(braille[ (byte) char.ToLower(c) - (byte) 'a'], 2).PadLeft(6, '0');
    }

    public static string ToBraille(this string s) {
      StringBuilder result = new StringBuilder();
      foreach (char c in s) {
        result.Append(c.ToBraille()).Append(" ");
      }

      return result.ToString();
    }
}


Optimizing reading integers directly into a set without a list comprehension in Python I am reading space separated integers, removing redundant ones and then sorting them. The following code works.

I was thinking whether this is the fastest way to do that or can it be optimized more?

import sys

def p():
    #Reads space seperated integers
    #Removes redundant
    #Then sorts them
    nums = set(map(int,next(sys.stdin).split()))
    nums = [i for i in nums] -
    nums.sort()
    print nums

EDIT:

tobia_k's suggestion is better performance-wise. Just 1 sentence instead of 3 and better in time-efficiency.

nums = sorted([int(i) for i in set(next(sys.stdin).split())])

inpt = raw_input("Enter a string of numbers: ")
numbers = sorted(set(map(int, inpt.split())))
print numbers

I would recommend separating the input-reading from the integer-extraction part.
Using raw_input (Python 2.x) or input (Python 3) is more usual for reading user inputs.
You can do it all in one line using sorted instead of sort.
Performance-wise it might be better to apply set first, and then map, so int has to be applied to fewer elements, but I don't think it makes much of a difference, if at all.


Dynamically generated attribute accessors in Perl In my spare time I'm working at a perl library, which is the lightweight implementation of a package I did some time ago. In the previous iteration I used Moose as object-oriented framework, as suggested by many people on the Internet.

Later in time I realized how Moose was a total overkill for my use case (I'm not a big fan of the 8-hours dependencies setup time), and that's the reason why I went for a refactoring.

Suddenly I found myself to face a situation where I want something similar to what I used to have with Moose, but in a far simper way: I would like one of my modules to have simple setter/getter properties without manually defining all of them.

I came out with the following simple implementation, which seems to be quite effective:

#!/usr/bin/perl -w

use strict;
use warnings;

package Foo;

BEGIN {
    no strict 'refs';
    foreach my $mname (qw/bar baz/) {
        *$mname = sub {
            my $self = shift;
            if (@_) {
                $self->{$mname} = shift
            } else {
                $self->{$mname}
            }
        }
    }
}

sub new { bless {}, shift }

What do you think of it? Do you see any evident drawback? This is the code I used for testing it, and it seems to work just fine:

package main;

use feature 'say';

my $x = Foo->new();
for (1 .. 10) {
    $x->bar($_);
    say 'bar: ', $x->bar;
    say 'baz: ', $x->baz || '<none>';
    $x->baz($_);
}
$x->goat_cheese(3) # broken

If Moose is an overkill, you can try Moo instead. There are some even simpler distributions like Class::Tiny or Object::Tiny. They are probably better tested than your own solution as their user base is larger. They might also be faster (more optimized) and play well with inheritance and roles.


Building a task listing for a task manager I'm super new to CodeIgniter 3 and I'm using it to build a task manager (Claritask.com). Also, I'm just starting to learn how to program, so forgive me beforehand if I'm not making sense and I'll try to clarify in comments.

As I'm building my specific Project page that lists specifc tasks, I was curious to know if I'm doing it right.

What I need on my view is: tasks belonging to a specific Project & the name of the project. I can get this to work fine, however I want to learn if my way of doing it correct.

I'm using two various methods

The first method I'm using, deals with having two functions on my Model, triggering two queries:

// Show project tasks
function get_tasks($project_id) {
    $this->db->from('tasks');
    $this->db->where('tasks.project_id', $project_id);
    $this->db->order_by('tasks.created_date', 'DESC');
    return $this->db->get()->result_array();
}

// Show single project name
function get_single_project($project_id) {
    $this->db->from('projects');
    $this->db->where('projects.id', $project_id);
    return $this->db->get()->row_array();
}

Then on my controller

$data['tasks'] = $this->project_model->get_tasks($project_id);
$data['project_name'] = $this->project_model->get_single_project($project_id)['project_name'];

This is working as expected, and producing an elegant array (especially on projects without tasks).

array (
  'tasks' =>
  array (
  ),
  'project_name' => 'Default name',
)

However I didn't like triggering two different queries, so I decided to JOIN projects with tasks and have one hit to the database, as such:

// Show project tasks
function get_project_tasks($project_id) {
    $this->db->select("tasks.*, projects.project_name");
    $this->db->from('projects');
    $this->db->where('projects.id', $project_id);
    $this->db->join('tasks', 'tasks.project_id = projects.id', "LEFT");
    $this->db->order_by('tasks.created_date', 'DESC');
    return $this->db->get()->result_array();
}

and then my controller:

$data['tasks'] = $this->project_model->get_project_tasks($project_id);
$data['project_name'] = $data['tasks'][0]['project_name'];

While the second approach is using one query only, It's also producing an "empty" array (below) on projects without any tasks and causing to print at least one empty line within a 'foreach'.

array (
  'tasks' =>
  array (
    0 =>
    array (
      'id' => NULL,
      'project_id' => NULL,
      'list_id' => NULL,
      'task_text' => NULL,
      'task_due_date' => NULL,
      'created_by' => NULL,
      'created_date' => NULL,
      'project_name' => 'Website design',
    ),
  ),
  'project_name' => 'Website design',
)

Even though, I'm mitigating that, with the following:

<?php foreach($tasks as $task) { ?>
    <?php if(isset($task['id'])): ?>

<?php echo $task['task_text']; ?>

    <?php endif; ?>
<?php } ?>

Even though there are thousands of ways to skin a cat, I would like to really know which approach is the correct one: Having a specific query for each element and printing an elegant output, or optimizing the outcome.

This is really a SQL question, I'd encourage you to keep learning true SQL because it's a skill that's invaluable and it's increasingly not learnt thanks to the popularisation of noSQL solutions and abstraction heavy ORMs

To answer your question swap the direction of the query from tasks to projects instead of from projects to tasks, this will ensure you only get projects that have tasks, alternatively you could have added an extra condition in your where clause to exclude NULL tasks on the left join

or you can simply swap the left join to a normal join

$this->db->select("tasks.*, projects.project_name");
$this->db->from('tasks');
$this->db->where('projects.id', $project_id);
$this->db->join('projects', 'tasks.project_id = projects.id', "LEFT");
$this->db->order_by('tasks.created_date', 'DESC');
return $this->db->get()->result_array();


Starting and stopping a thread for a command handler I built an application class that should be runnable as a Thread and it would be nice to hear opinions of Java developers to improve my coding style.

Main class.

package com.ozankurt;

public class Main {

    public static void main(String[] args) {

        Application application = new Application();

        application.start();

    }
}

Application class:

package com.ozankurt;

import com.ozankurt.utilities.Input;

public class Application implements Runnable {

    private CommandHandler commandHandler;

    private boolean running;

    private volatile Thread thread;
    private String threadName = "ApplicationThread";

    public Application() {

        Handler handler = new Handler(this);

        this.commandHandler = new CommandHandler(handler);

        this.running = true;
    }

    public void start() {

        if (thread != null) {
            System.out.println(threadName + "is already running.");

            if (!isRunning())
            {
                System.out.println("Resuming " + threadName + "...");
                setRunning(true);
            }
        } else {
            System.out.println("Starting " + threadName + "...");

            thread = new Thread(this, threadName);
            thread.start();

            System.out.println("Started " + threadName + ".");
        }
    }

    public void stop() {
        System.out.println("Halting " + threadName + "...");

        setRunning(false);

        System.out.println("Halted " + threadName + ".");
    }

    @Override
    public void run() {

        while (isRunning()) {
            String userInput = Input.readLine();

            commandHandler.handle(userInput);
        }
    }

    public boolean isRunning() {

        return running;
    }

    public void setRunning(boolean running) {

        this.running = running;
    }
}

I read this Java documentation about how to stop threads in Java without using stop method that has become deprecated because it might throw some exceptions.

As far as I understand we shouldn't try to stop a thread and instead we should define a property corresponding to the state of the thread.

In my case I defined a boolean running in my Application class and as you can read from my code in the run method I basicly check if the application is running. Which mean that I actually never stop the thread, I only remove its functionality by using a while statement. Is this the correct approach explained in the Java documentation?

Your approach is works for a single start-stop cycle. Afterwards, things may go wrong. Let's look at the simple scenario first, though:

You should keep in mind that you want to halt the running threads as soon as possible whilst maintaining system functionality. That is, when I say stop, and there's no calculating or whatever going on, the system ought to stop. But in your case, it might not.

You see, what you have here,

    while (isRunning()) {
        String userInput = Input.readLine();

        commandHandler.handle(userInput);
    }

Is either a spin loop or does not check the isRunning fast enough. Or so I think.

If there is no timeout for Input.readLine() and it does block until input is required, then stopping the application whilst it is waiting for input will lead to the application only actually shutting down AFTER the input has arrived and been handled. In case of a system shutdown, or programmed shutdown, this input may never arrive. The JVM will handle power situations for you, forcibly killing your thread, but if you were to use this as a library and another thread was told to shutdown the application your application thread wouldn't be shut down.

If there is no timeout for Input.readLine() and it does not block until input is required, then you've got a spin loop. When running, the application will check thousands of times whether you've got input, and then pass the non-existing input (maybe as empty string, maybe null), through your commandhandler. This is a waste of system resources.

By setting a timeout on the input reading, you can resolve this issue: By blocking for only 100ms (or so) for input, you can reduce the amount of checks to about 10 per second (which is easily handled by processors), whilst at the same time maintaining a fast response upon shutdown.

Secondly, your code is not threadsafe. There are certain cases which will cause things you do not want to happen.

If two threads call start() at the same time, then...

public void start() {

    if (thread != null) {
        System.out.println(threadName + "is already running.");

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            setRunning(true);
        }
    } else {
        System.out.println("Starting " + threadName + "...");

        thread = new Thread(this, threadName);
        thread.start();

        System.out.println("Started " + threadName + ".");
    }
}

Both threads start at the top of the function.

First line is this:

if (thread != null)

Well, thread is null in both cases. Thread 1 goes into the else statement, and so does thread 2.

    else {
        System.out.println("Starting " + threadName + "...");

        thread = new Thread(this, threadName);
        thread.start();

        System.out.println("Started " + threadName + ".");
    }

No checks occur here, so both threads start the application. You now have 2 application threads running.

Now, there are a few more scenarios I could describe, but there are some structural flaws in the code which should be addressed before we go into that.

First, you keep track of your state like this:

public Application() {

    Handler handler = new Handler(this);

    this.commandHandler = new CommandHandler(handler);

    this.running = true;
}

In the constructor you set running to true. But the application isn't running yet! What you should do is set running to true in the start function. That way, whenever start is called, the result is that the application is running (either it was running before, or it was started).

Secondly, whenever a thread needs to be "resumed", you check like this:

        System.out.println(threadName + "is already running.");

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            setRunning(true);
        }

That's not going to work, sadly. When you call stop, and set running to false, the thread can leave the while loop and terminate. A terminated thread cannot be restarted. So you must use a new thread!

But even this is not that easy - let's go with the naive approach first.

First, since we are going to be making threads both in the regular (no thread running) and resuming (thread was running but has terminated) functions of start, let's extract threadmaking to a separate function:

private void startApplicationThread(){
    thread = new Thread(this, threadName);
    setRunning(true);
    thread.start();
}

Next, add the startApplicationThread to the start function like so...

public void start() {

    if (thread != null) {
        System.out.println(threadName + "is already running.");

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            startApplicationThread();
        }
    } else {
        System.out.println("Starting " + threadName + "...");

        thread = new Thread(this, threadName);
        thread.start();

        System.out.println("Started " + threadName + ".");
    }
}

But, like this, we have another case of thread unsafe code: If thread 1 and thread 2 resume application via start at the same time, they will both reach startApplicationThread!

So let's fix that.

It's pretty easy to fix - just add synchronized. What synchronized does is say "only 1 thread can be in any method that is synchronized per instance at the same time". For now, this is good enough, but later you may want to use locking objects to prevent other possible coding errors.

public synchronized void start()
public synchronized void stop()

But it's still not threadsafe!

Let's say we have 1 thread running the application and 1 external thread. The external thread first calls stop. It goes through and running is now false. The application thread, however, is still busy with Input.readLine. That is to say, it's still waiting for input - as a result, it's still in the while loop and won't be going anywhere.

Quick overview of the situation:

Application thread is here:

    while (isRunning()) {
        String userInput = Input.readLine(); // <= applicationthread

        commandHandler.handle(userInput);
    }

External thread is here:

//external code
application.stop();

// <= externalthread
application.start();

And running is false.

Next, the external thread calls start. Because there is no other thread busy with start or stop, it can go into the method. It goes through the checks, thread != null is true, !isRunning() is also true, and starts a new thread (because the old one is "dead"). running is set to true again... and... oh snap.

Application threads:

    //<= applicationthread 2
    while (isRunning()) {
        String userInput = Input.readLine(); // <= applicationthread

        commandHandler.handle(userInput);
    }

External thread is here:

//external code
application.stop();

application.start();
// <= externalthread

And running is true, so application thread 2 will soon be moving into the while loop.

Now we've got 2 application threads again! Darn it.

So, before we start a new thread, we should make sure that it's actually dead. Now, whilst it is possible to save the running thread, it makes things hard, so lets discard the running thread - wait for it to die first.

Looking through the Javadoc of Thread, we can find Thread.join, which does this: "Waits for this thread to die." Great!

    if (thread != null) {
        System.out.println(threadName + "is already running.");

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            thread.join();
            startApplicationThread();
        }
    }

Now you get a compiler error because you need to catch InterruptedException - maybe you ought to deal with that in a cleaner way, but for now I'm going to ignore it.

    if (thread != null) {
        System.out.println(threadName + "is already running.");

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            try {
                thread.join();
            } catch (InterruptedException ie){
                //ignore
            }
            startApplicationThread();
        }
    }

Oh wait, now we could get InterruptedException whilst waiting. Not good. Let's just throw a while loop around it: (not the cleanest solution, but it'll work)

    if (thread != null) {
        System.out.println(threadName + "is already running.");

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            boolean stoppedRunningThread = false;
            while(!stoppedRunningThread){
                try {
                    thread.join();
                    stoppedRunningThread = true;
                } catch (InterruptedException ie){
                    //ignore
                }
            }
            startApplicationThread();
        }
    }

Might even make it do-while, since we have to do this at least once anyway.

    if (thread != null) {
        System.out.println(threadName + "is already running.");

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            boolean stoppedRunningThread = false;
            do {
                try {
                    thread.join();
                    stoppedRunningThread = true;
                } catch (InterruptedException ie){
                    //ignore
                }
            } while(!stoppedRunningThread);
            startApplicationThread();
        }
    }

So, now we are done, right?

Well... not quite. You see, what would happen if the thread that wants to restart the application is the application thread?

Well, we'd go through start, and wait for the currently running thread to... die... Yeah, that's not going to work! In that case, we can use Thread.currentThread() to compare! If we find that the thread that is restarting the application is the application thread, then we don't need to wait for it to die, we can just set running to true again, because we know it is still in the while loop and won't go out of the while loop whilst we are setting the variable.

    if (thread != null) {
        System.out.println(threadName + "is already running.");

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            if (thread != Thread.currentThread())
            {
                boolean stoppedRunningThread = false;
                do {
                    try {
                        thread.join();
                        stoppedRunningThread = true;
                    } catch (InterruptedException ie){
                        //ignore
                    }
                } while (!stoppedRunningThread);
                startApplicationThread();
            } else {
               setRunning(true);
            }
        }
    }

So now, finally, everything is threadsafe, yes?

Well no. You see, this method here is public:

public void setRunning(boolean running) {

    this.running = running;
}

And that ruins pretty much everything. You can kill the application at any time by just calling setRunning(false). What's worse, an application that's been stopped and been told to setRunning(true) can't be restarted! So you should make it private, and access it via start and stop.

private void setRunning(boolean running) {
    this.running = running;
}

So, total overview of how the class is looking right now:

package com.ozankurt;

import com.ozankurt.utilities.Input;

public class Application implements Runnable {

    private CommandHandler commandHandler;

    private boolean running = false;

    private volatile Thread thread;
    private String threadName = "ApplicationThread";

    public Application() {

        Handler handler = new Handler(this);

        this.commandHandler = new CommandHandler(handler);
    }

    public synchronized void start() {

        if (thread != null) {
            System.out.println(threadName + "is already running.");

            if (!isRunning())
            {
                System.out.println("Resuming " + threadName + "...");
                if (thread != Thread.currentThread())
                {
                    boolean stoppedRunningThread = false;
                    do {
                        try {
                            thread.join();
                            stoppedRunningThread = true;
                        } catch (InterruptedException ie){
                            //ignore
                        }
                    } while (!stoppedRunningThread);
                    startApplicationThread();
                } else {
                   setRunning(true);
                }
            }
        } else {
            System.out.println("Starting " + threadName + "...");

            startApplicationThread();

            System.out.println("Started " + threadName + ".");
        }
    }

    public synchronized void stop() {
        System.out.println("Halting " + threadName + "...");

        setRunning(false);

        System.out.println("Halted " + threadName + ".");
    }

    @Override
    public void run() {

        while (isRunning()) {
            String userInput = Input.readLine();

            commandHandler.handle(userInput);
        }
    }

    public boolean isRunning() {

        return running;
    }

    private void setRunning(boolean running) {

        this.running = running;
    }
}

Okay.

So is it threadsafe NOW?! Hmmmm... well... no.

Here's a new situation - an external thread stops the application, and then starts it again, but meanwhile the application thread ALSO wants to stop the application.

Basically like so:

//external code
application.stop();

application.start();

//... inside application.start()...

    if (thread != null) {
        System.out.println(threadName + "is already running.");

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            if (thread != Thread.currentThread())
            {
                boolean stoppedRunningThread = false;//<= external thread
                do {
                    try {
                        thread.join();
                        stoppedRunningThread = true;
                    } catch (InterruptedException ie){
                        //ignore
                    }
                } while (!stoppedRunningThread);
                startApplicationThread();
            } else {
               setRunning(true);
            }
        }
    }

And the application thread is here:

        while (isRunning()) {
            String userInput = Input.readLine();

            commandHandler.handle(userInput);//<= applicationthread
        }

Which is going to stop the application because the user typed "quit".

What happens is that because the external thread is in the start function, the application thread cannot enter stop, and as a result, it has to wait. But it's still alive. So the external thread, waiting in thread.join for the applicationthread to die, will have to wait forever.

A deadlock again!

Resolving this would be hard, but possible:

You could make the quit command break out of the while loop instead of calling stop
You could forcibly kill the thread after a bit of time
You could ignore the issue, how likely is it that this happens?
You could rewrite the locking to use a lock object, so that you can handle this in the stop method
You could make an application a single-use object, instantiating a new Application instead, making the whole restart-locking moot and everything a whole lot easier
You could make "quit" set a flag in the applicationthread, so that the application thread knows that it should stop the while loop even if running is true because the thread has quit...
Use "InterruptedException" to check if applicationthread is blocking you with a smart locking system
Use Locking libraries to be able to conditionally gain a lock before quitting, if you cannot gain a lock, and running is false and thread is not currentthread, then exit function

A whole list of options.

Here's mine:

        while (isRunning() && thread == Thread.currentThread()) {
            String userInput = Input.readLine();

            commandHandler.handle(userInput);
        }

    public synchronized void stop() {
        System.out.println("Halting " + threadName + "...");

        setRunning(false);
        thread = null;

        System.out.println("Halted " + threadName + ".");
    }

This should take care of most of the issues with minimum effort. The rest, I opt to ignore because I think them too rare to occur.

Alternatively, I'd rewrite my application flow so that I don't get into all sorts of crazy start-stop-start-start-stop situations. I'd prefer to change the flow to use Application only once per instance - a simple lifecycle of "start, stop, remain dead".


Split a string and write the i-th element into a buffer with C Question
Recently I've written the following splitv() function, without using strtok. Any way to make it shorter and more efficient?
Descrption
The function calculates the index-th element of the string str split by delim. The element is written into buff and 0 is returned. When an index error is occurred, any non 0 value is returned. This function was inspired by the python split function: buff = str.split(delim)[index], but I think it works too hard and can be improved.
int splitv(char buff[], char str[], char delim[], int index) {
    // returns 0 if an element is written. Else non 0.
    char *start = str, *end;
    int i = 0, buff_i = 0;
    // check if delim not in str
    if ((end = strstr(str, delim)) == NULL) {
        if (index == 0) strcpy(buff, str);
        return index;
    }
    // delim is in str
    for (i = 0; i < index; i++) {
        start = end + strlen(delim);
        end = strstr(start, delim);
        if (end == NULL) {
            // reached the last element
            if (i == index - 1) {
                end = str + strlen(str);
                break;
            }
            // index error: index >= elements_count
            return -1;
        }
    }
    // copy element to buffer
    while (start != end) {
        buff[buff_i] = *start;
        start++;
        buff_i++;
    }
    buff[buff_i] = '\0';
    return 0;
}

Thanks in advance!

Before we talk about improving the efficiency of your function, lets talk about safety:

You never check the validity of your function parameters.
You have no way of knowing how big buff is.

For the first issue, if str or delim is null, passing them directly in to strstr will cause a segfault and crash your program.
The second issue is worse: if buff is smaller then str, your function may end up clobbering memory when it tries to copy from str to buff.
This is undefined behavior, and the results are unpredictable.
Now for some efficiency suggestions:

Calculate string length only once.
Since strlen works by running over the string looking for '\0' terminator, it is more efficient to call it only once at the beginning of your function and store the result in a variable.
This can also allow you to implement a simple shortcut for edge cases such as where str or delimiter are empty strings, or delimiter is longer then str.

You don't need your own while loop at the end and the buff_i variable.
You can simply use memcpy or strncpy to copy the element in to the buffer.
memcpy would be slightly more efficient, since it does not check for '\0', while strncpy does.

Since your logic is that if delimiter is never found, but the requested element is 0 the result is "success", you don't need to handle this as a separate edge case.
That means you can get rid of the first if block, and handle everything in one loop.
You can also get rid of the special checks in the if loop and just focus on the search.

The following code example does not include the suggested validity checks:
int i;
char *start = str;
char *end = start;
size_t delim_len = strlen(delim);
size_t copy_len;

for (i = 0; i <= index && end != NULL; i++) {
    end = strstr(start, delim);
    if (end != NULL && i != index) start = end + delim_len;
}

/* element not found */
if (i != index) return index;

if (end == NULL) {
    copy_len = strlen(start);
} else {
    copy_len = (ssize_t)(end - start);
}

memcpy(buff, start, copy_len);
buff[copy_len] = '\0';

return 0;


Menu and SideBar Slides, Fades, and Hides I will admit that this code looks sloppy and repetitive. Which is why I'm asking how I can improve the efficiency and readability of this code.

// Sidebar tab highlight when clicked
$("#sidebar-tabs-container table td").click(function() {
  $("#sidebar-tabs-container table td").removeClass("sidebar-tab-selected");
  $(this).addClass("sidebar-tab-selected");
});

// poll and faq
// when faq question bar is clicked, slide down answer
$(document).on("click", ".question-container", function() {
$(".answer-container").stop().slideUp("fast");
$(this).parent().find(".answer-container").stop().slideToggle("fast");
});

// news and social feed

// activity
// activity new media content
//media buttons click
$(document).on("click", "#activity-media-icons-container span", function() {
    $("#activity-media-icons-container span").removeClass("activity-new-media-icon-  clicked");
$(this).addClass("activity-new-media-icon-clicked");
});

// show new media category and hide others depending on button clicked
$(document).on("click", "#new-media-images-button", function() {
   $("#activity-media-video-container, #activity-media-audio-container").slideUp(    function() {
   $("#activity-media-images-container").slideDown();
   });
});

$(document).on("click", "#new-media-videos-button", function() {
$("#activity-media-audio-container, #activity-media-images-container").slideUp( function() {
    $("#activity-media-video-container").slideDown();
});
});

$(document).on("click", "#new-media-audio-button", function() {
$("#activity-media-images-container, #activity-media-video-container").slideUp( function() {
    $("#activity-media-audio-container").slideDown();
});
});

/* show image link icon when hovered over image */
$(document).ajaxComplete(function() {
$(".media-image-buttons-container").hide();
});
// fade in image buttons on hover
$(document).on("mouseenter", "#media-content-container .media-image-container", function() {
$(this).children(".media-image-buttons-container").stop().fadeIn(200);
});
$(document).on("mouseleave", "#media-content-container .media-image-container", function() {
$(this).children(".media-image-buttons-container").stop().fadeOut(200);
});

This code can use quite a bit of work.

Run this code through a beautifier, ideally prior to posting it to codereview ;)
You are repeating yourself a lot, example :

// Sidebar tab highlight when clicked
$("#sidebar-tabs-container table td").click(function() {
  $("#sidebar-tabs-container table td").removeClass("sidebar-tab-selected");
  $(this).addClass("sidebar-tab-selected");
});

Unless you have a dynamic number of sidebar tabs ( seems unlikely ), you can do the following.

var $sidebarTabs  = $('#sidebar-tabs-container table td');
var selectedClass = 'sidebar-tab-selected';

$sidebarTabs.click(function() {
  $sidebarTabs.removeClass( selectedClass );
  $(this).addClass( selectedClass );
});

The interesting part is that you do this type of code twice, so you could also write a helper.

function wireClassToggler( elements, className )
{
  elements.click(function()
  {
    elements.removeClass( className );
    $(this).addClass( className );
  });
}

wireClassToggler( $('#sidebar-tabs-container table td'), 'sidebar-tab-selected' );
wireClassToggler( $('#activity-media-icons-container span'), 'activity-new-media-icon-clicked' );

You also seem to use copy pasted code for the slideUp and slideDown functions, you should generalize that as well.


How to design shopping cart Java application which satisfy modular, extensible and maintainable I am new in Application Design. I have use-case as below

As per the above story I have implemented code as below without any Modularity, Extensible and Maintainable. Could someone share the thoughts how to design below bad code to production grade application ?

Based on this story context, there could be other stories which could build-upon in later.
Example: Introduce new customer types like Gold, Diamond, Platinum, etc and their discounts slabs.

I really appreciate if some one have a look at the code below and let me know the how to design before creating data members, Interfaces and classes of improvements.

public class ShoppingCartDiscount {

    public static void main(String args[]) {
        System.out.println("Price for the premium customer: " + calculatePrice("Premium", 20000));
        System.out.println("Price for the regular customer: " + calculatePrice("Regular", 8000));
    }

    static float calculatePrice(String customerType, float purchaseAmount) {

        float total = 0;
        if (customerType.equalsIgnoreCase("Regular") {

            if (purchaseAmount > 5000 && purchaseAmount <= 10000) {

                float firstSlab = purchaseAmount - 5000;

                firstSlab = firstSlab - (float) (firstSlab * 0.1);

                total = 5000 + firstSlab;
            }

            else if (purchaseAmount > 10000) {

                float secondSlab = purchaseAmount - 10000;

                secondSlab = secondSlab - (float) (secondSlab * 0.2);

                float firstSlab = 10000 - 5000;

                firstSlab = firstSlab - (float) (firstSlab * 0.1);
                System.out.println("firstSlab:" + firstSlab);

                total = 5000 + firstSlab;

                total = total + secondSlab;

            } else if (purchaseAmount <= 5000 && purchaseAmount >= 0) {

                total = purchaseAmount;

            } else {
                return purchaseAmount;
            }

        } else if (customerType.equalsIgnoreCase("premium")) {
            if (purchaseAmount <= 4000) {

                total = purchaseAmount - (float) (purchaseAmount * 0.1);

            }
            if (purchaseAmount > 4000 && purchaseAmount <= 8000) {

                float secondSlab = purchaseAmount - 4000;

                secondSlab = secondSlab - (float) (secondSlab * 0.15);

                float firstSlab = 8000 - 4000;

                total = firstSlab - (float) (firstSlab * 0.1);

                total = total + secondSlab;
            }

            if (purchaseAmount > 8000 && purchaseAmount <= 12000) {

                float thirdSlab = purchaseAmount - 8000;

                thirdSlab = thirdSlab - (float) (thirdSlab * 0.20);

                float secondSlab = 8000 - 4000;

                secondSlab = secondSlab - (float) (secondSlab * 0.15);

                float firstSlab = 8000 - 4000;

                total = firstSlab - (float) (firstSlab * 0.1);

                total = total + secondSlab + thirdSlab;

            }
            if (purchaseAmount > 12000) {

                float fourthSlab = purchaseAmount - 12000;

                fourthSlab = fourthSlab - (float) (fourthSlab * 0.30);

                float thirdSlab = 8000 - 4000;

                thirdSlab = thirdSlab - (float) (thirdSlab * 0.20);

                float secondSlab = 8000 - 4000;

                secondSlab = secondSlab - (float) (secondSlab * 0.15);

                float firstSlab = 8000 - 4000;

                total = firstSlab - (float) (firstSlab * 0.1);

                total = total + secondSlab + thirdSlab + fourthSlab;

            }
        }

        return total;
    }

}

I separated the calculatePrice method into calculatePremiumPrice and calculateRegularPrice methods.  This made the code easier to visually inspect.

I also made the two blocks of if statements consistent, starting with the lower discounts and working my way up to the larger discounts.  Again, this makes the code easier to visually inspect.

I was more measured in my use of blank lines.  There's no need to put a blank line after every statement.  Use blank lines to separate logical concepts within a method.

If the method code fits on one screen, it's easier to visually inspect.

Finally, I ran all the test cases and formatted the output into currency.

import java.text.NumberFormat;

public class ShoppingCartDiscount {

    static NumberFormat formatter = NumberFormat.getCurrencyInstance();

    public static void main(String args[]) {
        System.out.println("Price for the regular customer: "
                + formatter.format(calculatePrice("Regular", 5000)));
        System.out.println("Price for the regular customer: "
                + formatter.format(calculatePrice("Regular", 10000)));
        System.out.println("Price for the regular customer: "
                + formatter.format(calculatePrice("Regular", 15000)));

        System.out.println();

        System.out.println("Price for the premium customer: "
                + formatter.format(calculatePrice("Premium", 4000)));
        System.out.println("Price for the premium customer: "
                + formatter.format(calculatePrice("Premium", 8000)));
        System.out.println("Price for the premium customer: "
                + formatter.format(calculatePrice("Premium", 12000)));
        System.out.println("Price for the premium customer: "
                + formatter.format(calculatePrice("Premium", 20000)));
    }

    static float calculatePrice(String customerType, float purchaseAmount) {
        float total = 0f;

        if (customerType.equalsIgnoreCase("Regular")) {
            total = calculateRegularPrice(purchaseAmount);
        } else if (customerType.equalsIgnoreCase("Premium")) {
            total = calculatePremiumPrice(purchaseAmount);
        }

        return total;
    }

    static float calculateRegularPrice(float purchaseAmount) {
        float total = 0f;

        if (purchaseAmount <= 5000) {
            total = purchaseAmount;
        } else if (purchaseAmount > 5000 && purchaseAmount <= 10000) {
            float firstSlab = purchaseAmount - 5000;
            firstSlab = firstSlab - (float) (firstSlab * 0.1);
            total = 5000 + firstSlab;
        } else {
            float secondSlab = purchaseAmount - 10000;
            secondSlab = secondSlab - (float) (secondSlab * 0.2);
            float firstSlab = 10000 - 5000;
            firstSlab = firstSlab - (float) (firstSlab * 0.1);
            total = 5000 + firstSlab;
            total = total + secondSlab;
        }

        return total;
    }

    static float calculatePremiumPrice(float purchaseAmount) {
        float total = 0f;

        if (purchaseAmount <= 4000) {
            total = purchaseAmount - (float) (purchaseAmount * 0.1);
        } else if (purchaseAmount > 4000 && purchaseAmount <= 8000) {
            float secondSlab = purchaseAmount - 4000;
            secondSlab = secondSlab - (float) (secondSlab * 0.15);
            float firstSlab = 8000 - 4000;
            total = firstSlab - (float) (firstSlab * 0.1);
            total = total + secondSlab;
        } else if (purchaseAmount > 8000 && purchaseAmount <= 12000) {
            float thirdSlab = purchaseAmount - 8000;
            thirdSlab = thirdSlab - (float) (thirdSlab * 0.20);
            float secondSlab = 8000 - 4000;
            secondSlab = secondSlab - (float) (secondSlab * 0.15);
            float firstSlab = 8000 - 4000;
            total = firstSlab - (float) (firstSlab * 0.1);
            total = total + secondSlab + thirdSlab;
        } else {
            float fourthSlab = purchaseAmount - 12000;
            fourthSlab = fourthSlab - (float) (fourthSlab * 0.30);
            float thirdSlab = 8000 - 4000;
            thirdSlab = thirdSlab - (float) (thirdSlab * 0.20);
            float secondSlab = 8000 - 4000;
            secondSlab = secondSlab - (float) (secondSlab * 0.15);
            float firstSlab = 8000 - 4000;
            total = firstSlab - (float) (firstSlab * 0.1);
            total = total + secondSlab + thirdSlab + fourthSlab;
        }

        return total;
    }

}

Based on the comment by the OP, I created more adaptable code.  I'm not sure what pattern this is, except I used a factory to build the rewards.

I created a Tier class to hold a tier, a Reward class to hold a reward, and a RewardFactory to define the rewards.  This should make it easier to change tiers or add new reward types.

If a new reward concept is created, then some code would have to be added.

Here's the revised code.

import java.text.NumberFormat;
import java.util.ArrayList;
import java.util.List;

public class ShoppingCartDiscount {

    static NumberFormat formatter =
            NumberFormat.getCurrencyInstance();

    public static void main(String args[]) {
        ShoppingCartDiscount scd = new ShoppingCartDiscount();
        RewardFactory rewardFactory = scd.new RewardFactory();

        String rewardType = "Regular";
        float amount = 5_000f;
        float discount = rewardFactory.calculateDiscount(
                rewardType, amount);
        displayDiscount(rewardType, amount, discount);

        amount = 10_000f;
        discount = rewardFactory.calculateDiscount(
                rewardType, amount);
        displayDiscount(rewardType, amount, discount);

        amount = 15_000f;
        discount = rewardFactory.calculateDiscount(
                rewardType, amount);
        displayDiscount(rewardType, amount, discount);

        System.out.println();

        rewardType = "Premium";
        amount = 4_000f;
        discount = rewardFactory.calculateDiscount(
                rewardType, amount);
        displayDiscount(rewardType, amount, discount);
        amount = 8_000f;
        discount = rewardFactory.calculateDiscount(
                rewardType, amount);
        displayDiscount(rewardType, amount, discount);
        amount = 12_000f;
        discount = rewardFactory.calculateDiscount(
                rewardType, amount);
        displayDiscount(rewardType, amount, discount);
        amount = 20_000f;
        discount = rewardFactory.calculateDiscount(
                rewardType, amount);
        displayDiscount(rewardType, amount, discount);
    }

    static void displayDiscount(String rewardType,
            float amount, float discount) {
        System.out.print(rewardType);
        System.out.print(" customer spends ");
        System.out.print(formatter.format(amount));
        System.out.print(", so we discount ");
        System.out.print(formatter.format(discount));
        System.out.print(", so he owes ");
        amount -= discount;
        System.out.print(formatter.format(amount));
        System.out.println(".");
    }

    public class RewardFactory {

        private List<Reward> rewards;

        public RewardFactory() {
            this.rewards = new ArrayList<>();
            createRewards();
        }

        private void createRewards() {
            Reward reward = new Reward("Regular");
            Tier tier = new Tier(0f, 5_000f, 0.00f);
            reward.addTier(tier);
            tier =  new Tier(5_000f, 10_000f, 0.10f);
            reward.addTier(tier);
            tier =  new Tier(10_000f, Float.MAX_VALUE, 0.20f);
            reward.addTier(tier);
            rewards.add(reward);

            reward = new Reward("Premium");
            tier = new Tier(0f, 4_000f, 0.10f);
            reward.addTier(tier);
            tier = new Tier(4_000f, 8_000f, 0.15f);
            reward.addTier(tier);
            tier = new Tier(8_000f, 12_000f, 0.20f);
            reward.addTier(tier);
            tier = new Tier(12_000f, Float.MAX_VALUE, 0.30f);
            reward.addTier(tier);
            rewards.add(reward);
        }

        public float calculateDiscount(String rewardType,
                float amount) {
            float discount = 0f;

            for (Reward reward : rewards) {
                if (reward.isDiscountApplied(rewardType)) {
                    discount += reward.calculateDiscount(amount);
                }
            }

            return discount;
        }
    }

    public class Reward {

        private final String rewardType;

        private List<Tier> tiers;

        public Reward(String rewardType) {
            this.rewardType = rewardType;
            this.tiers = new ArrayList<>();
        }

        public void addTier(Tier tier) {
            this.tiers.add(tier);
        }

        public boolean isDiscountApplied(String type) {
            return (rewardType.equalsIgnoreCase(type));
        }

        public float calculateDiscount(float amount) {
            float discount = 0f;

            for (Tier tier : tiers) {
                if (tier.isDiscountApplied(amount)) {
                    discount += tier.calculateDiscount(amount);
                }
            }

            return discount;
        }

    }

    public class Tier {

        private final float lowerAmount;
        private final float upperAmount;
        private final float percentDiscount;

        public Tier(float lowerAmount, float upperAmount,
                float percentDiscount) {
            this.lowerAmount = lowerAmount;
            this.upperAmount = upperAmount;
            this.percentDiscount = percentDiscount;
        }

        public boolean isDiscountApplied(float amount) {
            return (lowerAmount < amount);
        }

        public float calculateDiscount(float amount) {
            if (amount > upperAmount) {
                return (upperAmount - lowerAmount) *
                        percentDiscount;
            } else {
                return (amount - lowerAmount) * percentDiscount;
            }
        }

    }

}


Validating C++ Time class objects I created a Time class. Now I want to modify the code to perform input validation. Hour should be between 0-24, minutes and seconds between 0-59. If class need improvements, please offer suggestions.

Time.h Header File

#ifndef TIME_H
#define TIME_H

class Time
{
    private :
        int hour;
        int minute;
        int second;
    public :
        Time(const int h = 0, const int m  = 0, const int s = 0);    //with default value
        void setTime(const int h, const int m, const int s);        //  setter function
        void print() const;       // Print a description of object in " hh:mm:ss"
        bool equals(Time) const;     //compare two time object
};

#endif

Time.cpp implementation file

#include <iostream>
#include "Time.h"

Time :: Time(const int h, const int m, const int s) : hour(h), minute (m), second(s)
{}

void Time :: setTime(const int h, const int m, const int s)
{
     hour = h;
     minute = m;
     second = s;
}

void Time :: print() const
{
    (hour < 10) ? std::cout << 0 << hour : std::cout << hour;
    std::cout << ":" ;
    (minute < 10) ? std::cout << 0 << minute : std::cout << minute ;
    std::cout << ":" ;
    (second < 10) ? std::cout << 0 << second : std::cout << second ;
    std::cout << "\n" ;
}

bool Time :: equals(Time otherTime) const
{
    if(hour == otherTime.hour && minute == otherTime.minute && second == otherTime.second)
        return true;
    else
        return false;
}

main.cpp

#include <iostream>
#include "Time.h"

int main()
{
   Time t1(10, 50, 59);
   t1.print();   // 10:50:59
   Time t2;
   t2.print(); // 06:39:09
   t2.setTime(6, 39, 9);
   t2.print();  // 06:39:09

   if(t1.equals(t2))
        std::cout << "Two objects are equal\n";
    else
        std::cout << "Two objects are not equal\n";

   return 0;
}

modified code of setTime for input validation (need suggestions)

void Time :: setTime(const int h, const int m, const int s)
{
    if(h>=0 && h <=23)
        hour = h;
    else
        hour = 0;

    if(m>=0 && m <=59)
        minute = m;
    else
        minute = 0;

    if(s>=0 && s <=59)
        second = s;
    else
        second = 0;
}

I don't like the internal storage type of (Hour/Min/Seconds).

private :
    int hour;
    int minute;
    int second;

This makes it hard to do comparison move forward backwards etc. I personally just store the number of seconds since midnight.

 private:
    int secondsSinceMidnight;

The converse to this is that usage may make this inefficient. If your main usage is just printing rather than modifying it may be worth keeping it as Hours/Minutes/Seconds (but I would measure before jumping to that conclusion as it is unlikely the main slowness in printing).

I don't like the default time being midnight.

    Time(const int h = 0, const int m  = 0, const int s = 0);    //with default value

If I construct a time object with no explicit value I usually expect it to be the current time. So personally I may default minutes and seconds to zero but I would expect at least the hours to be set. I would then provide a default constructor that sets the object to the current time:

    Time();  // Current time.
    explicit Time(const int h, const int m  = 0, const int s = 0);

Don't like Getters/Setters (they break encapsulation). Also I don't really see the use of this setter over the constructor.

    Time  x(5, 6, 7);

    // Now at a latter point I want to reset `x`;
    x = Time(8, 9, 10);

    // Performs equivalent action to:
    x.setTime(8, 9, 10);

When you provide a print method it is usally a good idea to also pass as an argument the stream you want to print on. This can default to std::cout but at least provide it.

    void print(std::ostream& str = std::cout) const;       // Print a description of object in " hh:mm:ss"

This also lets you define the standard output operator for the class very easily.

    std::ostream& operator<<(std::ostream& str, Time const& data)
    {
        data.print(str);
        return str;
    }

Now you can print using the method. Or the more more normal C++ technique.

    Time  x(5,7,8);
    x.print();              // defaults to std::cout
    x.print(std::cout);     // Or explicitly specify a stream.
    std::cout << x;         // Or just stream to the output.

Fine to have a named function (but pass parameter as const reference).

    bool equals(Time const& other) const;     //compare two time object

But why not define the equality operator as well. It makes the code more natural to read.

    bool operator==(Time const& lhs, Time const& rhs)
    {
         return lhs.equals(rhs);
    }

You don't need to manually put a zero into each field.

       (hour < 10) ? std::cout << 0 << hour : std::cout << hour;

There is a standard manipulator that does that automatically.

   std::cout << std::setw(2) << std::setfill('0') << h << ":"
             << std::setw(2) << std::setfill('0') << m << ":"
             << std::setw(2) << std::setfill('0') << s << "\n";


Reduce number of operations to get one array value I can't wrap my head around that. There seem to be a lot of redundancy, and even if it is not called called more than ten times in the main loop, I still find it hard to swallow that it loops through the whole list at each invocation.

def get_file_info(f, stream, field):

    discoverer = GstPbutils.Discoverer()
    discoverer.connect('discovered', on_discovered)
    info = discoverer.discover_uri(f)

    for vinfo in info.get_video_streams():
        ftype = (vinfo.get_stream_type_nick())
        width = vinfo.get_width()
        height = (format(vinfo.get_height()))
        depth = (format(vinfo.get_depth()))
        framerate = ('Frate: {}'.format(vinfo.get_framerate_num()))
        bitrate = ('Brate: {}'.format(vinfo.get_max_bitrate()))
        par = vinfo.get_par_num()
        fname = vinfo.get_caps().to_string().split(',')[0]

    if stream == 'video':
        if field == 'duration':
            return str(convert_time(info.get_duration(), 'hh:mm:ss'))
        elif field == 'width':
            return str(width)
        elif field == 'height':
            return str(height)
        elif field == 'type':
            return str(ftype)
        elif field == 'depth':
            return str(depth)
        elif field == 'framerate':
            return str(framerate)
        elif field == 'bitrate':
            return str(bitrate)
        elif field == 'par':
            return str(par)
        elif field == 'name':
            return str(par)

I read the lambda documentation, but can't seem to write a proper one, but I would sure like to.

@Stewie Griffin's answer is good, but I dare say it promotes eval, where dict is safer and more expansive.
The main aim here is to use the same if's as Stewie, as they are definitely good.
As a dictionary contains things and you can't get other things out of it it's safer.
With Stiewies for example you can pass 'get_file_info' as the field and it'll give you '<function get_file_info at 0x03335150>'.
It also means that you can return a dictionary for each and every stream in info.get_video_streams().

To obtain this we will make a variable video_info that will hold the needed information.

video_info = {
    'width': 2,
    'height': 3,
    'name': 'Name of video'
}

field = 'width'
str(video_info[field])
Out[3]: '2'

field = 'name'
str(video_info[field])
Out[5]: 'Name of video'

This means we just need to move all the information into this dictionary.
And so you'll need to change the for-loop to create a dictionary.

video_info = {
    'type': vinfo.get_stream_type_nick(),
    'width': vinfo.get_width(),
    'height': format(vinfo.get_height()),
    'depth': format(vinfo.get_depth()),
    'framerate': 'Frate: {}'.format(vinfo.get_framerate_num()),
    'bitrate': 'Brate: {}'.format(vinfo.get_max_bitrate()),
    'par': vinfo.get_par_num(),
    'fname': vinfo.get_caps().to_string().split(',')[0]
}

I'd also go on to say that you can change the code to return a dictionary and you can then remove all the field stuff from this function.
And you can use a list comprehension to return a dictionary for every info.get_video_streams().

which leads to:

def get_file_info(f, stream):
    discoverer = GstPbutils.Discoverer()
    discoverer.connect('discovered', on_discovered)
    info = discoverer.discover_uri(f)
    duration = convert_time(info.get_duration(), 'hh:mm:ss')
    return [
        {
            'duration': duration,
            'type': vinfo.get_stream_type_nick(),
            'width': vinfo.get_width(),
            'height': format(vinfo.get_height()),
            'depth': format(vinfo.get_depth()),
            'framerate': 'Frate: {}'.format(vinfo.get_framerate_num()),
            'bitrate': 'Brate: {}'.format(vinfo.get_max_bitrate()),
            'par': vinfo.get_par_num(),
            'fname': vinfo.get_caps().to_string().split(',')[0],
        }
        for vinfo in info.get_video_streams()
    ]

def get_file_field(f, stream, field):
    videos = get_file_info(f, stream)
    video_info = videos[-1]
    if stream == 'video':
        return str(video_info[field])


Calculating the sum of array elements to the left and right I am using C++ to code the following logic:

  An Array is given we have to traverse the array such that sum of array elements to its left must be equal to sum of elements to its right.

BTW this is hackerrank Problem.

Here's the code , but this is giving me time-out, that is my code is too slow for very large input, I want to make it faster.

int sumArray(int arr[],int start,int end){
    int sum = 0;
    for(int i=start;i<=end;i++)
        sum += arr[i];
    return sum;
}

// this is inside main
int ar[N];
    for(int n=0;n<N;n++)
        cin>>ar[n];

    bool found = false;
    if(N == 1)
        found = true;
    for(int i=1;i<N;i++){
        int left = sumArray(ar,0,i-1);
        int right = sumArray(ar,i+1,N-1);
        if( left == right)
            found = true;
    }
    if (found)
        cout<<"YES"<<endl;
    else
        cout<<"NO"<<endl;

  This is how the problem works, this is given array
   1 2 3

  1 2 3 3

  In the first test case, no such index exists.

  In the second test case, A[0] + A[1] = A[3]. So 2(start from 0) is the position where this condition occurs.

  this is slow for N=10000 elements in array it excceds 2ms time limit

So where should my code be improved? My mind says I should give a try with   dynamic programming because I am calling same process each time and start it from zero. Is there any good method to do this. Or is there a better way to make above code work for large inputs (I know there's some better way)?

All suggestions are warmly welcomed!!

This problem is a common problem in coding challenges, and it's a nice one, because there's a really elegant solution which is really fast. It does not even need dynamic programming.... just a little trick of logic.

Your solution takes each position, and for each position, sums all values to the left, and right, then it repeats that until it hits a match. Your solution thus scans all N elements about N/2 times (on average, you scan half the data until you hit a match). This makes your solution a time-complexity of \$O(n^2)\$. If you double the size of the input, the solution takes 4 times longer.

Now, a simple solution is to scan the entire data once, and calculate the sum of all the values:

int sum = 0;
for(int i = 0; i < N; i++){
    sum += arr[i];
}

Note, in the code above, I have added spaces to the expressions to make them more readable.

Now, with that value, if your position is 0, the sum to the left is 0, and to the right is sum.

If you move your cursor to the right, the sum to the left is now the value at element 0 ar[0], and to the right is the sum - ar[0].

So, you can loop until you find the match.....

int left = 0;
int right = sum;
for(int i = 0; i < N; i++){
    left += arr[i];
    right -= arr[i];
    if (left == right) {
        return i;
    }
}

See how, as you go, you can "shift" the value from the one side to the other? This makes the solution a simple \$O(n)\$ complexity.


Comparing and counting thousands of dates converted using moment.js I have a very large list of users (a Mongo Collection with roughly +4000 users). Each user belongs to a site.

On the client-side I need to loop over both the sites and users to get completed and incomplete counts between a startDate and endDate. This logic must take place on the client-side.

So far I have two for loops and am using moment.js for the time conversions. I noticed that after adding moment, this page took significantly longer to load (about 3 seconds vs. 40ms originally).

Is there a way I can optimize the code below such that the moment time conversions do not drastically slow down my app?

  function getStatusCounts(sites, users, startDate = new Date(-10000000000000), endDate = new Date(2640000000000000)) {
    startDate = moment.tz(startDate, 'America/Los_Angeles').format('MM/DD/YYYY h:mm a');
    endDate   = moment.tz(endDate, 'America/Los_Angeles').format('MM/DD/YYYY h:mm a');
    var tempDate, tempUserDate;

    for (var i = 0; i < sites.length; i++) {
      for (var j = 0; j < users.length; j++) {
        tempDate = new Date(users[j].userSubmittedAt);
        tempUserDate = moment.tz(tempDate, 'America/Los_Angeles').format('MM/DD/YYYY h:mm a');

        if (sites[i].url === users[j].site_url && moment(tempUserDate).isAfter(moment(startDate)) && moment(tempUserDate).isBefore(moment(endDate))) {

          if (users[j].isCompleted === 'true') {
            completed[i]++;
          } else {
            incompleted[i]++;
          }
        }
      }
    }
  }

If you have \$N\$ users and \$M\$ total sites, your two for loops have time complexity \$O(NM)\$. This is a bit high when \$N\$ and \$M\$ are on the order of a couple thousand, and probably explain your performance issues.

You are wasting a lot of time comparing sites with users with different site_url parameters. You can speed up your code to run in time \$O(N + M)\$ by doing the following:

Build a map siteMap from url values to their index in sites. That is, siteMap should satisfy siteMap[sites[i].url] = i. It is easy to construct this in \$O(M)\$ time by just looping over sites.
Omit the outer for loop over all sites. Instead, just set i in the inner for loop to siteMap[users[j].site_url]. I would also recommend renaming i to something more descriptive in this case, e.g. siteIndex.

This costs an extra \$O(M)\$ worth of space to store siteMap, but since you have multiple arrays of this size already, this shouldn't be a huge increase in memory burden.

One final note: usually it's good to make the database query do as much of the work as possible instead of filtering it out later manually in the client. One example here where you could possibly speed things up is, instead of checking that the user's time is between startDate and endDate in this JavaScript function, you should adjust your MongoDB query to just return to you the users in this range.

If you don't have the option of adjusting the MongoDB query (or this would be a bad idea for other reasons, like you need to find the answer quickly for a bunch of ranges and DB calls have high latency), then it would be a good idea to sort your users by their userSubmittedAt parameter. Then you can use a simple binary search to find the appropriate range of users in \$O(\log N)\$ time.


Creating a linked list of strings Warning: I'm quite new to low-level programming and memory management so please don't mind it when I'm asking incredibly stupid things.

I'm working on a project where I need to read all the lines from a file and store them for further usage. I decided to use a linked list of untyped pointers for this which will contain pointers to characters.

The following code for testing actually does the job but I've got a feeling that the creation and extending of the list could be done in a "cleaner" way.

list-test.c

struct Node {
  void *value;
  struct Node *next;
};

long int count_to_next(char which, char *buffer, long int max);
void string_copy(char *from, char *to, long int chunk_size);

int main() {
  char *raw = "Line 1\nLine 2\nLine 3";
  int remaining = 20;
  int chunk_size;
  char *pointer = raw;
  char *buffer;
  struct Node *root = malloc(sizeof(struct Node));
  struct Node *last = root;
  struct Node *lines;

  while (remaining > 0) {
    chunk_size = count_to_next('\n', pointer, remaining);

    buffer = malloc(chunk_size * sizeof(char) + 1);

    string_copy(pointer, buffer, chunk_size);
    *(buffer + chunk_size) = '\0';

    last->next = malloc(sizeof(struct Node));
    last->next->value = (void *) buffer;
    last->next->next = NULL;
    last = last->next;

    /* + 1 bc the '\n' is ignored */
    remaining -= chunk_size + 1;
    pointer += chunk_size + 1;
  }

  lines = root->next;

  /* do something with lines */
}

While I agree with most of what's been said in the comments, I do think that writing your own linked list is a worthwhile exercise. (Though so is learning about reallocating arrays.) So I'll go with that premise - that it's worth your time to write a linked list. And I think you've done a pretty good job to start with! Here are some suggestions.

Don't Reinvent The Wheel

You've written your own functions for count_to_next() and string_copy(). There are functions in the standard C library that do these same things, so you don't need to write your own. Instead, you could use strpbrk() and strdup() or memcpy(). strdup() actually allocates the memory for you, if you want to go that route.

Edit: Also, instead of calling malloc() and then manually setting each field to 0, you can use calloc(sizeof(Node), 1); to both allocate and clear the memory.

Use Types

You should create type definitions for your structures to make it easier to read and save some typing. I'd do this:

typedef struct Node {
    void* value;
    struct Node* next;
} Node;

Then, whenever you declare one, you can simply write:

Node *root;

instead of:

struct Node *root;

Avoid Magic Numbers

Instead of just putting the number 20 into your code, you should either have a named constant telling you what the value 20 represents (something like const int TEST_STR_LENGTH = 20;, or get the value from the string so you can change the string without having to update the value in the code. Like this:

int remaining = strlen(raw);

Name Your Variables Appropriately

The variable names raw, pointer, and buffer aren't very informative. I recommend naming them something like input_string, next_line, and string_copy or something along those lines.

Bugs

I think the last line is incorrect. I think it should be:

lines = root;

Otherwise you're starting with the second line.

Also, you were starting with an empty node with an uninitialized string, and ending with an empty node with an uninitialized string. I fixed the first by setting the value field of last before allocating a new node. I fixed the second by only allocating the next node if we have some characters remaining in the string.

Edit: I also realized that both of our implementations have another bug in them. If the file (or input string) is empty, we end up with a list containing an empty node, whereas we probably should not have a list at all. You could fix that by not allocating a node until you know you have a string. So root would start out as NULL and could end up as NULL if there are no lines in the input. I'll leave that as an exercise for the reader. :-)

So putting it all together, your function might look something like this:

#include <stdlib.h>
#include <string.h>

typedef struct Node {
    void* value;
    struct Node* next;
} Node;

int main() {
    char *input_string = "Line 1\nLine 2\nLine 3";
    long remaining = (long)strlen(input_string);
    long chunk_size;
    char *next_line = input_string;
    char *string_copy;
    Node *root = calloc(sizeof(Node), 1);
    Node *last = root;
    Node *lines = NULL;

    while (remaining > 0) {
        char* next_new_line = strpbrk(next_line, "\n");
        /* If the string doesn't end with "\n", we need the rest of the string */
        if (next_new_line == NULL) {
            chunk_size = remaining;
        }
        else {
            chunk_size = next_new_line - next_line;
        }

        /* This allocates and copies the string */
        string_copy = strndup(next_line, chunk_size);

        last->value = (void *) string_copy;

        /* + 1 bc the '\n' is ignored */
        remaining -= chunk_size + 1;
        next_line += chunk_size + 1;

        /* If there's more, then allocate another node for the next go round */
        if (remaining > 0) {
            last->next = calloc(sizeof(Node), 1);
            last = last->next;
        }
    }

    lines = root;

    /* do something with lines */
}

Oh, and don't forget to free() all those strings and Nodes after you've used them!


Random bits with a specified density (Bernoulli Trials) I'm sure this is a fully 'solved' problem and expecting cross references but I can't find this on Google. I'm probably missing a keyword.

Here is a naive function for populating a bool array (b[i]) of length N in a Bernoulli distribution.
P(b[i]==true)==p and P(b[i]==false)==(1-p) for each independent random variable b[i].

#include <stdlib.h>
#include <stdio.h>
#include <stdbool.h>

void randomize(bool*const bits,const size_t len,double p){

    for(size_t i=0;i<len;++i){
        if(p>=1.0||rand()<p*RAND_MAX){//Need to force the hand for p>=1.0 as 0 and RAND_MAX possible.
            bits[i]=true;
        }else{
            bits[i]=false;
        }
    }
}

int main(void) {

    const size_t len=100;//N in the text.
    bool* bits=malloc(len*sizeof(*bits));//b[i] in the text.
    if(bits==NULL){
        return EXIT_FAILURE;
    }

    srand(1234);//Fixed for Reproducibility.

    randomize(bits,len,0.3);

    int count=0;
    for(size_t i=0;i<len;++i){
        printf("%c" , (bits[i]?'1':'0'));
        if(bits[i]){
            ++count;
        }
    }
    printf("\ncount=%f\n",((double)count)/((double)len));

    free(bits);

    return EXIT_SUCCESS;
}

Obviously it's linear \$\mathcal{O}(N)\$ for 'seeks' in the array.

However I could start with an array full of (0) and calculate a random variable (\$B\$) in the binomial distribution to get the number of bits to set then select them at random.

If \$p\$ is small that will involve far fewer seeks. It will still be (expected) asymptotically linear but with a far reduced constant.

If I naively pick the \$B\$ elements to set I might pick the same one twice (Birthday Paradox anyone?) and will need to pick again. If I just 'scan' forwards to the first free slot I will introduce a 'clumping' bias and violate the independence of the random variables P(b[i+1]==1|b[i]==1)>p. Clumping is a no-no for my application.

Therein lies the rub. If \$p\$ is near to 1.0 it will take an increasingly massive amount of time to fill the array. When it's nearly full randomly picking a 0 will typically take a lot of retries.

I can be 'clever' and if \$B \gt \frac{N}{2}\$ I can block set to 1 and set \$N-B\$ back to 0. That means only counting 'seeks' I will always beat the full scan above.
That's because the average retries at each step is \$\le 1 \$so expected seek count is \$ \lt N\$.

Does anyone have such a piece of code readily on hand?

PS: The data structure I'm working with isn't an array; it's a particular kind of tree. That's not important but might explain why I'm counting 'seeks' as the overriding parameter. However I am able to easily conjure up a 'tree of false' and a 'tree of true' to implement that 'block fill' requirement.

For what I can see, the code fills an array with a given amounts of true values (the rest being false) in random order. I'd rather initialize first p*N elements to true, and then random_shuffle the array.

That said I don't understand why you need this array at all. Can we see how your application uses it?


Multi-Threaded Java Server As practice I wanted to write a Socket in Java:

/* User: koray@tugay.biz Date: 21/02/15 Time: 13:30 */

import java.io.IOException;
import java.net.ServerSocket;

public class MyServer {

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

        ServerSocket serverSocket = new ServerSocket(8888);

        while (true) {
            new Thread(new ServerSocketThread(serverSocket.accept())).start();
        }
    }

}

and the rest of it:

/* User: koray@tugay.biz Date: 21/02/15 Time: 18:14 */

import java.io.*;
import java.net.Socket;
import java.util.Scanner;

public class ServerSocketThread implements Runnable {

    Socket socket;

    public ServerSocketThread(Socket accept) {
        this.socket = accept;
    }

    @Override
    public void run() {
        try {

            Scanner s = new Scanner(socket.getInputStream());
            String readLine;

            while (!(readLine = s.nextLine()).equals("bye")) {
                System.out.println(readLine);
            }

            new PrintWriter(socket.getOutputStream()).write("Bye then..");
            socket.close();

        } catch (IOException e) {
            e.printStackTrace();
        }

    }
}

I wanted to write it as clean as possible. Any improvements, suggestions?

I can use it like this:

Korays-MacBook-Pro:~ koraytugay$ telnet localhost 8888
Trying ::1...
Connected to localhost.
Escape character is '^]'.
koray
tugay
asdfako
bye
Connection closed by foreign host.

There's not a lot to say about such a simple program, mainly nitpicks:

Member fields that you don't need to expose to the outside should be private, such as the socket in ServerSocketThread.
Whenever you can, make member fields final, for example the socket in ServerSocketThread.
The Socket parameter in the constructor of ServerSocketThread is poorly named accept. It would be better to call it what it is: socket
s for a Scanner is not a great name either. How about scanner ?
It's a bit odd that the server doesn't talk back to the client, just prints stuff on its console.
If the program gets more complex later:

It might be good to move the literal string like "bye" to a constant variable, as it's somewhat special, being the special terminating sequence
Printing stack trace on the console is considered bad practice
Printing pretty much anything to the console is considered bad practice

However, at this point, your program being a toy, it doesn't really matter, just for the record...


Requesting product data from an eBay webform The code takes in a hash that comes from a webform and also works with product data from the database. It then uses an eBay gem I built that connects with the eBay API and then returns a response object that you can call #extract on to return desired data. #extract takes an optional argument that will return data other than the default.

I've been working with this code for quite some time, trying to figure out how I can break it up. It feels very clunky and unorganized.

require 'ebay'

class ListingForm
  include ActiveModel::Model

  attr_accessor :title, :price, :subtitle, :product_id, :duration

  def initialize(form={})
    @title = form[:title]
    @price = form[:price]
    @subtitle = form[:subtitle]
    @product_id = form[:product_id]
    @duration = form[:duration]
    @product = set_product if @product_id
  end

  def post
    yield self if block_given?
    ebay_response = api.add_item(item: item)
    item_id = ebay_response.extract
    if valid_listing?(item_id)
      Listing.new(product_id: @product.id,
                  item_number: item_id).save
    else
      raise "invalid item number"
    end
    ebay_response
  end

  def valid_listing?(item_id)
    item_id.to_i != 0
  end

  def item_specifics
    item_specifics = []
    @product.properties.each do |k,v|
      name = k.to_s.for_humans
      value = "#{v}#{PROPERTY_FORMATTER[k.to_sym]}"
      if value == "1"
          value = "included"
      elsif value == "0"
        next
      end
      item_specifics << Ebay::Types::NameValueList.new({name: name, value: value})
    end
    item_specifics
  end

  def remote_images
    remote_images = []
    @product.images.each do |i|
      url = "http://www.howmuchcomputer.com" + i
      remote_url = @api.upload_site_hosted_pictures({:external_picture_url => url})
      remote_images << remote_url.extract(ignore_errors: true)
    end
    remote_images.reverse!
  end

  def template(path)
    ERB.new(File.open(path, "r").read).result(binding).to_s
  end

  def item
    Ebay::Types::Item.new({
      :start_price => price.to_s || @product.price.to_s,
      :description => template("lib/ebay_templates/description.html.erb"),
      :condition_description => template("lib/ebay_templates/condition_description"),
      :listing_duration => "Days_#{duration}",
      :item_specifics => item_specifics,
      :listing_type => "FixedPriceItem",
      :title => title || @product.ebay_title,
      :currency => "USD",
      :country => "US",
      :postal_code => "21030",
      :global_shipping => "true",
      :payment_methods => ["PayPal", "VisaMC", "Discover"],
      :auto_pay => "true",
      :condition_id => @product.condition.to_s,
      :paypal_email_address => "info@EMAIL.com",
      :dispatch_time_max => 1,
      :picture_details => remote_images
    })
  end

  def set_product
    @product = Product.find(@product_id)
  end

  def api
    @api ||= Ebay::Api.new
  end
end

Since you are sub-classing ActiveModel::Model you can use
you can use its ability to "magically" map an input hash to attributes:

def initialize(form={})
    super # magic!
    @product = set_product if @product_id
end

This works since you have created accessors for the properties :title, :price, :subtitle, :product_id, :duration.

Also as @nicolas-mccurdy mentioned you can replace each the enumerable methods
 map, select and reject. The rule of thumb being that you should only use each when you are solely concerned with
 the side-effects of a loop.

def item_specifics
    @product.properties.reject { |k,v| v == "0" }.map do |k,v|
        value = "#{v}#{PROPERTY_FORMATTER[k.to_sym]}"
        value = "included" if value == "1"
        Ebay::Types::NameValueList.new(name: k.to_s.for_humans, value: value)
    end
end

def remote_images
    # Side note - where does the variable `url` come from? Document it!
    @product.images.map do |i|
        url = "http://www.howmuchcomputer.com" + i
        remote_url = @api.upload_site_hosted_pictures({:external_picture_url => url})
        remote_images << remote_url.extract(ignore_errors: true)
    end.reverse
end

Also note the use of chaining - extremely common in Ruby - instead of creating intermediate variables or mutating.

The template method looks like it may be violation of MVC - models should not know about template or any kind of
visual representation. Presenting data to users and dealing with user input is the concern of your controllers.

# @todo refactor into controller! Violation of concerns!
def template(path)
    ERB.new(File.open(path, "r").read).result(binding).to_s
end

Also you should consider the visibility of your methods, all methods which you do not consider part of the "public API"
of a class should be private. These look like prime canidates:

def set_product
    @product = Product.find(@product_id)
end

def api
    @api ||= Ebay::Api.new
end

Addition

In general I would question the whole purpose of the class - "form classes" or form builders are pretty common in other frameworks such as Symfony2.
IMHO they are dinosaur concepts from the days of ASP.net, that get filled with gluecode and cruft.

Just stick with good old MVC:

Views create forms and can be DRY-ed with partials and helpers. (or the awesome gem SimpleForm
Controllers deal with the user input and pass DATA to model.
Models deal with getting/persisting data from databases or external sources. They also handle validation. DRY them out with concerns.

If you want to refactor I would consider moving the relevant functionality into Listing - for example the post method.

item_specifics and remote_images is also an example of something that should probably belong to Product.


Compressing time series by removing repeated samples I work on a project with time series data. So there are samples (\$y\$), and each sample has a timestamp (\$x\$). The data will be visualized, but often there are time series which contain samples that do not change over several timestamps. In a visualization (a \$y(x)\$ plot) those repeated samples can be removed, since the visualization will just show a horizontal line. (For example, if a constant value 3.14 is sampled 1 million times in one second, the plot would have the same visual appearance as if the value had been sampled just twice in that period.) I wrote an algorithm that removes those repeated samples in the order to reduce the amount of data and speed up visualization. My algorithm works, but I know that it can be done better.

Here is the algorithm (in the file compress_2D_signal.py):

def compress_2D_signal(x, y):
    end = len(x)
    end_1 = len(x) - 1
    x1 = []
    y1 = []

    x1append = x1.append
    y1append = y1.append

    for ind, elements in enumerate(zip(x, y)):
        if ind == end or ind >= end_1 or y[ind - 1] != y[ind] or y[ind + 1]     != y[ind] or ind == 0:
        x1append(elements[0])
        y1append(elements[1])
    return x1, y1

and here are the tests:

from unittest import TestCase
import time
from compress_2D_signal import compress_2D_signal

class Test2DSignal(TestCase):
    def test_store_sample_array_one_signal_parameter_returns_same_when_each_sample_different(self):
        x = [1, 2, 3, 4]
        y = [0, 1, 2, 3]
        x1, y1 = compress_2D_signal(x, y)
        self.assertEqual(x, x1)
        self.assertEqual(y, y1)

    def test_store_sample_array_one_signal_parameter(self):
        x = [1, 2, 3, 4, 5]
        y = [0, 2, 2, 2, 0]
        x1, y1 = compress_2D_signal(x, y)
        self.assertEqual([1, 2, 4, 5], x1)
        self.assertEqual([0, 2, 2, 0], y1)

    def test_store_sample_array_one_signal_parameter_2(self):
        x = [1, 2, 3, 4, 5, 6, 7, 8, 9]
        y = [0, 2, 2, 2, 0, 0, 1, 1, 1]
        x1, y1 = compress_2D_signal(x, y)
        self.assertEqual([1, 2, 4, 5, 6, 7, 9], x1)
        self.assertEqual([0, 2, 2, 0, 0, 1, 1], y1)

    def test_time_self(self):
        length = int(1e6)
        x = range(length)
        y = range(length)
        start = time.time()
        compress_2D_signal(x, y)
        stop = time.time()
        print('Time elapsed for %s points : %s this are %s points/second' % (length, stop - start, length / (stop - start)))

You want to keep points where the value for \$y\$ is different from either the preceding or succeeding value. So in NumPy you can write:

import numpy as np

def compress_2D_signal(x, y):
    """Compress signal y(x) by omitting repeated values for y.

    Takes a signal y(x) as two array-likes x and y. Returns the
    compressed signal y1(x1) as the tuple x1, y1, where y1 contains
    the first and last values of y, and values of y that are different
    from the preceeding or succeeding value, and x1 contains the
    corresponding values from x.

    """
    x, y = np.asarray(x), np.asarray(y)
    keep = np.empty_like(x, dtype=bool)
    if len(x) > 0:
        keep[0] = keep[-1] = True
        keep[1:-1] = (y[1:-1] != y[:-2]) | (y[1:-1] != y[2:])
    return x[keep], y[keep]

Notes on this code:

This returns NumPy arrays (not Python lists), so you'll need to revise your test cases to use numpy.testing.assert_array_equal instead of unittest.TestCase.assertEqual.
If you pass it Python lists, then much of the runtime will be in the numpy.asarray call (converting the input to NumPy arrays) and not in the compression code. So for a fair test I think it makes sense to update test_time_self so that it calls numpy.arange instead of range.

With this change, I find that the revised code is about 30 times faster than the original code.

A couple of other review points:

There's no docstring for compress_2D_signal. It's hard to use and maintain code when there's no documentation.
The test case code is very repetitive — it would be simpler to loop over a list of cases, like this:

from numpy.testing import assert_array_equal

class Test2DSignal(TestCase):
    _CASES = [
        # x, y, x1, y1
        ([1, 2, 3, 4], [0, 1, 2, 3],
         [1, 2, 3, 4], [0, 1, 2, 3]),
        ([1, 2, 3, 4, 5], [0, 2, 2, 2, 0],
         [1, 2, 4, 5], [0, 2, 2, 0]),
        ([1, 2, 3, 4, 5, 6, 7, 8, 9], [0, 2, 2, 2, 0, 0, 1, 1, 1],
         [1, 2, 4, 5, 6, 7, 9], [0, 2, 2, 0, 0, 1, 1]),
    ]

    def test_2D_signal(self):
        for x, y, x1_expected, y1_expected in self._CASES:
            x1_found, y1_found = compress_2D_signal(x, y)
            assert_array_equal(x1_expected, x1_found)
            assert_array_equal(y1_expected, y1_found)

This makes it easier to add new cases. In particular, I would add tests for edge cases such as empty and single-element inputs, like this:

    _CASES = [
        # x, y, x1, y1
        ([], [], [], []),
        ([1], [1], [1], [1]),
        ([1, 2], [1, 1], [1, 2], [1, 1]),
        ([1, 2, 3], [1, 1, 1], [1, 3], [1, 1]),
        # etc.